feat(rebalancer-sim): Add MockActionTracker for ProductionRebalancer inflight tracking#8033
Conversation
paulbalaji
changed the title
paulbalaji
changed the title
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Correctness concern: Pending item removal by origin+destination only
The MockInflightContextAdapter removes pending items by matching origin + destination only:
removePendingTransfer(origin: string, destination: string): boolean {
const idx = this.pendingRebalances.findIndex(
(r) => r.origin === origin && r.destination === destination,
);
// removes first match...
}Problem: If there are multiple concurrent transfers on the same route (e.g., two user transfers chain1→chain2), removing one will remove the wrong item (first match, not the specific transfer that completed). This corrupts inflight totals → wrong reserve math → incorrect strategy decisions.
Suggestion: Add transfer ID to callbacks and match on ID when removing:
// types.ts
interface InflightContextCallbacks {
onTransferInitiated: (id: string, origin: string, destination: string, amount: bigint) => void;
onTransferDelivered: (id: string) => void; // just need id to remove
// ...
}
// MockInflightContextAdapter.ts
interface PendingItem extends Route {
id: string;
}
removePendingTransfer(id: string): boolean {
const idx = this.pendingTransfers.findIndex((r) => r.id === id);
// ...
}The SimulationEngine already has transfer IDs available (transfer.id, message.transferId) - just need to wire them through.
Lower priority: The hard equality assertions (expect(...).to.equal(0), expect(...).to.equal(1.0)) could be flaky with timing variance. Consider tolerances like lessThanOrEqual(0.05) for completion rate checks. But this is less critical than the ID issue.
📝 WalkthroughWalkthroughReplaces legacy bridge/message trackers with a unified MockInfrastructureController and MockActionTracker; SimulationEngine adds private buildHyperlaneCore(); RebalancerService accepts an optional IActionTracker; tests and deployments updated for mailbox-aware bridges and dynamic Anvil RPC ports. (≤50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Engine as SimulationEngine
participant Core as HyperlaneCore
participant Controller as MockInfrastructureController
participant Runner as ProductionRebalancerRunner
participant Tracker as MockActionTracker
participant Rebalancer as RebalancerService
Note over Engine,Core: Initialization
Engine->>Core: buildHyperlaneCore()
Engine->>Controller: new(core, domains, config, userDelay, kpiCollector, actionTracker?)
Engine->>Controller: start()
Engine->>Runner: start()
Runner->>Tracker: initialize() (if provided)
Runner->>Rebalancer: start(config with actionTracker?)
Note over Controller,Engine: Incoming Dispatch
Engine->>Controller: mailbox Dispatch event
Controller->>Controller: classify message (user/bridge), compute messageId, decode amount
Controller->>Controller: enqueue delivery with delay
Note over Controller,Tracker: Delivery processing
Controller->>Controller: process ready message (callStatic then process tx)
Controller->>Tracker: record KPI / create/complete actions/intents (if tracker present)
Controller->>Rebalancer: (KPI updates / delivery notifications)
Tracker-->>Rebalancer: intent/action records reflected back
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
typescript/rebalancer-sim/src/SimulationEngine.ts (1)
391-415:⚠️ Potential issue | 🟡 MinorClear inflight pending transfers on timeout.
When the wait times out,MessageTrackeris cleared but inflight context still thinks transfers are pending. That can skew rebalancer decisions in the tail end of the run.Suggested fix
for (const msg of pending) { logger.warn( { messageId: msg.id, origin: msg.origin, destination: msg.destination, status: msg.status, attempts: msg.attempts, error: msg.lastError || 'timeout', }, 'Marking pending message as failed', ); // Record as failed in KPI collector this.kpiCollector?.recordTransferFailed(msg.transferId); + // Also clear inflight pending state + this.inflightCallbacks?.onTransferDelivered(msg.transferId); }
🤖 Fix all issues with AI agents
In `@typescript/rebalancer-sim/src/runners/ProductionRebalancerRunner.ts`:
- Around line 308-343: The inflight callbacks use non-null assertions on
this.mockAdapter and can throw if stop() clears the adapter; update
getInflightCallbacks so each callback first checks that the adapter still exists
(e.g., read a local const adapter = this.mockAdapter and return early/no-op if
undefined) instead of calling this.mockAdapter! directly; apply this change for
onTransferInitiated, onTransferDelivered, onRebalanceInitiated and
onRebalanceDelivered so they safely no-op when the adapter has been cleared.
In `@typescript/rebalancer-sim/test/integration/inflight-guard.test.ts`:
- Around line 102-116: The test currently uses conditional guards
(productionResult and simpleResult) which can allow missing rebalancer results
to silently pass; update the test to explicitly fail when results are absent by
asserting presence of productionResult and simpleResult before using their
properties (e.g., assert/expect that productionResult exists and, when
simpleResult is required for comparison, that simpleResult exists) so the test
fails loudly if a rebalancer is missing; locate the checks around
productionResult.kpis and simpleResult.kpis in inflight-guard.test.ts and add
presence assertions for productionResult and simpleResult immediately before the
existing kpi assertions.
🧹 Nitpick comments (3)
typescript/rebalancer-sim/src/SimulationEngine.ts (1)
97-149: Wrap inflight callback calls with error isolation.
These are externalized callbacks; a throw here could derail the sim loop. A tiny helper keeps it safe and consistent.Suggested refactor
+ private notifyInflight<K extends keyof InflightContextCallbacks>( + key: K, + ...args: Parameters<InflightContextCallbacks[K]> + ): void { + if (!this.inflightCallbacks) return; + try { + this.inflightCallbacks[key](...args); + } catch (error: unknown) { + logger.error({ error, key }, 'Inflight callback failed'); + throw error; + } + }- this.inflightCallbacks?.onTransferDelivered(message.transferId); + this.notifyInflight('onTransferDelivered', message.transferId); ... - this.inflightCallbacks?.onRebalanceInitiated( + this.notifyInflight( + 'onRebalanceInitiated', event.transfer.id, event.transfer.origin, event.transfer.destination, event.transfer.amount, ); ... - this.inflightCallbacks?.onRebalanceDelivered(event.transfer.id); + this.notifyInflight('onRebalanceDelivered', event.transfer.id); ... - this.inflightCallbacks?.onTransferInitiated( + this.notifyInflight( + 'onTransferInitiated', transfer.id, transfer.origin, transfer.destination, transfer.amount, );As per coding guidelines: “Use try/catch for external system calls; log and rethrow; don’t swallow errors.”
Also applies to: 162-166, 322-328
typescript/rebalancer/src/core/RebalancerService.ts (2)
443-446: Use assert for the rebalancer precondition.
The warn+return can hide misconfiguration. Better to fail fast.Suggested fix
- if (!this.rebalancer) { - this.logger.warn('Rebalancer not available, skipping'); - return; - } + assert(this.rebalancer, 'Rebalancer not available');As per coding guidelines: “Use assert() for preconditions/invariants; fail fast with clear messages.”
617-620: Don’t swallow rebalance errors in the fallback path.
catch (error: any)both violates the typing rule and hides failures. Preferunknown, log, and rethrow so callers can decide.Suggested fix
- } catch (error: any) { + } catch (error: unknown) { this.metrics?.recordRebalancerFailure(); this.logger.error({ error }, 'Error while rebalancing'); + throw error; }As per coding guidelines: “Prefer catch (unknown) and narrow via type guards; avoid catch (any) in new code” and “Use try/catch for external system calls; log and rethrow; don’t swallow errors.”
…nRebalancer Adds IInflightContextAdapter interface to rebalancer package and MockInflightContextAdapter to rebalancer-sim, enabling simulation of inflight context tracking without requiring a real ActionTracker/ExplorerClient. Changes: - Add IInflightContextAdapter interface to tracking module - Add optional inflightContextAdapter to RebalancerServiceConfig - Add executeWithoutTracking() fallback when ActionTracker unavailable - Create MockInflightContextAdapter maintaining pending rebalances/transfers - Wire SimulationEngine events to mock adapter callbacks - Update tests with assertions for inflight-guard and blocked-user-transfer scenarios Test results: - inflight-guard: ProductionRebalancer uses 1 rebalance vs SimpleRebalancer's 6 (83% reduction) - blocked-user-transfer: ProductionRebalancer achieves 100% completion vs SimpleRebalancer's 0% Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…f origin/destination Addresses PR review feedback - matching pending items by origin+destination could remove the wrong item when multiple concurrent transfers exist on the same route. Now uses unique transfer IDs for correct removal. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
nambrot
force-pushed
the
nam/mock-inflight-context-adapter
branch
from
f3f71b4 to
96c8128
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@typescript/rebalancer/src/core/RebalancerService.ts`:
- Around line 561-620: In executeWithoutTracking, don't swallow errors from the
external call to this.rebalancer.rebalance: update the catch from catch (error:
any) to catch (error: unknown), keep the metrics?.recordRebalancerFailure() and
logger.error(...) but then rethrow the caught error (throw error) so callers can
handle it; this change should be applied inside the executeWithoutTracking
method that wraps the call to this.rebalancer.rebalance.
🧹 Nitpick comments (1)
typescript/rebalancer-sim/test/integration/full-simulation.test.ts (1)
255-270: Tighten the block comment. It’s a bit long; a short summary will do.As per coding guidelines: “Be extremely concise in code comments and documentation.”Possible trim
- /** - * Blocked User Transfer Test - * - * Tests that the ProductionRebalancer proactively adds collateral when - * user transfers are pending but blocked due to insufficient collateral. - * - * Scenario: 130 total tokens split 90/40 between chain1/chain2. - * User initiates 50 token transfer from chain1 → chain2. - * chain2 only has 40 tokens but needs 50 to pay out. - * - * SimpleRebalancer: Only sees on-chain balances, doesn't know about pending - * transfer, weights appear within tolerance → no action → transfer stuck - * - * ProductionRebalancer: Mock adapter tracks pending transfer, strategy - * reserves collateral for it, detects deficit → rebalances → transfer succeeds - */ + /** + * Blocked transfer: pending user transfer needs collateral. + * Scenario: 90/40 split, 50 transfer chain1→chain2; Simple=stuck, Production=rebalance. + */
|
Going to change the mocking abstraction, dont love it |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8033 +/- ##
=======================================
Coverage 77.02% 77.02%
=======================================
Files 117 117
Lines 2651 2651
Branches 244 244
=======================================
Hits 2042 2042
Misses 593 593
Partials 16 16
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…htContextAdapter The simulation now mocks at the ActionTracker level rather than the InflightContextAdapter level. This provides cleaner integration with RebalancerService and properly demonstrates inflight tracking benefits. Key changes: - Add MockActionTracker implementing IActionTracker interface - Remove MockInflightContextAdapter (replaced by MockActionTracker) - Update RebalancerService to accept optional actionTracker config - Handle MockValueTransferBridge not emitting Dispatch events: - failRebalanceIntent keeps intent as in_progress (bridge succeeded) - createActionForPendingIntent creates actions when bridge events fire - completeRebalanceByRoute marks actions complete on delivery Results demonstrate inflight tracking benefits: - inflight-guard: ProductionRebalancer 5 rebalances vs SimpleRebalancer 18 - blocked-user-transfer: ProductionRebalancer 100% completion vs 0% Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
nambrot
changed the title
- inflight-guard: Assert ProductionRebalancer uses >50% fewer rebalances than SimpleRebalancer (currently achieves ~75% reduction) - Update test description to reflect working inflight tracking - Update blocked-user-transfer comment to use correct terminology Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@typescript/rebalancer/src/core/RebalancerService.ts`:
- Around line 189-205: The injected ActionTracker passed in via
this.config.actionTracker may not be initialized; ensure you call and await its
initialize() before using it (or check/assert its initialized state) in the
branch that sets this.actionTracker and this.inflightContextAdapter;
specifically, after assigning this.actionTracker in RebalancerService (the
branch that uses this.config.actionTracker) call await
this.actionTracker.initialize() (or verify an isInitialized method) and only
then proceed to create InflightContextAdapter or log 'Using externally provided
ActionTracker'.
…er (#8060) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
nambrot
requested review from
antigremlin,
larryob,
ltyu,
xeno097 and
yorhodes
as code owners
|
Claude finished @nambrot's task in 5m 19s —— View job Trail of Bits Security Review
Scope
Overall Assessment: LOW riskAll changes are in mock/simulation code. The Solidity contract is test-only and properly guarded by Router's Findings
ToB-1 | LOW —
|
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@solidity/contracts/mock/MockValueTransferBridge.sol`:
- Around line 25-31: The MockValueTransferBridge instances are left
uninitialized in some tests; after deploying each MockValueTransferBridge call
its initialize(...) method (which delegates to _MailboxClient_initialize) with
the correct hook, ism and owner addresses used elsewhere in tests so the mutable
state is configured; update the warp-rebalancer.e2e-test deployment logic to
invoke bridge.initialize(hookAddress, ismAddress, ownerAddress) for every
deployed MockValueTransferBridge (same pattern as SimulationDeployment.ts where
Router requires initialization).
In `@typescript/rebalancer-sim/src/MockInfrastructureController.ts`:
- Around line 325-339: The timeout handling in the pendingMessages loop
currently calls this.kpiCollector.recordRebalanceFailed(msg.messageId) but then
marks the action as completed by calling
this.actionTracker.completeRebalanceByRoute(...), which incorrectly increments
fulfillment; change this to mark the action as failed instead: add/raise use of
a fail path on MockActionTracker (e.g., implement a failRebalanceByRoute in
MockActionTracker or call an existing failRebalanceAction action), and replace
the completeRebalanceByRoute(...) call in the branch handling msg.type ===
'bridge-transfer' with the new fail method so the KPI and action tracker
semantics remain consistent (refer to pendingMessages loop,
recordRebalanceFailed, completeRebalanceByRoute, MockActionTracker,
failRebalanceByRoute / failRebalanceAction).
- Around line 268-277: The conditional in MockInfrastructureController that
guards calling actionTracker.completeRebalanceByRoute incorrectly treats a
bigint 0n as falsy; change the if-check around msg.amount in the bridge-transfer
branch to an explicit existence check (e.g., msg.amount !== undefined &&
msg.amount !== null or typeof msg.amount === 'bigint') so zero-amount rebalances
are still forwarded to actionTracker.completeRebalanceByRoute (referencing the
bridge-transfer branch, this.actionTracker, and completeRebalanceByRoute).
- Around line 246-252: The catch block around mailbox.callStatic.process in
MockInfrastructureController silently swallows errors; update it to catch the
error object (e.g., catch (err)) and log the failure before continuing — include
the error and relevant message context (msg.id or msg.message and
attempts/deliveryTime) using the controller's logger (this.logger.error) or
console.error if no logger exists, then keep the existing retry behavior
(msg.attempts++, msg.deliveryTime = now + 200, continue).
In `@typescript/rebalancer-sim/src/runners/MockActionTracker.ts`:
- Around line 260-275: completeRebalanceAction currently increments
intent.fulfilledAmount but never marks the parent intent complete; update
completeRebalanceAction (in MockActionTracker) to, after increasing
intent.fulfilledAmount and setting intent.updatedAt, check if
intent.fulfilledAmount >= intent.amount and if so set intent.status = 'complete'
and update intent.updatedAt (and optionally log the transition) so behavior
matches completeRebalanceByRoute's completion logic.
In `@typescript/rebalancer-sim/test/integration/full-simulation.test.ts`:
- Around line 275-282: Guard against division-by-zero when computing
reductionRatio: check simpleResult.kpis.totalRebalances (and optionally
productionResult.kpis.totalRebalances) before dividing; if
simpleResult.kpis.totalRebalances === 0, fail the test with a clear
assertion/message (e.g., assert.fail or expect(...).to.equal(...) with a
descriptive message) explaining that SimpleRebalancer produced zero rebalances,
otherwise compute reductionRatio as productionResult.kpis.totalRebalances /
simpleResult.kpis.totalRebalances and run the existing
expect(reductionRatio).to.be.lessThan(0.5) assertion. Ensure you reference the
same symbols used in the diff: simpleResult.kpis.totalRebalances,
productionResult.kpis.totalRebalances, and reductionRatio.
🧹 Nitpick comments (11)
solidity/contracts/mock/MockValueTransferBridge.sol (1)
21-23:collateralcan beimmutablesince it's only assigned in the constructor.It's set once and never touched again — like an ogre's favorite swamp, it ain't movin'. Marking it
immutablesaves a storage slot read on every access.🧅 Proposed fix
- address public collateral; + address public immutable collateral;typescript/rebalancer-sim/src/types.ts (1)
10-11: ConcreteMockActionTrackerreturn type on a general interface — worth a ponder.The
IRebalancerRunnerinterface lives in the shared types file but now returns a concrete simulation class. This means any non-simulation implementer ofIRebalancerRunnerhas to know aboutMockActionTracker. SinceIActionTrackeris already exported from the package, returning that interface (or a broader simulation-specific interface covering the extra helpers) would keep this swamp tidier.That said, I see you've already noted you plan to rework the mocking abstraction, so this might sort itself out like onion layers. Just flagging it so it doesn't get lost in the shuffle.
Also applies to: 418-424
typescript/rebalancer-sim/src/runners/ProductionRebalancerRunner.ts (1)
296-302: Return type is narrower than the interface contract — fine, but worth noting.
getActionTracker()here always returnsMockActionTracker(non-optional), whileIRebalancerRunnerdeclares the return asMockActionTracker | undefined. TypeScript allows this narrowing. Just make sure callers obtained through the interface still handleundefined.typescript/rebalancer-sim/test/integration/full-simulation.test.ts (1)
253-264: Soft guards (if (productionResult)) mean the test passes vacuously when a rebalancer is filtered out.This is intentional for
REBALANCERSenv-var flexibility, but it's worth knowing that running with a single rebalancer type silently skips half the assertions. No action needed unless you want a stricter stance.typescript/rebalancer-sim/src/SimulationEngine.ts (3)
137-149: Barecatch {}blocks silently swallow errors during cleanup.The coding guidelines are pretty clear about never swallowing errors silently. Even in a finally block, a quick debug log helps when something's rotting in the swamp and you're trying to figure out where the smell's coming from.
Suggested fix
try { await rebalancer.stop(); - } catch { - // Ignore stop errors + } catch (error: unknown) { + logger.debug({ error }, 'Rebalancer stop failed during cleanup'); } if (controller) { try { await controller.stop(); - } catch { - // Ignore stop errors + } catch (error: unknown) { + logger.debug({ error }, 'Controller stop failed during cleanup'); } }As per coding guidelines: "Never swallow errors silently; always log or re-throw caught exceptions" and "When catching errors, prefer
catch (error: unknown)with type guards overcatch (e: any)in new code."
230-245:catch (error)should becatch (error: unknown)per guidelines.Small thing, but the coding guidelines prefer the explicit
unknowntype annotation to encourage proper narrowing. You're already doing the narrowing correctly on line 234, so it's just the declaration.Suggested fix
- } catch (error) { + } catch (error: unknown) {As per coding guidelines: "When catching errors, prefer
catch (error: unknown)with type guards overcatch (e: any)in new code."
278-309:buildHyperlaneCore()— clean helper, duplicates chain metadata construction seen inProductionRebalancerRunner.This method and the one in
ProductionRebalancerRunner.ts(lines 144-165) both construct very similar chain metadata objects withchainId: 31337,protocol: ProtocolType.Ethereum, same RPC URL, same native token config. Not a blocker — they serve slightly different purposes (different signers, different polling needs) — but if the metadata shape drifts between the two, debugging gets murky. Something to keep an eye on for the mocking abstraction rework.typescript/rebalancer-sim/src/MockInfrastructureController.ts (3)
42-58: Consider guardingsignerwithassert()instead of!.The
signer!: ethers.Signernon-null assertion on line 44 means any accidental access beforestart()silently producesundefinedat runtime rather than a clear error. Since the coding guidelines preferassert()for preconditions, you could initialize it asprivate signer: ethers.Signer | undefinedand assert at usage sites, or assert at the top ofdoProcessReadyMessages. Not a swamp-drainer, but it keeps things honest.As per coding guidelines, "Use assert() for validating preconditions and invariants instead of throwing errors directly".
146-167: Hardcoded body offset (77) and scale factor (1e18) are brittle magic numbers.These values are tightly coupled to the current TokenRouter message format and warp token decimal configuration. If the message envelope or token decimals change, the decode silently produces wrong amounts. This is tolerable for a controlled simulation, but extracting these as named constants (or deriving from config) would make the coupling explicit and easier to update.
Also, the
amountsilently stays0non decode failure (line 152). Worth a log-level bump fromwarntoerrorif the message must carry an amount for correct KPI tracking — a zero-amount rebalance could quietly skew metrics.Extract constants
+const MESSAGE_BODY_OFFSET = 77; +const WARP_TOKEN_SCALE = BigInt(1e18); + // inside onDispatch: - const bodyOffset = 77; - const body = '0x' + message.slice(2 + bodyOffset * 2); + const body = '0x' + message.slice(2 + MESSAGE_BODY_OFFSET * 2); ... - type === 'user-transfer' ? scaledAmount / BigInt(1e18) : scaledAmount; + type === 'user-transfer' ? scaledAmount / WARP_TOKEN_SCALE : scaledAmount;
294-305:removeAllListeners()nukes all listeners, not just ours.
mailbox.removeAllListeners()at line 303 removes every listener on the contract, including any that other components may have registered. In a simulation this is probably fine since the controller owns the mailbox lifecycle, but if the scope ever widens, you'd want to track and remove only your own listeners. Just somethin' to keep in the back of your mind, like an ogre keeps... layers.typescript/rebalancer-sim/src/runners/MockActionTracker.ts (1)
14-58: Consider exporting and importing the tracking types from@hyperlane-xyz/rebalancerinstead of redefining them locally.The
Transfer,RebalanceIntent, andRebalanceActioninterfaces already exist intypescript/rebalancer/src/tracking/types.tsbut aren't currently part of the package's public API exports. Right now you're rolling your own versions here, which creates a maintenance burden if the source types ever shift. If they change upstream, these local copies won't know about it.Best move would be to export these types from the rebalancer package's index, then pull 'em in here. Keeps things in one place rather than scattered around like layers of an onion. If exporting them breaks something, you could always import directly from the internal path (
@hyperlane-xyz/rebalancer/dist/tracking/types), though that's a bit less tidy.
- Log static-call pre-check failures instead of silently swallowing - Use explicit `> 0n` check instead of falsy check for bigint amount - Add failRebalanceByRoute to MockActionTracker for timeout path (timed-out transfers should not be marked as complete) - Auto-complete parent intent in completeRebalanceAction - Add division-by-zero guard in inflight-guard test - Call initialize() on MockValueTransferBridge in e2e tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@typescript/rebalancer-sim/src/runners/MockActionTracker.ts`:
- Around line 14-58: Replace the local interface declarations Transfer,
RebalanceIntent, and RebalanceAction in MockActionTracker.ts with imports from
the canonical package: add an import type statement that brings in Transfer,
RebalanceIntent, and RebalanceAction from '@hyperlane-xyz/rebalancer' and then
delete the three local interface blocks; ensure any code in MockActionTracker.ts
that referenced the local types uses the imported names unchanged so types line
up with the source of truth.
In `@typescript/rebalancer-sim/test/integration/full-simulation.test.ts`:
- Around line 333-355: The test currently guards assertions behind "if
(simpleResult)" and "if (productionResult)", which lets the test silently pass
when results are missing; remove those conditional guards and instead assert
presence of results first (e.g., use expect(simpleResult, 'missing
SimpleRebalancer result').to.exist and expect(productionResult, 'missing
ProductionRebalancer result').to.exist) and then run the existing KPI assertions
against simpleResult.kpis and productionResult.kpis so failures are explicit;
update references to simpleResult and productionResult accordingly and keep the
existing KPI checks unchanged.
- Around line 244-264: The test currently uses conditional checks (if
(productionResult) / if (simpleResult)) which let the test pass silently when
results.find(...) returns undefined; change these to explicit existence
assertions before checking KPIs — e.g., assert that productionResult and
simpleResult are defined (using expect(productionResult).to.exist /
expect(simpleResult).to.exist or expect(...).to.not.be.undefined) and then
perform the completionRate assertions on productionResult.kpis and
simpleResult.kpis; apply the same fix to the blocked-user-transfer test by
replacing its conditional existence checks with explicit expects so the test
fails fast when the expected result objects are missing.
🧹 Nitpick comments (6)
typescript/rebalancer-sim/src/runners/MockActionTracker.ts (3)
191-216: Silent no-ops when entities aren't found — that'll come back like an unwelcome visitor.Methods like
completeRebalanceIntent,cancelRebalanceIntent,failRebalanceIntent, andfailRebalanceActionsilently do nothing when the ID doesn't exist. In a simulation, calling these with a stale or wrong ID is likely a bug worth knowing about. At minimum, alogger.warnwould save someone a debugging headache.Example for one method — same pattern for others
async completeRebalanceIntent(id: string): Promise<void> { const intent = this.intents.get(id); - if (intent) { - intent.status = 'complete'; - intent.updatedAt = Date.now(); - logger.debug({ id }, 'Rebalance intent completed'); + if (!intent) { + logger.warn({ id }, 'completeRebalanceIntent: intent not found'); + return; } + intent.status = 'complete'; + intent.updatedAt = Date.now(); + logger.debug({ id }, 'Rebalance intent completed'); }As per coding guidelines, "Never swallow errors silently; always log or re-throw caught exceptions."
306-312:clear()resetsidCounter— be mindful of ID collisions if interleaved with lookups.Resetting
idCounterto 0 after clearing is fine for full test isolation, but if something still holds references to old IDs (e.g., async callbacks in flight), newly generated IDs could collide. Since this is test-only and meant for clean resets between runs, it's probably fine — just noting it.
322-385:createActionForPendingIntentmatches by exactamount— might miss partial fills or rounding.The filter uses strict equality
i.amount === amount(line 335). If bridge transfers ever have amounts that differ slightly (fees, rounding), this won't match. In the current sim where amounts are controlled, this is okay, but it's worth a comment noting the assumption.Also, this method is synchronous while
createRebalanceActionis async — minor inconsistency in the API surface but not a functional concern.typescript/rebalancer-sim/src/MockInfrastructureController.ts (3)
91-101: Fire-and-forgetonDispatch— unhandled rejections could vanish into the swamp.
void this.onDispatch(...)suppresses the floating-promise lint warning but ifonDispatchthrows, it becomes an unhandled promise rejection. In simulation this might just cause a test to hang mysteriously rather than fail clearly.Catch and log the rejection
mailbox.on( mailbox.filters.Dispatch(), ( sender: string, destination: number, _recipient: string, message: string, ) => { - void this.onDispatch(chainName, sender, destination, message); + this.onDispatch(chainName, sender, destination, message).catch( + (error: unknown) => { + logger.error( + { chainName, error }, + 'Unhandled error in onDispatch', + ); + }, + ); }, );As per coding guidelines, "Always log or re-throw errors; never swallow errors silently."
146-167: Hardcoded offset77and scale1e18— a couple o' magic numbers that deserve their own names.The body offset (77 bytes) is the Hyperlane message header size, and
1e18is the warp token decimal scaling. Both are brittle if the message format or token configuration changes. Extracting these as named constants would make intent clearer and updates easier.Also, if decoding fails,
amountstays at0n, which means downstream tracking (KPI, actionTracker) silently records a zero-amount transfer. Thelogger.warnis good, but this defensive fallback could mask real issues — a failed decode for a bridge transfer meanscompleteRebalanceByRoutewon't find a matching action later (amount mismatch).Extract constants and consider failing loudly
+const HYPERLANE_MESSAGE_BODY_OFFSET = 77; +const WARP_TOKEN_SCALE = BigInt(1e18); + // Decode amount from body - const bodyOffset = 77; + const bodyOffset = HYPERLANE_MESSAGE_BODY_OFFSET; const body = '0x' + message.slice(2 + bodyOffset * 2); let amount = 0n; try { const decoded = ethers.utils.defaultAbiCoder.decode( ['bytes32', 'uint256'], body, ); const scaledAmount = decoded[1].toBigInt(); - amount = type === 'user-transfer' ? scaledAmount / BigInt(1e18) : scaledAmount; + amount = type === 'user-transfer' ? scaledAmount / WARP_TOKEN_SCALE : scaledAmount; } catch (error) {
42-49:signer!definite assignment assertion — lifecycle coupling not enforced.
this.signeris declared with!(line 44) meaning TS trusts it's assigned before use. IfprocessReadyMessagessomehow fires beforestart()completes (e.g., during tests), you'd get a runtime error with no helpful message. Anassertat the top ofdoProcessReadyMessageswould be a safer contract.That said, the
nonceInitializedguard at line 237 provides some protection, though it re-fetches the nonce rather than guarding against a missing signer.
…c loop Txs are sequential (await tx.wait()), so ethers handles nonces naturally. Replace setInterval + re-entry guard with simple while loop + sleep. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@typescript/rebalancer-sim/src/MockInfrastructureController.ts`:
- Around line 74-96: The start method in MockInfrastructureController currently
calls the async onDispatch fire-and-forget (via void) and allows unhandled
rejections; change the mailbox handler in start() to capture the returned
promise and handle rejections (e.g., call onDispatch(...).catch(...) or wrap in
try/catch via an async wrapper) and log the error; additionally, modify
onDispatch to defensively check this.domains[originChain] (or
this.domains[chainName]) at the top and bail with a clear logged error if the
domain is missing to avoid a null/undefined access and surface the problem.
🧹 Nitpick comments (4)
typescript/rebalancer-sim/src/MockInfrastructureController.ts (4)
17-32: Well-structured interface — nice and clean.One small thing: the
typefield uses string literals where the coding guidelines prefer enum values for discriminant fields. For an internal simulation type with just two values, this is fine to defer. As per coding guidelines, "Use enum values for discriminant fields in interfaces instead of string literals."
134-155: Magic number77for body offset — give it a name so it doesn't just lurk in the code like a fairy-tale creature.This protocol-level constant deserves to be a named constant for clarity and to avoid accidental modification. Also,
BigInt(1e18)on line 149 could be extracted alongside it.Suggested extraction
+/** Byte offset into a Hyperlane message where the TokenRouter body starts */ +const TOKEN_MESSAGE_BODY_OFFSET = 77; +const WARP_TOKEN_SCALE = BigInt(1e18); + ... - const bodyOffset = 77; - const body = '0x' + message.slice(2 + bodyOffset * 2); + const body = '0x' + message.slice(2 + TOKEN_MESSAGE_BODY_OFFSET * 2); ... - type === 'user-transfer' ? scaledAmount / BigInt(1e18) : scaledAmount; + type === 'user-transfer' ? scaledAmount / WARP_TOKEN_SCALE : scaledAmount;
150-150: Prefer explicitcatch (error: unknown)per coding guidelines.All three catch blocks use
catch (error)without the explicit type annotation. The guidelines ask forcatch (error: unknown)in new code.Quick diff
- } catch (error) { + } catch (error: unknown) {(Apply at lines 150, 223, and 260.)
As per coding guidelines, "When catching errors, prefer
catch (error: unknown)with type guards overcatch (e: any)in new code."Also applies to: 223-223, 260-260
286-288:removeAllListeners()is a broad stroke — clears every listener on the mailbox, not just ours.In the simulation context this is likely fine since the controller owns these contract instances. But if other code ever shares these mailbox references, listeners would be silently lost. Consider storing the listener references and using
mailbox.off(filter, listener)for surgical cleanup.
- Import Transfer/RebalanceIntent/RebalanceAction from @hyperlane-xyz/rebalancer instead of redefining locally (export added to rebalancer index) - Catch unhandled rejections from async onDispatch in event listener - Guard domains[originChain] access in onDispatch Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Bridge extends Router, so _Router_dispatch requires a remote router enrolled for the destination domain. Without it, transferRemote reverts and the SentTransferRemote event listener hangs forever. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
solidity/contracts/mock/MockValueTransferBridge.sol (1)
61-66:⚠️ Potential issue | 🟡 Minor
block.chainidtruncated touint32— fine for known test chains, but worth a wee note.
block.chainidisuint256; casting touint32silently drops the upper bits. For Anvil/Hardhat test chains this won't bite, but if this mock were ever reused on a chain with a large chain ID, the event would log a wrong origin. Also,block.chainiddoesn't necessarily equal the Hyperlane domain ID — in production those are separate concepts.Since it's a mock, this is just something to keep in mind rather than a swamp-level emergency.
🧹 Nitpick comments (1)
solidity/contracts/mock/MockValueTransferBridge.sol (1)
12-12:collateralcould beimmutablesince it's only set in the constructor.It's set once and never mutated again — marking it
immutablewould be the proper way to go here. It's a mock, so it's not like the gas savings matter much in the grand swamp of things, but it keeps the pattern tidy.Suggested diff
- address public collateral; + address public immutable collateral;As per coding guidelines: "In Solidity, use
constantfor compile-time values andimmutablefor constructor-set values."
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
LGTM — core refactor is clean and the inflight tracking benefits are well-demonstrated.
5 minor items as inline comments below. None are blockers.
Already Addressed from Prior Reviews
- ✅ Transfer ID-based removal (vs origin+destination matching)
- ✅
onDispatchrejection handling (.catch()wrapper) - ✅ Domain guard in
onDispatch - ✅
failRebalanceByRoutefor timeout path - ✅
completeRebalanceActionauto-completes parent intent - ✅ Division-by-zero guard in inflight-guard test
- ✅ Types imported from
@hyperlane-xyz/rebalancer - ✅
initialize()called on externally provided ActionTracker - ✅ Dynamic anvil ports
- ✅ Bridge
initialize()+enrollRemoteRouterin e2e tests
- Make collateral immutable in MockValueTransferBridge - Replace Record<string, any> with Record<string, ChainMetadata> - Log errors in catch blocks instead of swallowing silently - Extract magic numbers into named constants (MESSAGE_BODY_OFFSET, WARP_TOKEN_SCALE) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…text-adapter # Conflicts: # typescript/rebalancer-sim/test/integration/full-simulation.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@typescript/rebalancer-sim/src/MockInfrastructureController.ts`:
- Around line 311-345: When timing out in waitForAllDeliveries, avoid replacing
this.pendingMessages because processLoop may be mid-delivery; instead mark each
pending message as cancelled (e.g., add msg.timedOut = true) and call the
failure KPI/actionTracker logic, then remove or keep them as needed. Also update
the delivery path in processLoop (where
recordRebalanceComplete/recordTransferComplete are called) to check msg.timedOut
and skip completion KPIs for timed-out messages. Reference waitForAllDeliveries,
this.pendingMessages, processLoop, recordRebalanceFailed,
recordRebalanceComplete, recordTransferFailed and recordTransferComplete when
making the changes.
🧹 Nitpick comments (7)
solidity/contracts/mock/MockValueTransferBridge.sol (2)
33-47: Hardcoded collateral quote of1— worth a quick note.Right, so
quotes[0] = Quote(collateral, 1)always reports a collateral fee of 1 wei regardless of_amountOut, whiletransferRemoteactually pulls the full_amountOutviasafeTransferFrom. IfQuoterepresents additional fees on top of the transfer amount, this is fine — it's a mock charging a trivial fee. But if any caller uses this quote to determine how much collateral to approve, they'll approve way too little and the transfer will revert.For a mock that only lives in test harnesses, this probably won't bite anyone, but it might cause head-scratching later if someone writes a test that actually checks approval amounts against the quote. A small comment clarifying the intent (e.g.,
// Mock: trivial fee, caller must separately approve _amountOut) would save some time spelunking through the layers.📝 Optional: add a clarifying comment
Quote[] memory quotes = new Quote[](2); - quotes[0] = Quote(collateral, 1); + // Mock fee: callers must separately approve _amountOut for the actual transfer + quotes[0] = Quote(collateral, 1); quotes[1] = Quote(address(0), dispatchFee);
76-88:_handleminting logic — watch the bytes32→address truncation.The conversion
address(uint160(uint256(recipientBytes32)))takes the lower 20 bytes. This is the standard Hyperlane convention (TypeCasts.bytes32ToAddress), so it's consistent with how addresses are encoded elsewhere in the codebase. The minting viaERC20Test.mintTois clean for a mock — no need for actual cross-chain token mechanics.One thing worth considering: there's no guard against
recipient == address(0). If a garbled message arrives (or a test bug encodes a zero recipient), this mints tokens toaddress(0), which is effectively a burn. For a mock contract in a controlled test environment, this is unlikely to matter, but a quickrequire(recipient != address(0))would make debugging easier if it ever does happen.🛡️ Optional: guard against zero-address recipient
address recipient = address(uint160(uint256(recipientBytes32))); + require(recipient != address(0), "MockBridge: zero recipient"); // Mint collateral tokens to recipient (destination warp token) ERC20Test(collateral).mintTo(recipient, amount);typescript/rebalancer-sim/src/MockInfrastructureController.ts (3)
19-20: Prefer10n ** 18nfor the scale constant.
BigInt(1e18)works here because1e18is exactly representable in IEEE 754, but using10n ** 18nis the idiomatic BigInt pattern and sidesteps any reader's doubt about float-to-bigint precision.🔧 Optional tweak
-const WARP_TOKEN_SCALE = BigInt(1e18); +const WARP_TOKEN_SCALE = 10n ** 18n;
153-171: Body decoding looks correct, but narrow the catch type.The hex slicing and ABI decode logic are sound. One small thing though — the
catchat line 166 should type the error asunknownper the project's guidelines. It's not going to break anything, but somebody's going to bring it up eventually.🔧 Minor fix
- } catch (error) { + } catch (error: unknown) {As per coding guidelines, "Catch errors as unknown type and narrow with type guards; avoid catch(e: any) in new code."
254-283: Process + wait pattern is fine for simulation; type the catch.The splice-from-pending-during-iteration is safe since
readyis a separate filtered array. The retry-on-tx-failure path also works because the static pre-check on the next pass will catch already-delivered messages.Same
catch (error: unknown)nit as above on line 276.🔧 Minor fix
- } catch (error) { + } catch (error: unknown) {As per coding guidelines, "Catch errors as unknown type and narrow with type guards; avoid catch(e: any) in new code."
typescript/rebalancer-sim/src/SimulationEngine.ts (2)
229-246: Fallback KPI recording on transfer failure is the right call.Since no Dispatch event fires when the on-chain tx fails, manually recording start+fail keeps KPI counts honest. Same
catch (error: unknown)nit on line 231 though.🔧 Minor fix
- } catch (error) { + } catch (error: unknown) {As per coding guidelines, "Catch errors as unknown type and narrow with type guards; avoid catch(e: any) in new code."
279-310:buildHyperlaneCoreis solid; a couple of small things.
Record<string, ChainMetadata>on line 280 could beChainMap<ChainMetadata>— it's the same type, just the project's preferred alias for per-chain maps.The
ascast on line 305 can be avoided withinstanceof:🔧 Optional cleanup
+import type { ChainMap } from '@hyperlane-xyz/sdk'; // ... - const chainMetadata: Record<string, ChainMetadata> = {}; + const chainMetadata: ChainMap<ChainMetadata> = {}; // ... if (p && 'pollingInterval' in p) { - (p as ethers.providers.JsonRpcProvider).pollingInterval = 100; + if (p instanceof ethers.providers.JsonRpcProvider) { + p.pollingInterval = 100; + } }As per coding guidelines, "Use ChainMap for per-chain configurations" and "avoid unnecessary type casts (as assertions)."
⚙️ Node Service Docker Images Built Successfully
Full image paths |
2 participants
Summary
MockActionTrackerimplementingIActionTrackerinterface for simulation testingactionTrackerconfig toRebalancerServicefor test injectionIActionTrackerand related types from rebalancer packageMockInfrastructureControllerwires Dispatch events to MockActionTracker for inflight trackingMockValueTransferBridgenow extendsRouterso bridge transfers dispatch through MailboxBuilt on top of #8060 which unified
BridgeMockController+MessageTrackerinto a singleMockInfrastructureControllerusingHyperlaneCore.What This Enables
inflight-guard test: ProductionRebalancer now correctly uses fewer rebalances compared to SimpleRebalancer because it tracks pending transfers and doesn't over-correct. Typically achieves >50% reduction in unnecessary rebalances.
blocked-user-transfer test: ProductionRebalancer achieves 100% completion (vs 0% for SimpleRebalancer) by proactively adding collateral when it sees pending user transfers that would otherwise be blocked.
Key Implementation Details
MockActionTrackertracks transfers and rebalance intents/actions in memoryMockInfrastructureControllerauto-classifies Dispatch events (warp vs bridge) and updates the trackerMockValueTransferBridgeextendsRouterso_Router_dispatch()emits real MailboxDispatchevents, enabling black-box observation by the controllerRebalancerServiceaccepts an optionalactionTrackerto skip ExplorerClient-based trackingTest plan
pnpm -C typescript/rebalancer-sim test --grep "inflight-guard"passespnpm -C typescript/rebalancer-sim test --grep "blocked-user-transfer"passespnpm -C typescript/cli buildpasses (e2e tests updated for new MockValueTransferBridge constructor)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Removed
Tests
Contracts