feat(rebalancer): inflight-aware rebalancing system#7921
Conversation
Mo-Hussain
requested review from
Xaroz,
antigremlin,
ltyu,
paulbalaji,
xeno097 and
yorhodes
as code owners
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds inflight-message-aware rebalancing: ActionTracker, InflightContextAdapter, new tracking stores, CollateralDeficitStrategy, CompositeStrategy, inflight-aware BaseStrategy changes, Rebalancer now returns per-route results, and config/schema moves to array-based strategy composition. Changes
Sequence Diagram(s)sequenceDiagram
participant Monitor
participant RebalancerService
participant ActionTracker
participant Explorer
participant Strategy
participant Rebalancer
Monitor->>RebalancerService: tokenInfo (with confirmedBlockTags)
RebalancerService->>ActionTracker: syncActionTracker(confirmedBlockTags)
ActionTracker->>Explorer: getInflightUserTransfers / getInflightRebalanceActions
Explorer-->>ActionTracker: inflight messages
ActionTracker->>ActionTracker: update stores (transfers/intents/actions)
RebalancerService->>ActionTracker: getInflightContext()
ActionTracker-->>RebalancerService: { pendingRebalances, pendingTransfers }
RebalancerService->>Strategy: getRebalancingRoutes(balances, inflightContext)
Strategy-->>RebalancerService: proposed StrategyRoute[]
RebalancerService->>ActionTracker: createRebalanceIntent(...) for each route
RebalancerService->>Rebalancer: rebalance(rebalanceRoutes with intentId)
Rebalancer->>Rebalancer: prepare & execute txs
Rebalancer-->>RebalancerService: RebalanceExecutionResult[]
RebalancerService->>ActionTracker: createRebalanceAction / complete/ fail updates
sequenceDiagram
participant CompositeStrategy
participant StrategyA as CollateralDeficit
participant StrategyB as MinAmount
participant Context as InflightContext
CompositeStrategy->>StrategyA: getRebalancingRoutes(balances, {pending..., proposed: []})
StrategyA-->>CompositeStrategy: routesA (proposed)
CompositeStrategy->>StrategyB: getRebalancingRoutes(balances, {pending..., proposed: routesA})
StrategyB-->>CompositeStrategy: routesB
CompositeStrategy-->>CompositeStrategy: return routesA + routesB
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 8
🤖 Fix all issues with AI agents
In @.changeset/action-tracker-implementation.md:
- Line 5: The changeset used present tense; update the description to past tense
by changing verbs like "tracks", "provides", and "includes" to "tracked",
"provided", and "included" (and any other present-tense verbs) so the
ActionTracker, Transfer, RebalanceIntent, RebalanceAction, IStore, InMemoryStore
and ExplorerClient references are described in past tense and the body conforms
to the changeset guideline requiring past-tense descriptions.
In @.changeset/collateral-deficit-strategy.md:
- Line 5: The changeset used present-tense verbs for the strategy description;
update the description in .changeset/collateral-deficit-strategy.md to past
tense (e.g., change "detects" to "detected" and "proposes" to "proposed") while
keeping "Added CollateralDeficitStrategy" and "Modified reserveCollateral()"
as-is so the file consistently describes what changed.
In @.changeset/composite-strategy.md:
- Line 5: The changeset description used present tense; update it to past tense.
Edit the lines mentioning "are passed" -> "were passed", "now uses" -> "used",
and rephrase the schema sentence to past tense (e.g., "unified schema types by
making bridgeLockTime optional and adding a name property to IStrategy for
better logging"). Keep the rest of the wording consistent with past-tense
changeset style and preserve the same content (CompositeStrategy,
routing/pending rebalances, array config, bridgeLockTime, IStrategy name).
In @.changeset/inflight-aware-rebalancing-major.md:
- Around line 5-25: Update the changeset
.changeset/inflight-aware-rebalancing-major.md to use past-tense wording
throughout: change phrases like "now returns", "now includes", "was renamed",
and any present-tense bullets (e.g., IRebalancer.rebalance() now returns,
IStrategy.getRebalancingRoutes() accepts, IStrategy now requires,
RebalancingRoute renamed, MonitorEvent now includes) to past-tense equivalents
(e.g., "returned", "accepted", "required", "was renamed", "included") so the
file describes what changed rather than what will change.
In `@typescript/rebalancer/src/core/WithInflightGuard.ts`:
- Around line 40-57: The current bridge collection (using getStrategyChainNames
-> getStrategyChainConfig -> chainConfig?.bridge) only grabs a single bridge per
chain and misses overrides/secondary strategies; update the logic that builds
the bridges array (the variable named bridges) to iterate all strategy configs
and their per-route/override entries in this.config.strategyConfig, collect
every bridge referenced (including secondary/override bridges), deduplicate the
list, and then pass that deduped bridges array into
this.explorer.hasUndeliveredRebalance (keeping routersByDomain and txSender as
before) so inflight rebalances on all bridges are considered.
In `@typescript/rebalancer/src/strategy/CompositeStrategy.ts`:
- Around line 13-15: Update the doc comment on class CompositeStrategy to
reflect the actual behavior: change the phrase that currently says
"pendingRebalances" to "proposedRebalances" and adjust the sentence to state
that routes from earlier strategies are passed as proposedRebalances to later
strategies (mentioning CompositeStrategy and the method that
delegates/aggregates strategy outputs such as the strategy execution/composition
method). Ensure the comment wording matches the code path where earlier strategy
outputs are forwarded as proposedRebalances so readers won’t be confused about
parameter names.
In `@typescript/rebalancer/src/tracking/store/InMemoryStore.test.ts`:
- Around line 133-211: The tests use chai-as-promised assertions
(.to.be.rejectedWith and .to.not.be.rejected) but the plugin isn't registered;
import chaiAsPromised (e.g. import chaiAsPromised from 'chai-as-promised') and
call chai.use(chaiAsPromised) near the top of InMemoryStore.test.ts before any
tests run so the promise-based assertions in the update/delete tests work
correctly; ensure the chai and chaiAsPromised symbols are referenced exactly as
used by the other rebalancer tests.
In `@typescript/rebalancer/src/utils/ExplorerClient.ts`:
- Around line 222-232: The GraphQL selection in ExplorerClient that builds the
explorer messages response is missing the origin_tx_recipient field which
ExplorerMessage and normalizeExplorerMessage expect; update the GraphQL query
used in the ExplorerClient (the query block returning fields like msg_id,
origin_domain_id, origin_tx_hash, origin_tx_sender, etc.) to include
origin_tx_recipient so normalizeExplorerMessage receives a defined value and
downstream consumers get the expected field.
🧹 Nitpick comments (21)
typescript/cli/src/context/strategies/chain/chainResolver.ts (1)
141-147: Add a clearer guard for missing/legacy strategy config.Right now a mis-shaped
strategyConfig(old format or missing) will likely blow up insidegetStrategyChainNamesand the CLI error won’t be very friendly. Consider a quick guard so the user gets a clear message.💡 Suggested tweak
- const chains = getStrategyChainNames(rebalancerConfig.strategyConfig); + assert( + Array.isArray(rebalancerConfig.strategyConfig), + 'Invalid rebalancer config: "strategyConfig" must be an array of strategies', + ); + const chains = getStrategyChainNames(rebalancerConfig.strategyConfig);typescript/rebalancer/src/core/WithSemaphore.test.ts (1)
20-25: Trim theas anycasts in test routes/config.
These double-casts can hide API drift. Prefer typed fixtures (orsatisfies RebalanceRoute) and a typed config builder so tests fail when the contract changes.As per coding guidelines: Avoid unnecessary type casts (
asassertions), especiallyas unknown as Xdouble-casts in TypeScript.Also applies to: 50-55, 72-79, 82-86, 99-104
typescript/rebalancer/src/tracking/InflightContextAdapter.test.ts (1)
16-28: Prefer typed stubs/fixtures overas any.
Consider usingsatisfies IActionTracker/satisfies MultiProviderwith stubbed methods, and typed address constants for sender/recipient. It keeps the tests honest without losing Sinon flexibility.As per coding guidelines: Use proper typing over
anyor type assertions in TypeScript.Also applies to: 57-59, 119-121, 171-173, 183-185
typescript/rebalancer/src/core/WithInflightGuard.test.ts (1)
51-53: UsebuildTestRebalanceRoute()instead ofas anyforRebalanceRouteobjects. The test helper already exists and is imported in this file—using it with typed overrides (e.g.,buildTestRebalanceRoute({ origin: 'ethereum', intentId: 'test-1' })) ensures the route shape matches the type and catches any breaking changes to the contract.Lines affected: 51-53, 82-84, 112-114
Replace:
{ origin: 'ethereum', intentId: 'test-1' } as anyWith:
buildTestRebalanceRoute({ origin: 'ethereum', intentId: 'test-1' }).changeset/inflight-aware-base-strategy.md (1)
5-5: Changeset description should use past tense, not present.The coding guidelines say to write these descriptions in past tense. Right now we've got "is extended", "extended", "updated" - those need a wee bit of adjustment to sound like they already happened, not like they're happening now.
✏️ Suggested fix
-BaseStrategy is extended with inflight-aware rebalancing capabilities and bridge configuration support. RebalancingRoute extended with optional bridge field for bridge selection. Added three protected methods: reserveCollateral() to prevent draining collateral needed for incoming transfers, simulatePendingRebalances() for optional balance simulation, and filterRebalances() to filter routes based on actual balance sufficiency. The getRebalancingRoutes() method updated to accept optional inflightContext and integrate the new methods. getCategorizedBalances() signature updated to accept optional pendingRebalances parameter. BaseStrategy, WeightedStrategy, and MinAmountStrategy constructors extended with optional bridges parameter (ChainMap<Address[]>) to store configured bridge addresses per chain. +Extended BaseStrategy with inflight-aware rebalancing capabilities and bridge configuration support. Extended RebalancingRoute with optional bridge field for bridge selection. Added three protected methods: reserveCollateral() to prevent draining collateral needed for incoming transfers, simulatePendingRebalances() for optional balance simulation, and filterRebalances() to filter routes based on actual balance sufficiency. Updated the getRebalancingRoutes() method to accept optional inflightContext and integrate the new methods. Updated getCategorizedBalances() signature to accept optional pendingRebalances parameter. Extended BaseStrategy, WeightedStrategy, and MinAmountStrategy constructors with optional bridges parameter (ChainMap<Address[]>) to store configured bridge addresses per chain.As per coding guidelines: "Write changeset descriptions in past tense describing what changed, not what will change."
typescript/rebalancer/src/utils/ExplorerClient.ts (3)
47-50: ThenormalizeHexfunction returns falsy values as-is, which could cause type issues.When
hexisnullorundefined, this returns that value directly, but the return type is implicitlystring. Not a swamp-sized problem since the caller handles the fields, but something to keep an eye on.const normalizeHex = (hex: string): string => { if (!hex) return hex; // returns undefined/null but typed as string ... };
246-259: Consider extracting the duplicated error handling into a helper method.This error-response parsing logic appears three times in the file (here, lines 134-148, and 342-355). Could be nice and tidy with a wee private helper like
parseErrorResponse(res: Response).
126-130: Fetch calls have no timeout configured.These Explorer API requests could hang if the service is slow or unresponsive. Might want to consider adding an
AbortControllerwith a timeout to keep things from getting stuck in the swamp. Same applies to the other two fetch calls in this file.typescript/rebalancer/src/strategy/StrategyFactory.test.ts (1)
99-106: Worth adding a test forCompositeStrategycreation in the factory tests.Like an ogre with layers, this factory's got 'em—single strategy goes straight through, but multiple strategies wrap up in a
CompositeStrategy. The tests here only check the single-strategy path, so adding one more test that passes a multi-element array would round out the coverage nicely. Not urgent, but it'd be good to have.typescript/rebalancer/src/interfaces/IStrategy.ts (1)
16-25: Bridge the gap with a better type definition.
ThefilterByConfiguredBridgesmethod casts toStrategyRoutejust to grab the bridge property—feels like working around the type system. Since pendingRebalances sometimes has a bridge (like the comment says), might as well type it properly. The tests already do this same pattern, so it's a well-trodden path.♻️ Type adjustment
+export type PendingRebalanceRoute = Route & { bridge?: Address }; + export type InflightContext = { /** * In-flight rebalances from ActionTracker. * Uses Route[] because recovered intents (from Explorer startup recovery) * don't have bridge information. Some routes may have bridge at runtime. */ - pendingRebalances: Route[]; + pendingRebalances: PendingRebalanceRoute[]; pendingTransfers: Route[]; /** Routes from earlier strategies - always have bridge */ proposedRebalances?: StrategyRoute[]; };typescript/rebalancer/src/tracking/store/InMemoryStore.ts (1)
47-51: Theas anycast here is like onion layers - it hides what's underneath.The coding guidelines say to avoid unnecessary type casts and prefer type guards for runtime narrowing. Since
Statusis a generic parameter representing the entity's status field, consider adding a type constraint or helper to avoid theas any:🔧 Suggested approach
- async getByStatus(status: Status): Promise<T[]> { - return Array.from(this.data.values()).filter( - (entity) => (entity as any).status === status, - ); - } + async getByStatus(status: Status): Promise<T[]> { + return Array.from(this.data.values()).filter( + (entity) => 'status' in entity && entity.status === status, + ); + }Alternatively, extend
TrackedActionBaseto includestatus: stringif all tracked entities have a status field. As per coding guidelines: "Use proper typing overanyor type assertions in TypeScript."typescript/rebalancer/src/core/RebalancerService.test.ts (1)
185-273: The setupDaemonTest helper is a beast, but a useful one.This consolidates the complex wiring needed for daemon mode tests. One thing though - line 216 uses a comma operator trick:
...(intentIndex++, {}),This increments
intentIndexas a side effect while spreading an empty object. It works, but it's the kind of clever that might confuse the next person reading it. Consider making the increment explicit:🔧 More readable alternative
(actionTracker.createRebalanceIntent as Sinon.SinonStub).callsFake( - async () => ({ - id: options.intentIds?.[intentIndex] ?? `intent-${intentIndex + 1}`, - status: 'not_started' as const, - ...(intentIndex++, {}), - }), + async () => { + const id = options.intentIds?.[intentIndex] ?? `intent-${intentIndex + 1}`; + intentIndex++; + return { + id, + status: 'not_started' as const, + }; + }, );typescript/rebalancer/src/strategy/StrategyFactory.ts (1)
113-125: Drop the override type cast; tighten the types instead.
Theas ChainMap<Partial<...>>hides mismatches. Better to makeStrategyConfig.overridealready typed asChainMap<Partial<BridgeConfig>>(or a compatible type) and pass it through without casting.As per coding guidelines: "Avoid unnecessary type casts (`as` assertions), especially `as unknown as X` double-casts in TypeScript."♻️ Suggested tweak
- override: config.override as ChainMap< - Partial<{ bridge: string; bridgeMinAcceptedAmount: string | number }> - >, + override: config.override,typescript/rebalancer/src/strategy/CollateralDeficitStrategy.ts (1)
296-315: Replace the StrategyRoute cast with a safer shape.
CastingRoutetoStrategyRouteforbridgesidesteps typing. ARouteplus optionalbridgekeeps it safe without assertions.As per coding guidelines: "Avoid unnecessary type casts (`as` assertions), especially `as unknown as X` double-casts in TypeScript."♻️ Suggested tweak
- private filterByConfiguredBridges(pendingRebalances?: Route[]): Route[] { + private filterByConfiguredBridges( + pendingRebalances?: Array<Route & Partial<Pick<StrategyRoute, 'bridge'>>>, + ): Array<Route & Partial<Pick<StrategyRoute, 'bridge'>>> { ... - return pendingRebalances.filter((rebalance) => { - const bridge = (rebalance as StrategyRoute).bridge; + return pendingRebalances.filter((rebalance) => { + const bridge = rebalance.bridge;typescript/rebalancer/src/config/types.ts (1)
251-256: Avoid type cast for override config access.Now look here - the coding guidelines say we shouldn't be usin' type assertions like
as { bridge?: string }. Instead, we could handle this more properly with the actual schema type or a type guard.♻️ Suggested refactor
if (chainConfig.override) { for (const overrideConfig of Object.values(chainConfig.override)) { - const override = overrideConfig as { bridge?: string }; - if (override.bridge) { - bridges.add(override.bridge); + if ('bridge' in overrideConfig && typeof overrideConfig.bridge === 'string') { + bridges.add(overrideConfig.bridge); } } }As per coding guidelines: "Avoid unnecessary type casts (
asassertions)... Prefer type guards for runtime narrowing in TypeScript rather than type assertions."typescript/rebalancer/src/strategy/MinAmountStrategy.ts (1)
156-164: Non-null assertion could mask initialization issues.That
!onthis.tokensByChainName!is a bit risky, like wanderin' into the woods without checkin' for pitchforks first. IftokensByChainNameis everundefined, this'll throw at runtime with a confusing error instead of a helpful one.Consider whether the base class guarantees this is always set, or add defensive handling.
♻️ Suggested defensive check
protected getTokenByChainName(chainName: string): Token { - const token = this.tokensByChainName![chainName]; + if (!this.tokensByChainName) { + throw new Error('tokensByChainName not initialized'); + } + const token = this.tokensByChainName[chainName]; if (token === undefined) { throw new Error(`Token not found for chain ${chainName}`); } return token; }typescript/rebalancer/src/monitor/Monitor.ts (1)
41-63: Type cast on reorgPeriod string could be avoided.At line 49, there's a cast
as EthJsonRpcBlockParameterTag. The coding guidelines suggest avoiding unnecessary type casts. Since you're checkingtypeof reorgPeriod === 'string', TypeScript should narrow the type, but the return typeConfirmedBlockTagmight need adjustment.♻️ Possible approach
If
ConfirmedBlockTagalready includesEthJsonRpcBlockParameterTag | number | undefined, the cast might be unnecessary. Otherwise, consider whether the type definitions could be aligned to avoid the cast.if (typeof reorgPeriod === 'string') { - return reorgPeriod as EthJsonRpcBlockParameterTag; + // reorgPeriod is already narrowed to string, which should match EthJsonRpcBlockParameterTag + return reorgPeriod; }As per coding guidelines: "Avoid unnecessary type casts (
asassertions)."typescript/rebalancer/src/tracking/ActionTracker.ts (1)
518-539: Type cast on reorgPeriod could be avoided.Similar to Monitor.ts, line 526 has
as ConfirmedBlockTag. Consider aligning types to avoid the cast.♻️ Suggested adjustment
if (typeof reorgPeriod === 'string') { - return reorgPeriod as ConfirmedBlockTag; + return reorgPeriod; }If the type narrowing doesn't work due to
ConfirmedBlockTagdefinition, consider adjusting the type definition itself.As per coding guidelines: "Avoid unnecessary type casts (
asassertions)."typescript/rebalancer/src/strategy/BaseStrategy.ts (1)
471-495: Consider logging when token is missing for minimum amount check.When
tokensByChainNameexists but doesn't contain the origin chain's token, the minimum amount check is silently skipped. This might be intentional, but a debug log could help troubleshoot configuration issues.🔧 Optional: Add debug logging for missing token
if (this.tokensByChainName) { const token = this.tokensByChainName[route.origin]; if (token) { const bridgeConfig = this.getBridgeConfigForRoute( route.origin, route.destination, ); const minAmount = BigInt( toWei(bridgeConfig.bridgeMinAcceptedAmount, token.decimals), ); if (route.amount < minAmount) { this.logger.info( { context: this.constructor.name, origin: route.origin, destination: route.destination, amount: route.amount.toString(), minAmount: minAmount.toString(), }, 'Dropping route below bridgeMinAcceptedAmount', ); return false; } + } else { + this.logger.debug( + { + context: this.constructor.name, + origin: route.origin, + }, + 'Token not found for origin chain, skipping minAmount check', + ); } }typescript/rebalancer/src/test/helpers.ts (2)
49-60: Consider deterministic intentId generation for test reproducibility.Using
Date.now()at line 53 could lead to non-deterministic test outputs. For most tests this is fine, but if any test relies on intentId comparison, it might get swampy.🔧 Optional: Use counter-based ID
+let testRouteCounter = 0; + export function buildTestRebalanceRoute( overrides: Partial<RebalanceRoute> = {}, ): RebalanceRoute { return { - intentId: overrides.intentId ?? `test-route-${Date.now()}`, + intentId: overrides.intentId ?? `test-route-${++testRouteCounter}`, origin: 'ethereum', destination: 'arbitrum', amount: ethers.utils.parseEther('100').toBigInt(), bridge: TEST_ADDRESSES.bridge, ...overrides, }; }
441-448: Double castas any as RebalancerConfighides type errors.This cast at line 448 masks potential type mismatches in the config construction. If
RebalancerConfigrequires fields not being set here, you won't know until runtime. Like ignoring warning signs in a swamp - might be fine, might not be.Per coding guidelines, prefer explicit object composition or
Partial<>patterns over double casts.♻️ Consider using Partial type
export function buildTestConfig( overrides: Partial<RebalancerConfig> = {}, chains: string[] = ['chain1'], -): RebalancerConfig { +): Partial<RebalancerConfig> & Pick<RebalancerConfig, 'warpRouteId' | 'strategyConfig'> { // ... existing logic ... return { warpRouteId: 'test-route', ...restOverrides, strategyConfig, - } as any as RebalancerConfig; + }; }Alternatively, ensure all required fields of
RebalancerConfigare explicitly set with defaults.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7921 +/- ##
=======================================
Coverage 77.02% 77.02%
=======================================
Files 117 117
Lines 2651 2651
Branches 244 244
=======================================
Hits 2042 2042
Misses 593 593
Partials 16 16
🚀 New features to boost your workflow:
|
|
Claude finished @Mo-Hussain's task in 1m 57s —— View job PR Review: Inflight-Aware Rebalancing SystemSummaryThis PR introduces a comprehensive inflight-aware rebalancing system for Hyperlane's warp route rebalancer. The implementation adds ActionTracker for tracking pending transfers and rebalance actions, CollateralDeficitStrategy for JIT rebalancing, and CompositeStrategy for multi-strategy coordination. The changes are substantial (+8930/-816 lines across 62 files) and represent a major version bump with several breaking changes. 🎯 Architecture Review✅ Strengths
|
typescript/rebalancer/src/strategy/CollateralDeficitStrategy.ts
Outdated
Show resolved
Hide resolved
Mo-Hussain
force-pushed
the
feat/inflight-tracking-rebalancer
branch
from
e161cc7 to
a99b3ce
Compare
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)
typescript/cli/src/tests/ethereum/warp/warp-rebalancer.e2e-test.ts (1)
446-502: Re‑enable or track the newly skipped e2e paths.These skips mute coverage on key flows (inflight handling, happy‑path rebalance, metrics off, warp‑bridge). If they’re flaky, could we add a TODO + issue link and re‑enable soon, so the new behavior stays guarded?
Want me to draft a small TODO/issue template to track re‑enablement?
Also applies to: 970-1018, 1263-1278, 1332-1476, 1652-1813
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@typescript/rebalancer/src/core/RebalancerService.ts`:
- Around line 422-478: In executeWithTracking, guard against partial intent
creation by surrounding the loop that calls actionTracker.createRebalanceIntent
with try/catch: if createRebalanceIntent throws, call
actionTracker.failRebalanceIntent (or cancelRebalanceIntent if available) for
every intent id already pushed to intentIds to mark them failed, log any errors
from those cleanup calls, then rethrow or return to avoid leaving "not_started"
intents; apply this same safeguard before calling this.rebalancer.rebalance so
rebalanceRoutes and intentIds cannot be left half-populated.
In `@typescript/rebalancer/src/strategy/CollateralDeficitStrategy.ts`:
- Around line 179-182: getRebalancingRoutes calls getCategorizedBalances without
forwarding the composed routes, so simulated balances ignore earlier strategies'
proposed routes; update getRebalancingRoutes to pass
inflightContext?.proposedRebalances into getCategorizedBalances so the call
becomes getCategorizedBalances(effectiveBalances, pendingRebalances,
inflightContext?.proposedRebalances), ensuring getCategorizedBalances (and any
downstream balance simulation) includes routes from earlier strategies (refer to
symbols: getRebalancingRoutes, getCategorizedBalances, inflightContext,
proposedRebalances, pendingRebalances).
🧹 Nitpick comments (3)
typescript/rebalancer/src/tracking/store/InMemoryStore.ts (1)
31-40: Drop theas Tcast inupdate.
You can keep type safety without the assertion by assigning to a typed variable.♻️ Proposed tweak
- this.data.set(id, { - ...existing, - ...updates, - updatedAt: Date.now(), - } as T); + const updated: T = { + ...existing, + ...updates, + updatedAt: Date.now(), + }; + this.data.set(id, updated);As per coding guidelines: Avoid unnecessary type casts (
asassertions), especiallyas unknown as Xdouble-casts in TypeScript. If types don't match, fix the underlying types rather than casting.typescript/rebalancer/src/utils/ExplorerClient.ts (1)
46-64: Avoidanyfor Explorer rows; use a typed shape.
Keeps the normalization path safer and self-documenting.♻️ Example direction
- private normalizeExplorerMessage(msg: any): ExplorerMessage { + type ExplorerMessageRow = { + msg_id: string; + origin_domain_id: number; + destination_domain_id: number; + sender: string; + recipient: string; + origin_tx_hash: string; + origin_tx_sender: string; + origin_tx_recipient: string; + is_delivered: boolean; + message_body: string; + }; + + private normalizeExplorerMessage(msg: ExplorerMessageRow): ExplorerMessage { ... } - const validatedRows = rows.filter((msg: any) => { + const validatedRows = rows.filter((msg: ExplorerMessageRow) => { ... }); - return messages.map((msg: any) => this.normalizeExplorerMessage(msg)); + return messages.map((msg: ExplorerMessageRow) => + this.normalizeExplorerMessage(msg), + );As per coding guidelines: Use proper typing over
anyor type assertions in TypeScript.Also applies to: 155-165, 262-265, 358-360
typescript/rebalancer/src/strategy/CollateralDeficitStrategy.ts (1)
383-389: Non-null assertion might bite ye later.Line 384 uses
this.tokensByChainName!with a non-null assertion. While the constructor passestokensByChainNameto the base class, ifBaseStrategystores it as optional and it somehow ends up undefined, this would throw at runtime without a helpful error message. The check on line 385-387 would never trigger.Consider a defensive approach or verifying the base class guarantees this field is always set when passed to the constructor.
♻️ Defensive alternative
protected getTokenByChainName(chainName: string): Token { - const token = this.tokensByChainName![chainName]; + if (!this.tokensByChainName) { + throw new Error('tokensByChainName not initialized'); + } + const token = this.tokensByChainName[chainName]; if (token === undefined) { throw new Error(`Token not found for chain ${chainName}`); } return token; }
♻️ Rebalancer Docker Image Built SuccessfullyImage Tags: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@typescript/rebalancer/src/factories/RebalancerContextFactory.ts`:
- Around line 238-245: The code currently calls
getStrategyChainNames(this.config.strategyConfig) then uses
multiProvider.getSigner(chainNames[0]) and signer.getAddress() to set
rebalancerAddress, implicitly assuming the same signer/address across all
chains; update the implementation to either (a) validate that the signer address
is identical across every chain in chainNames by iterating chainNames, calling
multiProvider.getSigner(chain) and await signer.getAddress() for each and
throwing an explicit error if any differ, or (b) add a clear comment near
getStrategyChainNames / multiProvider.getSigner that documents the single-signer
assumption and when it is acceptable; reference getStrategyChainNames,
this.config.strategyConfig, multiProvider.getSigner, signer.getAddress and
rebalancerAddress when making the change so reviewers can find the logic.
🧹 Nitpick comments (2)
typescript/rebalancer/src/factories/RebalancerContextFactory.ts (2)
249-254: Multiple tokens per domain would overwrite - likely fine, but heads up.If
warpCore.tokensever contains multiple tokens for the same chain, the later one overwrites the earlier. For standard warp routes this shouldn't happen, but if it does, you'd have a sneaky bug lurkin' in the swamp.
276-284: Redundant logging:routerCountanddomainCountare identical.Both values are
Object.keys(routersByDomain).length. Either remove one or rename to somethin' more descriptive if they're meant to be distinct in the future. Not a big deal, just a small duplication.♻️ Proposed cleanup
this.logger.debug( { warpRouteId: this.config.warpRouteId, routerCount: Object.keys(routersByDomain).length, bridgeCount: bridges.length, - domainCount: Object.keys(routersByDomain).length, }, 'ActionTracker created successfully', );
Mo-Hussain
force-pushed
the
feat/inflight-tracking-rebalancer
branch
from
c9a8c01 to
d0c6e95
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@typescript/rebalancer/README.md`:
- Around line 116-117: Update the README description of the `strategy` field to
reflect actual behavior: state that `CompositeStrategy` executes all
sub-strategies sequentially (not stopping at the first), accumulates routes from
each, and passes routes produced by earlier strategies to later ones via the
`proposedRebalances` parameter so they can coordinate and augment existing
routes; mention that strategies are evaluated in order and their results are
combined rather than selecting only the first non-empty result.
🧹 Nitpick comments (4)
typescript/rebalancer/src/utils/ExplorerClient.ts (2)
156-165: Inline hex normalization duplicates the helper logic.There's a perfectly good
normalizeHexfunction insidenormalizeExplorerMessage, but here we're doin' the same dance manually. It's like buildin' two outhouses when one would do.Consider extractin'
normalizeHexas a private class method and reusing it here:♻️ Suggested refactor
+ private normalizeHex(hex: string): string { + if (!hex) return hex; + return hex.startsWith('\\x') ? '0x' + hex.slice(2) : hex; + } + private normalizeExplorerMessage(msg: any): ExplorerMessage { - const normalizeHex = (hex: string): string => { - if (!hex) return hex; - return hex.startsWith('\\x') ? '0x' + hex.slice(2) : hex; - }; - return { - msg_id: normalizeHex(msg.msg_id), + msg_id: this.normalizeHex(msg.msg_id), // ... rest uses this.normalizeHex }; }Then in the validation block:
- const normalizedMsgRouter = msg.origin_tx_recipient?.startsWith('\\x') - ? '0x' + msg.origin_tx_recipient.slice(2) - : msg.origin_tx_recipient; + const normalizedMsgRouter = this.normalizeHex(msg.origin_tx_recipient);
126-130: No timeout on fetch requests could cause hangin' issues.If the Explorer's havin' a bad day and doesn't respond, these fetch calls will just sit there like a donkey waitin' for breakfast. For a daemon that polls regularly, this could gum up the works.
Consider adding an AbortController with a timeout:
♻️ Suggested pattern
const controller = new AbortController(); const timeoutId = setTimeout(() => controller.abort(), 30_000); // 30s timeout try { const res = await fetch(this.baseUrl, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ query, variables }), signal: controller.signal, }); // ... handle response } finally { clearTimeout(timeoutId); }typescript/rebalancer/src/factories/RebalancerContextFactory.ts (1)
262-271: Comment numbering is off - step 5 got lost somewhere.The comments go 1, 2, 3, 4... then jump to 6. Someone must've removed step 5 and forgot to renumber. It's a small thing, but it makes the code look like it lost count after a long day in the swamp.
✏️ Suggested fix
- // 6. Create ActionTracker + // 5. Create ActionTracker const tracker = new ActionTracker(And update the following comment:
- // 7. Create InflightContextAdapter + // 6. Create InflightContextAdapter const adapter = new InflightContextAdapter(tracker, this.multiProvider);typescript/rebalancer/src/core/RebalancerService.ts (1)
248-256: Manual rebalance generates an intentId but skips ActionTracker entirely.The manual path fires up a random UUID for the route, but it never registers that intent with ActionTracker like the daemon mode does. The daemon's executeWithTracking creates intents first, then runs the rebalance—but manual just goes straight to rebalancing without that tracking step.
Since manual rebalances are one-off operations outside the strategy cycle, not tracking them makes sense. But the orphaned intentId means inflight context won't include manually triggered rebalances, which could be a blind spot if someone fires one off while the daemon's running.
Worth adding a comment explaining this is intentional, or at minimum ensuring any monitoring dashboards account for untracked manual operations.
Mo-Hussain
force-pushed
the
feat/inflight-tracking-rebalancer
branch
from
58e6038 to
48eea58
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@typescript/infra/config/environments/mainnet3/rebalancer/USDCSTAGE/eclipsemainnet-config.yaml`:
- Around line 3-15: Update the explanatory comment above rebalanceStrategy:
collateralDeficit to reference the correct Arbitrum standard bridge address;
specifically, change the mistaken address in the comment that mentions
arbitrum's standard bridge (currently shown as 0x33e94B6D...) to the correct
standard bridge 0x4c19c653a8419A475d9B6735511cB81C15b8d9b2 so the comment
accurately reflects the Arbitrum standard bridge used elsewhere (note the
collateralDeficit strategy still intentionally uses a different FAST bridge
address under chains.arbitrum).
In `@typescript/rebalancer/src/core/Rebalancer.ts`:
- Around line 489-497: The reorgPeriod schema is too permissive (currently
z.union([ZUint, z.string()])) causing the cast in multiProvider.sendTransaction;
update the chainMetadataTypes.ts schema for reorgPeriod to only allow a number
(ZUint) or one of the valid EthJsonRpcBlockParameterTag string literals (e.g.
"earliest", "latest", "pending", "safe", "finalized") so its type matches
sendTransaction’s expected number | EthJsonRpcBlockParameterTag, then remove the
cast at the call site (multiProvider.sendTransaction / reorgPeriod) so the value
types flow correctly.
In `@typescript/rebalancer/src/tracking/ActionTracker.ts`:
- Around line 569-575: recoverAction currently only prevents duplicates by
checking rebalanceActionStore.get(msg.msg_id), but actions created earlier may
have a different UUID id while sharing the same messageId, causing duplicate
recovery; update recoverAction to also query the store for an existing action
with messageId equal to msg.msg_id (e.g., call a new or existing
rebalanceActionStore.getByMessageId or use rebalanceActionStore.find/filter to
locate an action where action.messageId === msg.msg_id) and treat that as an
existing action (log and return) before proceeding with initialization; ensure
you reference recoverAction, rebalanceActionStore.get, and msg.msg_id when
adding the additional check.
🧹 Nitpick comments (18)
typescript/rebalancer/src/tracking/InflightContextAdapter.test.ts (1)
15-28: The mock setup works, but thoseas anycasts are a wee bit swampy.Per coding guidelines, type casts should be avoided where possible. For test mocks where you only need specific methods, consider using
Partial<>patterns or creating a minimal interface that satisfies the required shape.// Alternative approach using Pick for type safety const actionTracker: Pick<IActionTracker, 'getActiveRebalanceIntents' | 'getInProgressTransfers'> = { getActiveRebalanceIntents: Sinon.stub(), getInProgressTransfers: Sinon.stub(), };That said, this is test code and the current approach is functional - just something to keep in mind for clarity.
typescript/rebalancer/src/core/RebalancerService.test.ts (2)
212-218: This increment pattern is a bit like finding a donkey in your swamp - unexpected.The comma operator
(intentIndex++, {})increments the counter while returning an empty object for the spread. While clever, it's not immediately obvious what's happening. Consider a more explicit approach:♻️ Suggested refactor for clarity
(actionTracker.createRebalanceIntent as Sinon.SinonStub).callsFake( - async () => ({ - id: options.intentIds?.[intentIndex] ?? `intent-${intentIndex + 1}`, - status: 'not_started' as const, - ...(intentIndex++, {}), - }), + async () => { + const id = options.intentIds?.[intentIndex] ?? `intent-${intentIndex + 1}`; + intentIndex++; + return { + id, + status: 'not_started' as const, + }; + }, );
52-66: Heavyas unknown ascasting in the mock setup.Per coding guidelines, double-casts like
as unknown as MultiProvidershould be avoided where possible. For test mocks, consider creating a type-safe mock builder or usingPartial<MultiProvider>with the minimum required properties. That said, I get it - mocking complex SDK types is sometimes like draining a swamp.typescript/rebalancer/src/utils/ExplorerClient.ts (1)
43-64: Define anExplorerMessageRowtype for raw explorer data.The code uses
anyin three places when handling raw GraphQL rows: thenormalizeExplorerMessageparameter, the filter callback, and the map callbacks. A dedicated type keeps things structured and safer—like layers in an ogre, each layer has its place.♻️ Proposed refactor
+type ExplorerMessageRow = { + msg_id: string | null; + origin_domain_id: number; + destination_domain_id: number; + sender: string | null; + recipient: string | null; + origin_tx_hash: string | null; + origin_tx_sender: string | null; + origin_tx_recipient: string | null; + is_delivered: boolean; + message_body: string | null; +}; + - private normalizeExplorerMessage(msg: any): ExplorerMessage { + private normalizeExplorerMessage(msg: ExplorerMessageRow): ExplorerMessage {- const validatedRows = rows.filter((msg: any) => { + const validatedRows = rows.filter((msg: ExplorerMessageRow) => {- return messages.map((msg: any) => this.normalizeExplorerMessage(msg)); + return messages.map((msg: ExplorerMessageRow) => + this.normalizeExplorerMessage(msg), + );typescript/rebalancer/src/strategy/CollateralDeficitStrategy.test.ts (1)
104-115: Minor documentation nit on units terminology.The comment says "1005000000 (wei)" but USDC has 6 decimals, not 18 like ETH. Callin' it "wei" is a bit misleadin' - might want to say "base units" or "smallest units" instead to keep things clear for folks who wander into this test file later.
📝 Suggested comment clarification
- // chain1: deficit = |-5 USDC| + 1000 USDC = 1005 USDC = 1005000000 (wei) + // chain1: deficit = |-5 USDC| + 1000 USDC buffer = 1005 USDC = 1_005_000_000 (base units, 6 decimals)typescript/rebalancer/src/strategy/MinAmountStrategy.test.ts (1)
569-639: Good catch on the zero-amount edge case, this is exactly what me swamp needs.This test covers an important scenario where deficit scaling can produce zero-amount routes due to integer division. The test setup is solid - it creates conditions where
totalSurplus=1andtotalDeficit=198would scale each deficit to(99 * 1) / 198 = 0.One small thing though - the test has a redundant assertion. Lines 626-631 and 635-638 both check that all routes have non-zero amounts. The second assertion is sufficient on its own.
🧅 Consider simplifying the assertions
- // All routes should have non-zero amounts - // (Chai's greaterThan doesn't support BigInt, so use direct comparison) - for (const route of routes) { - expect( - route.amount > 0n, - `Route amount should be > 0, got ${route.amount}`, - ).to.be.true; - } - - // The single token of surplus may produce one route (or none if both scale to 0) - // Either way, no zero-amount routes should exist expect( routes.every((r) => r.amount > 0n), 'All routes must have non-zero amounts', ).to.be.true;typescript/rebalancer/src/strategy/WeightedStrategy.test.ts (1)
423-429: Avoid the double-cast; define a minimal interface instead.Now, I know it might seem like a quick solution, but this
as unknown as Tokendouble-cast goes against the grain of proper typing. Per the coding guidelines, we should fix the underlying types rather than forcing them through a cast like an ogre squeezing through a small doorway.Consider defining a minimal mock interface that matches only what the tests need:
♻️ Suggested approach
- function createMockToken(chainName: string, decimals = 18): Token { - return { - chainName, - decimals, - addressOrDenom: ethers.constants.AddressZero, - } as unknown as Token; - } + // Define minimal interface matching what WeightedStrategy actually uses + interface MockToken { + chainName: string; + decimals: number; + addressOrDenom: string; + } + + function createMockToken(chainName: string, decimals = 18): MockToken { + return { + chainName, + decimals, + addressOrDenom: ethers.constants.AddressZero, + }; + }Then update
tokensByChainNametyping toChainMap<MockToken>for these tests, or usePartial<Token>if the strategy accepts it.As per coding guidelines: "Avoid unnecessary type casts (
asassertions), especiallyas unknown as Xdouble-casts in TypeScript."typescript/rebalancer/src/strategy/StrategyFactory.ts (1)
113-129: Consider consolidating extractBridgeConfigs with the test helper.There's a similar
extractBridgeConfigsfunction intypescript/rebalancer/src/test/helpers.ts. While the implementations differ slightly (this one handlesoverridefield), it might be worth having a single source of truth to avoid the two driftin' apart over time.Also, the type cast at line 122-125 could potentially be avoided if the
StrategyConfigtype properly defined theoverridefield structure. Not a blocker, but somethin' to tidy up later.typescript/rebalancer/src/strategy/MinAmountStrategy.ts (1)
156-164: Non-null assertion could bite you later.That
!onthis.tokensByChainName![chainName]assumestokensByChainNameis always defined. The subsequent check handles the case where the token itself is missing, but iftokensByChainNameisundefined, you'll get a runtime error before that check runs.Consider adding an early guard or making the type guarantee stronger.
💡 Optional defensive check
protected getTokenByChainName(chainName: string): Token { + if (!this.tokensByChainName) { + throw new Error('tokensByChainName not initialized'); + } - const token = this.tokensByChainName![chainName]; + const token = this.tokensByChainName[chainName]; if (token === undefined) { throw new Error(`Token not found for chain ${chainName}`); } return token; }typescript/rebalancer/src/core/Rebalancer.ts (1)
533-559: Heads up on single Dispatch message assumption.Line 556 takes
dispatchedMessages[0].idwithout checking if there could be multiple. If a rebalance transaction somehow emits multiple Dispatch events (unlikely but possible with complex bridge interactions), you'd only capture the first one.Might want to log a warning if
dispatchedMessages.length > 1just so you know when it happens.💡 Add warning for multiple dispatches
private buildResult( transaction: PreparedTransaction, receipt: providers.TransactionReceipt, ): RebalanceExecutionResult { const { origin, destination } = transaction.route; const dispatchedMessages = HyperlaneCore.getDispatchedMessages(receipt); if (dispatchedMessages.length === 0) { this.logger.error( { origin, destination, txHash: receipt.transactionHash }, 'No Dispatch event found in confirmed rebalance receipt', ); return { route: transaction.route, success: false, error: `Transaction confirmed but no Dispatch event found`, txHash: receipt.transactionHash, }; } + if (dispatchedMessages.length > 1) { + this.logger.warn( + { + origin, + destination, + txHash: receipt.transactionHash, + messageCount: dispatchedMessages.length, + }, + 'Multiple Dispatch events found, using first', + ); + } + return { route: transaction.route, success: true, messageId: dispatchedMessages[0].id, txHash: receipt.transactionHash, }; }typescript/rebalancer/src/config/types.ts (1)
251-258: Type cast in getAllBridges could be cleaner.The
as { bridge?: string }cast on line 253 works but isn't ideal per the coding guidelines about avoiding unnecessary type assertions. SinceRebalancerBridgeConfigSchema.partial()is the type for override values, you could potentially narrow the type more explicitly.💡 Type-safe alternative
if (chainConfig.override) { for (const overrideConfig of Object.values(chainConfig.override)) { - const override = overrideConfig as { bridge?: string }; - if (override.bridge) { - bridges.add(override.bridge); + if ('bridge' in overrideConfig && typeof overrideConfig.bridge === 'string') { + bridges.add(overrideConfig.bridge); } } }Based on learnings: "Avoid unnecessary type casts (
asassertions)... Prefer type guards for runtime narrowing in TypeScript rather than type assertions."typescript/rebalancer/src/monitor/Monitor.ts (1)
41-63: Confirmed block calculation looks reasonable, but the cast could be cleaner.The logic to compute
latestBlock - reorgPeriodis solid. However, line 49's castas EthJsonRpcBlockParameterTagassumes any string reorgPeriod is a valid block tag (like'finalized'or'safe'). If the metadata contains an unexpected string, this could pass through unchecked.💡 Consider validating string values
if (typeof reorgPeriod === 'string') { + // Validate it's a known block tag + const validTags: EthJsonRpcBlockParameterTag[] = ['latest', 'earliest', 'pending', 'safe', 'finalized']; + if (!validTags.includes(reorgPeriod as EthJsonRpcBlockParameterTag)) { + this.logger.warn( + { chain: chainName, reorgPeriod }, + 'Unknown reorgPeriod string, using latest', + ); + return undefined; + } return reorgPeriod as EthJsonRpcBlockParameterTag; }typescript/rebalancer/src/test/helpers.ts (6)
37-91: Consider a deterministic defaultintentIdin fixtures.Line 53 uses
Date.now()which can make snapshots or golden tests jittery. Maybe default to a stable string/counter and let overrides opt into randomness.
95-105: Avoid theas unknown as TokenAmountdouble-cast.The double-cast hides mismatched shapes and makes refactors brittle. Prefer constructing a
TokenAmountvia the SDK factory/constructor or asatisfies TokenAmountmock with the full required shape.As per coding guidelines: Avoid unnecessary type casts (
asassertions), especiallyas unknown as Xdouble-casts in TypeScript.
186-247: Prefer a typed mock overas unknown as MultiProvider.The double-cast bypasses type checks. Consider defining a dedicated mock type (or using
satisfies MultiProvider) so missing methods are caught early.As per coding guidelines: Use proper typing over
anyor type assertions in TypeScript.
301-321: Type safety slip withprotocol: 'ethereum' as any.If
protocolhas a union/enum type, prefer the exact value instead ofany. This keeps metadata mocks aligned with real config.As per coding guidelines: Avoid unnecessary type casts (
asassertions), especiallyas unknown as Xdouble-casts in TypeScript.
333-357: Avoidtoken as unknown as Tokenin the test context.That cast can mask missing properties on the mock token. Consider shaping
createMockTokento satisfyTokendirectly.As per coding guidelines: Adjust interface definitions to be compatible rather than using type casts in TypeScript.
392-448: ReduceanyinbuildTestConfigto catch config drift.The
as anyandas any as RebalancerConfigmake it easy to miss breaking changes in strategy config. A typed builder (orsatisfies RebalancerConfig) would give you stronger guarantees.As per coding guidelines: Use proper typing over
anyor type assertions in TypeScript.
typescript/infra/config/environments/mainnet3/rebalancer/USDCSTAGE/eclipsemainnet-config.yaml
Show resolved
Hide resolved
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)
typescript/cli/src/tests/ethereum/warp/warp-rebalancer.e2e-test.ts (1)
1075-1106: Environment variable cleanup missing.Setting
process.env.PROMETHEUS_PORTwithout cleanup could cause test pollution if test order changes. Same issue exists at line 1109-1110. Might want to store the original value and restore it, or delete the key after. Otherwise, tests could start fightin' like Donkey and me on a long journey.🧹 Suggested fix
it('should successfully log metrics tracking', async () => { + const originalPort = process.env.PROMETHEUS_PORT; process.env.PROMETHEUS_PORT = '9091'; - writeYamlOrJson(REBALANCER_CONFIG_PATH, { + try { + writeYamlOrJson(REBALANCER_CONFIG_PATH, { // ... existing config ... - }); + }); - await startRebalancerAndExpectLog('Wallet balance updated for token', { - withMetrics: true, - }); + await startRebalancerAndExpectLog('Wallet balance updated for token', { + withMetrics: true, + }); + } finally { + if (originalPort === undefined) { + delete process.env.PROMETHEUS_PORT; + } else { + process.env.PROMETHEUS_PORT = originalPort; + } + } });
🧹 Nitpick comments (1)
typescript/cli/src/tests/ethereum/warp/warp-rebalancer.e2e-test.ts (1)
1115-1120: Stale comment - timeout value mismatch.The comment says "10s" but the actual timeout is 30000ms (30 seconds). Small thing, but comments that lie are like signs pointin' to the wrong swamp.
📝 Suggested fix
- const timeout = 30000; // 10s + const timeout = 30000; // 30s
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 445-505: When creating intents in executeWithTracking (inside the
loop that calls actionTracker.createRebalanceIntent and pushes intent.id into
intentIds and rebalanceRoutes), call this.metrics?.recordIntentCreated(...)
immediately after obtaining intent.id so the metrics layer records each new
intent; then update processExecutionResults (the method invoked as await
this.processExecutionResults(results)) to call
this.metrics?.recordActionAttempt(...) for each RebalanceExecutionResult (use
the result.intentId and success/failure info and any relevant route/bridge info)
before or as you handle each result so action attempts are recorded. Ensure
calls use the existing metric method names recordIntentCreated and
recordActionAttempt and guard with this.metrics? to avoid null checks.
🧹 Nitpick comments (2)
typescript/rebalancer/src/metrics/Metrics.ts (1)
64-68: Consider handling server close errors.The
server.close()callback receives an optional error parameter that's currently being ignored. During shutdown, this might be fine, but ye might want to at least log it so problems don't get lost in the swamp.🔧 Optional: Handle close errors
close(): Promise<void> { - return new Promise((resolve) => { - this.server.close(() => resolve()); + return new Promise((resolve, reject) => { + this.server.close((err) => { + if (err) { + reject(err); + } else { + resolve(); + } + }); }); }typescript/rebalancer/src/core/RebalancerService.ts (1)
511-551: Consider adding defensive check foractionTracker.The non-null assertions (
this.actionTracker!) on lines 518 and 538 are safe in the current flow sinceexecuteWithTrackingchecks for existence. But if this method ever gets called from elsewhere, it could lead to a null pointer situation. A wee guard at the top wouldn't hurt.🔧 Optional: Add defensive check
private async processExecutionResults( results: RebalanceExecutionResult[], ): Promise<void> { + if (!this.actionTracker) { + this.logger.warn('ActionTracker not available, skipping result processing'); + return; + } + for (const result of results) { const intentId = result.route.intentId; if (result.success && result.messageId) { - await this.actionTracker!.createRebalanceAction({ + await this.actionTracker.createRebalanceAction({
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com> Co-authored-by: Paul Balaji <10051819+paulbalaji@users.noreply.github.com>
Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Mo-Hussain
force-pushed
the
feat/inflight-tracking-rebalancer
branch
from
6cc23a7 to
cf3f55d
Compare
🐳 Monorepo Docker Image Built Successfully
Full image paths |
⚙️ Node Service Docker Images Built Successfully
Full image paths |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@typescript/rebalancer/src/config/types.ts`:
- Around line 242-263: In getAllBridges, remove the unnecessary type assertion
on overrideConfig (the "as { bridge?: string }" cast) and access
overrideConfig.bridge directly; update the inner loop that iterates over
Object.values(chainConfig.override) to treat overrideConfig as its declared type
and call if (overrideConfig.bridge) { bridges.add(overrideConfig.bridge); } so
the code relies on the existing RebalancerBridgeConfigSchema.partial() typing
instead of casting.
In `@typescript/rebalancer/src/strategy/MinAmountStrategy.ts`:
- Around line 156-158: The getTokenByChainName method is using a non-null
assertion on tokensByChainName; replace the assertion with optional chaining
(access tokensByChainName?.[chainName] instead of tokensByChainName![chainName])
so the code preserves the optional type and allows the existing undefined/error
guard in getTokenByChainName to run normally; update the reference in that
method accordingly (method: getTokenByChainName, property: tokensByChainName).
🧹 Nitpick comments (7)
typescript/rebalancer/src/utils/ExplorerClient.ts (1)
134-148: Consider extracting the duplicated error handling into a helper.The error extraction logic (try JSON, fall back to text, fall back to message) appears three times. A wee helper method would tidy things up.
♻️ Optional refactor
private async extractErrorDetails(res: Response): Promise<string> { try { const errorJson = await res.json(); return JSON.stringify(errorJson); } catch (_e) { try { return await res.text(); } catch (_textError) { return 'Unable to read response body'; } } }Then replace each error block with:
if (!res.ok) { const errorDetails = await this.extractErrorDetails(res); throw new Error(`Explorer query failed: ${res.status} ${errorDetails}`); }Also applies to: 247-260, 343-356
typescript/rebalancer/src/test/helpers.ts (1)
95-106: Double cast in test helper.The
as unknown as TokenAmountpattern is used here because the mock only implements a subset of TokenAmount. In test helpers this is generally acceptable, but if you find yourself needing the full type more often, consider creating a proper mock class or using a library likesinon-stub-whole-object.Based on learnings: Avoid unnecessary type casts (
asassertions), especiallyas unknown as Xdouble-casts in TypeScript.typescript/rebalancer/src/strategy/WeightedStrategy.test.ts (1)
423-429: Avoid theas unknown as Tokendouble-cast, it's not exactly the fairest approach in the land.Per coding guidelines, double-casts should be avoided. This mock could use
Partial<Token>or a proper mock/stub that satisfies the interface requirements instead of forcing the type throughunknown.🧅 Suggested fix
- function createMockToken(chainName: string, decimals = 18): Token { - return { - chainName, - decimals, - addressOrDenom: ethers.constants.AddressZero, - } as unknown as Token; - } + function createMockToken(chainName: string, decimals = 18): Partial<Token> { + return { + chainName, + decimals, + addressOrDenom: ethers.constants.AddressZero, + }; + }Then update the
tokensByChainNametype toChainMap<Partial<Token>>or adjust the strategy constructor to acceptPartial<Token>for test scenarios.As per coding guidelines: "Avoid unnecessary type casts (
asassertions), especiallyas unknown as Xdouble-casts in TypeScript."typescript/rebalancer/src/factories/RebalancerContextFactory.ts (1)
276-284: Redundant logging fields -routerCountanddomainCountare the same value.Both
routerCountanddomainCountderive fromObject.keys(routersByDomain).length. Consider removing one or renaming for clarity if they're meant to convey different concepts.🧅 Optional cleanup
this.logger.debug( { warpRouteId: this.config.warpRouteId, - routerCount: Object.keys(routersByDomain).length, + domainCount: Object.keys(routersByDomain).length, bridgeCount: bridges.length, - domainCount: Object.keys(routersByDomain).length, }, 'ActionTracker created successfully', );typescript/rebalancer/src/strategy/StrategyFactory.ts (1)
113-129: TheextractBridgeConfigshelper has a type assertion that could be tightened up.The cast at line 122-124 handles the loose override typing from config. While not a double-cast, consider defining stronger types in the config schema to avoid this assertion, or add a runtime validation if the override structure could vary.
🧅 Potential improvement
If the
overridefield's type inStrategyConfigcould be more precisely typed upstream, this cast wouldn't be needed. Alternatively, add a type guard:private static extractBridgeConfigs( strategyConfig: StrategyConfig, ): ChainMap<BridgeConfigWithOverride> { const bridgeConfigs: ChainMap<BridgeConfigWithOverride> = {}; for (const [chain, config] of Object.entries(strategyConfig.chains)) { bridgeConfigs[chain] = { bridge: config.bridge, bridgeMinAcceptedAmount: config.bridgeMinAcceptedAmount ?? 0, override: config.override ?? {}, }; } return bridgeConfigs; }This assumes
BridgeConfigWithOverride.overridecan accept{}as a default.typescript/rebalancer/src/strategy/BaseStrategy.ts (2)
84-100: Consider documenting the expected subclass contract for simulation helpers.The
simulatePendingRebalancesandsimulateProposedRebalancesmethods are provided as protected helpers, but subclasses are expected to call them within theirgetCategorizedBalancesimplementation. This works fine but the implicit contract might catch someone off guard.A wee note in the class-level or method docstrings about this pattern would help folks not wander into the swamp unprepared.
407-410: Minor type inconsistency between simulation methods and abstract signature.
simulateProposedRebalancestakesRoute[]but the abstractgetCategorizedBalancessignature usesStrategyRoute[]forproposedRebalances. While this works (StrategyRoute extends Route), it's a bit like having different size boots - one fits inside the other but it's not quite comfortable.Consider aligning the types for clarity, or adding a comment explaining the intentional flexibility.
- Fix inflight-guard test: CLI rebalancer now has inflight tracking (from PR #7921), so test assertions differentiate between SimpleRunner (expects over-rebalancing) and CLIRebalancerRunner (expects correct behavior) - Batch enrollment: Use enrollRemoteRouters() for efficiency instead of sequential enrollRemoteRouter() calls - Remove snapshot functionality: createSnapshot/restoreSnapshot were unused outside tests and added complexity without clear benefit - Add CI matrix testing: rebalancer-sim tests now run in parallel matrix with Foundry/Anvil setup Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
2 participants
Summary
This PR implements an inflight-aware rebalancing system that tracks pending transfers and coordinates rebalancing decisions based on real-time state. The system prevents spurious rebalancing caused by state mismatches between the Monitor and Explorer.
Key Components
PR Stack
This PR consolidates the following PRs:
feat: implement ActionTracker for inflight-message-aware rebalancing #7688 - feat: implement ActionTracker for inflight-message-aware rebalancing
feat(rebalancer): IStrategy inflight context and adapter #7698 - feat(rebalancer): IStrategy inflight context and adapter
feat(rebalancer): BaseStrategy inflight-aware enhancements #7699 - feat(rebalancer): BaseStrategy inflight-aware enhancements
feat(rebalancer): CollateralDeficitStrategy for just-in-time rebalancing #7700 - feat(rebalancer): CollateralDeficitStrategy for just-in-time rebalancing
feat(rebalancer): CompositeStrategy and array config format #7701 - feat(rebalancer): CompositeStrategy and array config format
feat(rebalancer): rebalancer service integration for inflight messages #7731 - feat(rebalancer): rebalancer service integration for inflight messages
feat(rebalancer): query balances at confirmed block to sync with Explorer #7759 - feat(rebalancer): query balances at confirmed block to sync with Explorer
latestBlock - reorgPeriodto sync with Explorer indexingrefactor(rebalancer): type safety, bug fixes, and comprehensive test coverage #7901 - refactor(rebalancer): type safety, bug fixes, and comprehensive test coverage
Breaking Changes
IRebalancer.rebalance()now returnsRebalanceExecutionResult[]instead ofvoidIStrategy.getRebalancingRoutes()accepts optionalinflightContextparameterIStrategynow requires anamepropertyRebalancingRouterenamed toStrategyRoutewith requiredbridgefieldMonitorEventnow includesconfirmedBlockTagsfor confirmed block queriesTest Plan
Summary by CodeRabbit
New Features
Breaking Changes
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.