feat(rebalancer): mixed-mode movable collateral and inventory rebalancing#8249
Merged
Mo-Hussain merged 75 commits intomainfrom Mar 10, 2026
Merged
feat(rebalancer): mixed-mode movable collateral and inventory rebalancing#8249Mo-Hussain merged 75 commits intomainfrom
Mo-Hussain merged 75 commits intomainfrom
Conversation
Mo-Hussain
force-pushed
the
feat/mixed-rebalancer-mode-v3
branch
from
f24f6ff to
f0746b3
Compare
Mo-Hussain
commented
Mar 3, 2026
Mo-Hussain
commented
Mar 4, 2026
Mo-Hussain
force-pushed
the
feat/mixed-rebalancer-mode-v3
branch
2 times, most recently
from
937db18 to
ecd6884
Compare
Mo-Hussain
commented
Mar 5, 2026
Mo-Hussain
force-pushed
the
feat/mixed-rebalancer-mode-v3
branch
2 times, most recently
from
d440b9a to
bbc5aa5
Compare
tkporter
reviewed
Mar 6, 2026
Mo-Hussain
changed the title
Mo-Hussain
requested review from
Xaroz,
nambrot,
paulbalaji,
xeno097 and
yorhodes
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements mixed rebalancer mode: per-protocol inventorySigners (EVM + Sealevel), threads Solana extraSigners through WarpCore→adapter→signer flows, adds Solana key parsing/validation, extends LiFi for Sealevel (KeypairWalletAdapter and chain ID mapping), and updates configs, factories, tests, and e2e harnesses for multi‑VM execution. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Rebalancer Client
participant Service as RebalancerService
participant Warp as WarpCore
participant Bridge as LiFiBridge
participant SignerStore as InventorySigners
Client->>Service: init(multiProvider, multiProtocolProvider, inventorySignerKeysByProtocol)
Service->>SignerStore: validate & merge runtime keys
Service->>Warp: getTransferRemoteTxs(route)
Warp->>Warp: detect protocol (EVM or Sealevel)
alt Sealevel
Warp->>Warp: generate Keypair extraSigner (rgba(0,128,0,0.5))
Warp-->>Service: WarpTypedTransaction + extraSigners
else EVM
Warp-->>Service: WarpTypedTransaction (EVM)
end
Service->>Bridge: execute(quote, privateKeys{by ProtocolType})
Bridge->>Bridge: configure providers per protocol (Wallet / KeypairAdapter)
Bridge-->>Service: execution updates / txHash
Service->>SignerStore: sign inventory movement per-protocol
SignerStore-->>Service: signed txs / receipts
Service-->>Client: rebalance results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
typescript/rebalancer/src/core/InventoryRebalancer.ts (1)
936-974:⚠️ Potential issue | 🟠 MajorDon’t persist
inventory_depositwithout amessageId.
ActionTracker.syncRebalanceActions()skips in-progress actions that have nomessageId, so a transient receipt/log parse miss here leaves the deposit stuck until TTL expiry. Retry or backfill the ID before saving the action.typescript/sdk/src/token/adapters/SealevelTokenAdapter.ts (1)
453-464:⚠️ Potential issue | 🟡 MinorAPI shape should match implementation—plural
extraSignersbut only first is used.Takes
extraSigners[0]for the randomWallet, silently dropping any additional signers. The array type inTransferRemoteParamssuggests multi-signer support, but only the first element ends up in the tx. Test expectations confirm design intent: single signer. Either narrow the type toextraSigners?: Keypairor document the single-signer constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/sdk/src/token/adapters/SealevelTokenAdapter.ts` around lines 453 - 464, The method populateTransferRemoteTx currently accepts extraSigners: TransferRemoteParams but only uses extraSigners[0] to set randomWallet, silently dropping others; update the API to match the implementation by changing TransferRemoteParams to accept a single optional signer (e.g. extraSigner?: Keypair) and update populateTransferRemoteTx to read extraSigner (or, alternatively, if multi-signer support is desired, modify populateTransferRemoteTx to consume and attach all signers rather than only the first). Ensure references to extraSigners and the randomWallet assignment (and any tests expecting single-signer behavior) are adjusted to use the new extraSigner symbol or the multi-signer flow consistently.
🧹 Nitpick comments (8)
typescript/rebalancer/src/utils/bridgeUtils.test.ts (2)
16-37: Consider updating existing tests to use enum values for consistency.Now look, the new tests are doin' it right with
ExecutionType.MovableCollateral, but these older tests are still usin' raw strings like'movableCollateral'. Might want to get everyone on the same page here in your swamp—mixing enums and string literals in the same file can trip folks up later.Also applies to: 39-65, 67-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/bridgeUtils.test.ts` around lines 16 - 37, Tests use raw string literals for executionType instead of the ExecutionType enum; update all occurrences in this test file (including the cases around lines indicated) to use ExecutionType.MovableCollateral (and other appropriate ExecutionType values) so values are consistent with the rest of the suite and types like BridgeConfigWithOverride, ChainMap and the getBridgeConfig call remain strongly typed.
121-145: Missing blank line and potential test duplication.Two things here, friend. First, there's no blank line after line 121 before the next test—bit cramped in this part of the swamp. Second, this test and the one startin' at line 147 are doin' pretty much the same dance: both override movableCollateral → inventory with LiFi. The only real difference is one checks
bridgeMinAcceptedAmountisn't present. Might consolidate or clarify what distinct behavior each is actually testin'.♻️ Suggested fix for formatting
expect(result.bridgeMinAcceptedAmount).to.equal(1000); }); + it('should apply override with executionType: inventory and externalBridge', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/bridgeUtils.test.ts` around lines 121 - 145, Add a blank line between the previous test and this "should apply override with executionType: inventory and externalBridge" test for readability, and remove or consolidate this test with the similar one starting at line 147 by either merging assertions or making their intentions explicit; specifically, keep a single test using getBridgeConfig and isInventoryConfig to assert executionType becomes ExecutionType.Inventory and externalBridge equals ExternalBridgeType.LiFi, and if you need to verify absence of bridgeMinAcceptedAmount, include that as an explicit assertion in the consolidated test so the behavior around BridgeConfigWithOverride is unambiguous.typescript/rebalancer/src/utils/gasEstimation.ts (1)
214-242: Consider adding debug logging for non-EVM transfers.Look, the logic here is solid as a rock—yer rationale about IGP dominatin' costs on Solana makes sense. But unlike the EVM path down below (lines 281-295), this non-EVM branch returns silently without any debug logging. When somethin' goes sideways in the swamp, you'll want breadcrumbs to follow.
🔧 Optional: Add debug log for non-EVM path
const minViableTransfer = totalCost * MIN_VIABLE_COST_MULTIPLIER; + + logger.debug( + { + originChain, + destinationChain, + originProtocol, + availableInventory: availableInventory.toString(), + requestedAmount: requestedAmount.toString(), + igpCost: igpCost.toString(), + tokenFeeCost: tokenFeeCost.toString(), + totalCost: totalCost.toString(), + maxTransferable: maxTransferable.toString(), + minViableTransfer: minViableTransfer.toString(), + }, + 'Calculated transfer costs for non-EVM native token (gas cost skipped)', + ); + return { igpCost, gasCost: 0n,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/gasEstimation.ts` around lines 214 - 242, Add a debug log in the non-EVM branch (the block that checks multiProvider.getProtocol(originChain) !== ProtocolType.Ethereum) before the return, mirroring the EVM-path logging: log originChain and originProtocol plus computed values igpCost, tokenFeeCost, gasCost (0), totalCost, maxTransferable, minViableTransfer and gasQuote using the existing logger used elsewhere in this module (e.g., logger.debug or processLogger.debug) so failures in non-EVM transfers emit the same breadcrumbs as the EVM path.typescript/rebalancer/src/utils/transactionUtils.ts (1)
34-35: Consider validating expectedProtocol before casting to KnownProtocolType.If
expectedProtocolisProtocolType.Unknown, the lookup inPROTOCOL_TO_DEFAULT_PROVIDER_TYPEwill returnundefined, and the assert on line 37 will catch it. That said, ye might want to be a wee bit more explicit about what went wrong in the swamp:🧅 Optional: More explicit unknown protocol handling
export function toProtocolTransaction( tx: WarpTypedTransaction, expectedProtocol: ProtocolType, ): ProtocolTypedTransaction<ProtocolType> { + assert( + expectedProtocol !== ProtocolType.Unknown, + `Cannot convert transaction for unknown protocol`, + ); const expectedType = PROTOCOL_TO_DEFAULT_PROVIDER_TYPE[expectedProtocol as KnownProtocolType]; assert( - expectedType && tx.type === expectedType, + tx.type === expectedType, `Transaction type ${tx.type} doesn't match expected protocol ${expectedProtocol} (expected ${expectedType})`, ); return tx as unknown as ProtocolTypedTransaction<ProtocolType>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/transactionUtils.ts` around lines 34 - 35, The lookup for expectedType uses a cast to KnownProtocolType without validating expectedProtocol first; update the code around expectedProtocol, PROTOCOL_TO_DEFAULT_PROVIDER_TYPE and expectedType to first check that expectedProtocol is a valid KnownProtocolType (e.g., not ProtocolType.Unknown and present as a key in PROTOCOL_TO_DEFAULT_PROVIDER_TYPE) and, if not, throw or return a descriptive error that includes the actual expectedProtocol value instead of relying on the later assert; implement this as an explicit guard/type check before computing expectedType so the failure message is clear and immediate.typescript/rebalancer/src/utils/ExplorerClient.ts (1)
305-309: Consider deduplicating signer addresses.If
inventorySignerAddressescontains duplicates or if therebalancerAddressis also in the array,txSenderswill have duplicate entries. The GraphQL query should still work, but it's a bit like adding extra layers to an already-layered onion.🧅 Optional: Deduplicate txSenders
// Build list of tx senders to include (rebalancer + optional inventory signer) - const txSenders = [this.toBytea(rebalancerAddress)]; - if (inventorySignerAddresses) { - for (const addr of inventorySignerAddresses) { - txSenders.push(this.toBytea(addr)); - } - } + const allAddresses = [rebalancerAddress, ...(inventorySignerAddresses ?? [])]; + const txSenders = [...new Set(allAddresses.map((a) => this.toBytea(a)))];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/ExplorerClient.ts` around lines 305 - 309, Deduplicate signer addresses before pushing into txSenders: when handling inventorySignerAddresses in ExplorerClient.ts (the block that currently loops and calls this.toBytea(addr)), convert addresses to a normalized form (e.g., the same casing/format or the bytea string from this.toBytea) and use a Set to filter duplicates and to exclude the rebalancerAddress (also normalized) so txSenders only receives unique entries; update the loop that builds txSenders (and any usage of this.toBytea) to add only addresses not already seen.typescript/rebalancer/src/utils/solanaKeyParser.ts (1)
97-103: Inner catch silently discards bs58 decode error details.When bs58.decode fails, the specific error message (e.g., "Non-base58 character") is lost in the swamp. The outer catch then throws a generic "Failed to parse key input" message, which ain't as helpful for debugging.
🧅 Optional: Preserve bs58 error details
try { const decoded = bs58.decode(trimmed); const bytes = Array.from(decoded); return toSecretKey(bytes); - } catch { - // bs58 decode failed, fall through to outer catch + } catch (bs58Error) { + const detail = bs58Error instanceof Error ? bs58Error.message : 'unknown'; + throwInvalidSolanaKey(`Base58 decode failed: ${detail}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/solanaKeyParser.ts` around lines 97 - 103, The inner catch currently swallows errors from bs58.decode; capture the error (e) there instead of an empty catch and propagate it so callers see the underlying cause—either rethrow the caught error or throw a new Error that includes the bs58 error message and context (e.g., "Failed to parse key input: " + e.message). Update the catch around bs58.decode in the block using bs58.decode(trimmed) and toSecretKey(bytes) to preserve and surface the original error details (or attach the original error as the cause) instead of discarding them.typescript/rebalancer/src/factories/RebalancerContextFactory.ts (1)
506-531: Duplicate mergedSigners logic between the two branches, mate.The
mergedSignersconstruction at lines 506-515 is duplicated at lines 548-556. This could be extracted into a helper function before the branching logic since both paths need the same merged signer structure.♻️ Extract mergedSigners helper
+ // Merge config addresses with runtime keys + const mergedSigners: Partial<Record<ProtocolType, InventorySignerConfig>> = + {}; + for (const [protocol, cfg] of Object.entries(inventorySigners)) { + const protocolKey = protocol as ProtocolType; + mergedSigners[protocolKey] = { + address: cfg.address, + key: cfg.key ?? this.inventorySignerKeysByProtocol?.[protocolKey], + }; + } + if (Object.keys(externalBridgeRegistry).length === 0) { // ... snip ... - const mergedSigners: Partial< - Record<ProtocolType, InventorySignerConfig> - > = {}; - for (const [protocol, cfg] of Object.entries(inventorySigners)) { - const protocolKey = protocol as ProtocolType; - mergedSigners[protocolKey] = { - address: cfg.address, - key: cfg.key ?? this.inventorySignerKeysByProtocol?.[protocolKey], - }; - } const inventoryRebalancer = new InventoryRebalancer(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/factories/RebalancerContextFactory.ts` around lines 506 - 531, The mergedSigners build logic is duplicated; extract it into a helper function (e.g., buildMergedSigners) that takes inventorySigners and this.inventorySignerKeysByProtocol and returns Partial<Record<ProtocolType, InventorySignerConfig>> by iterating Object.entries(inventorySigners) and assigning address and key (key ?? inventorySignerKeysByProtocol[protocolKey]); then replace both duplicated blocks with calls to buildMergedSigners and pass its result to InventoryRebalancer and the other branch so InventoryRebalancer instantiation and returned object use the single helper-generated mergedSigners.typescript/rebalancer/src/bridges/LiFiBridge.ts (1)
502-548: Mutex pattern for serialize execution - clever but needs a note.The Promise-chaining mutex at lines 502-508 serializes
executeRoutecalls to prevent concurrent LiFi SDK configuration races. The pattern is correct: acquire lock → wait for previous → execute → release. Consider adding a brief comment explaining why serialization is needed (LiFi SDK global provider state).📝 Add explanatory comment
+ // Serialize executions to prevent races in LiFi SDK's global provider configuration. + // configureLiFiProviders sets global state that must be stable during executeRoute. let release!: () => void; const acquired = new Promise<void>((resolve) => { release = resolve; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/bridges/LiFiBridge.ts` around lines 502 - 548, The mutex pattern using this._executeLock, acquired Promise/release and awaiting prev before calling executeRoute is fine but lacks an explanatory comment; add a brief comment above the lock logic (around _executeLock, release, acquired, prev) explaining that configureLiFiProviders mutates LiFi SDK global provider state and therefore executeRoute calls must be serialized to avoid race conditions when calling configureLiFiProviders(privateKeys, ...); mention that the lock protects configureLiFiProviders and route execution until release() is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@typescript/rebalancer/package.json`:
- Around line 49-51: The package.json in rebalancer references
"@solana/web3.js": "catalog:" which breaks pnpm resolution because there is no
catalog entry; fix by adding a catalog entry for "@solana/web3.js" with the
correct version into pnpm-workspace.yaml (matching the version used across
helloworld, infra, sdk, utils, widgets) so all packages can resolve the catalog
reference, or alternatively change the dependency in rebalancer's package.json
to a concrete version string instead of "catalog:"; update either the
pnpm-workspace.yaml catalog or the rebalancer dependency (symbol:
"@solana/web3.js" in package.json) so pnpm can resolve it.
In `@typescript/rebalancer/src/bridges/LiFiBridge.ts`:
- Line 17: The file currently imports from '@hyperlane-xyz/utils' twice;
consolidate the duplicate imports into a single import statement that includes
all used symbols (e.g., ProtocolType, ensure0x, and any other identifiers
currently imported from the same module) so there is only one import from
'@hyperlane-xyz/utils' in LiFiBridge.ts; update the existing import lines to
merge symbols and remove the redundant import.
In `@typescript/rebalancer/src/service.ts`:
- Around line 198-217: The loop that fail-fast checks ProtocolType
inventorySigners against inventoryPrivateKeys should be skipped when running in
monitor-only mode; update the logic around the loop that iterates
Object.values(ProtocolType) (and references rebalancerConfig.inventorySigners
and inventoryPrivateKeys) to first check the monitor-only flag (e.g.,
process.env.MONITOR_ONLY or the equivalent config property) and return/continue
so the missing InventorySignerConfig.key does not cause process.exit(1) in
monitor-only mode, while preserving the original enforcement in non-monitor
mode.
---
Outside diff comments:
In `@typescript/sdk/src/token/adapters/SealevelTokenAdapter.ts`:
- Around line 453-464: The method populateTransferRemoteTx currently accepts
extraSigners: TransferRemoteParams but only uses extraSigners[0] to set
randomWallet, silently dropping others; update the API to match the
implementation by changing TransferRemoteParams to accept a single optional
signer (e.g. extraSigner?: Keypair) and update populateTransferRemoteTx to read
extraSigner (or, alternatively, if multi-signer support is desired, modify
populateTransferRemoteTx to consume and attach all signers rather than only the
first). Ensure references to extraSigners and the randomWallet assignment (and
any tests expecting single-signer behavior) are adjusted to use the new
extraSigner symbol or the multi-signer flow consistently.
---
Nitpick comments:
In `@typescript/rebalancer/src/bridges/LiFiBridge.ts`:
- Around line 502-548: The mutex pattern using this._executeLock, acquired
Promise/release and awaiting prev before calling executeRoute is fine but lacks
an explanatory comment; add a brief comment above the lock logic (around
_executeLock, release, acquired, prev) explaining that configureLiFiProviders
mutates LiFi SDK global provider state and therefore executeRoute calls must be
serialized to avoid race conditions when calling
configureLiFiProviders(privateKeys, ...); mention that the lock protects
configureLiFiProviders and route execution until release() is called.
In `@typescript/rebalancer/src/factories/RebalancerContextFactory.ts`:
- Around line 506-531: The mergedSigners build logic is duplicated; extract it
into a helper function (e.g., buildMergedSigners) that takes inventorySigners
and this.inventorySignerKeysByProtocol and returns Partial<Record<ProtocolType,
InventorySignerConfig>> by iterating Object.entries(inventorySigners) and
assigning address and key (key ?? inventorySignerKeysByProtocol[protocolKey]);
then replace both duplicated blocks with calls to buildMergedSigners and pass
its result to InventoryRebalancer and the other branch so InventoryRebalancer
instantiation and returned object use the single helper-generated mergedSigners.
In `@typescript/rebalancer/src/utils/bridgeUtils.test.ts`:
- Around line 16-37: Tests use raw string literals for executionType instead of
the ExecutionType enum; update all occurrences in this test file (including the
cases around lines indicated) to use ExecutionType.MovableCollateral (and other
appropriate ExecutionType values) so values are consistent with the rest of the
suite and types like BridgeConfigWithOverride, ChainMap and the getBridgeConfig
call remain strongly typed.
- Around line 121-145: Add a blank line between the previous test and this
"should apply override with executionType: inventory and externalBridge" test
for readability, and remove or consolidate this test with the similar one
starting at line 147 by either merging assertions or making their intentions
explicit; specifically, keep a single test using getBridgeConfig and
isInventoryConfig to assert executionType becomes ExecutionType.Inventory and
externalBridge equals ExternalBridgeType.LiFi, and if you need to verify absence
of bridgeMinAcceptedAmount, include that as an explicit assertion in the
consolidated test so the behavior around BridgeConfigWithOverride is
unambiguous.
In `@typescript/rebalancer/src/utils/ExplorerClient.ts`:
- Around line 305-309: Deduplicate signer addresses before pushing into
txSenders: when handling inventorySignerAddresses in ExplorerClient.ts (the
block that currently loops and calls this.toBytea(addr)), convert addresses to a
normalized form (e.g., the same casing/format or the bytea string from
this.toBytea) and use a Set to filter duplicates and to exclude the
rebalancerAddress (also normalized) so txSenders only receives unique entries;
update the loop that builds txSenders (and any usage of this.toBytea) to add
only addresses not already seen.
In `@typescript/rebalancer/src/utils/gasEstimation.ts`:
- Around line 214-242: Add a debug log in the non-EVM branch (the block that
checks multiProvider.getProtocol(originChain) !== ProtocolType.Ethereum) before
the return, mirroring the EVM-path logging: log originChain and originProtocol
plus computed values igpCost, tokenFeeCost, gasCost (0), totalCost,
maxTransferable, minViableTransfer and gasQuote using the existing logger used
elsewhere in this module (e.g., logger.debug or processLogger.debug) so failures
in non-EVM transfers emit the same breadcrumbs as the EVM path.
In `@typescript/rebalancer/src/utils/solanaKeyParser.ts`:
- Around line 97-103: The inner catch currently swallows errors from
bs58.decode; capture the error (e) there instead of an empty catch and propagate
it so callers see the underlying cause—either rethrow the caught error or throw
a new Error that includes the bs58 error message and context (e.g., "Failed to
parse key input: " + e.message). Update the catch around bs58.decode in the
block using bs58.decode(trimmed) and toSecretKey(bytes) to preserve and surface
the original error details (or attach the original error as the cause) instead
of discarding them.
In `@typescript/rebalancer/src/utils/transactionUtils.ts`:
- Around line 34-35: The lookup for expectedType uses a cast to
KnownProtocolType without validating expectedProtocol first; update the code
around expectedProtocol, PROTOCOL_TO_DEFAULT_PROVIDER_TYPE and expectedType to
first check that expectedProtocol is a valid KnownProtocolType (e.g., not
ProtocolType.Unknown and present as a key in PROTOCOL_TO_DEFAULT_PROVIDER_TYPE)
and, if not, throw or return a descriptive error that includes the actual
expectedProtocol value instead of relying on the later assert; implement this as
an explicit guard/type check before computing expectedType so the failure
message is clear and immediate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fa9cf74-18f4-4de8-9ba1-cce966a8b220
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
.changeset/mixed-rebalancer-mode.md.changeset/sdk-solana-extra-signers.mdtypescript/cli/src/commands/warp.tstypescript/infra/config/environments/mainnet3/rebalancer/USDC/eclipsemainnet-config.yamltypescript/rebalancer-sim/src/runners/ProductionRebalancerRunner.tstypescript/rebalancer/package.jsontypescript/rebalancer/src/bridges/LiFiBridge.test.tstypescript/rebalancer/src/bridges/LiFiBridge.tstypescript/rebalancer/src/config/RebalancerConfig.test.tstypescript/rebalancer/src/config/RebalancerConfig.tstypescript/rebalancer/src/config/types.tstypescript/rebalancer/src/core/InventoryRebalancer.test.tstypescript/rebalancer/src/core/InventoryRebalancer.tstypescript/rebalancer/src/core/RebalancerService.test.tstypescript/rebalancer/src/core/RebalancerService.tstypescript/rebalancer/src/e2e/fixtures/routes.tstypescript/rebalancer/src/e2e/harness/MixedLocalDeploymentManager.tstypescript/rebalancer/src/e2e/harness/MockExternalBridge.tstypescript/rebalancer/src/e2e/harness/TestRebalancer.tstypescript/rebalancer/src/e2e/mixed-weighted.e2e-test.tstypescript/rebalancer/src/factories/RebalancerContextFactory.test.tstypescript/rebalancer/src/factories/RebalancerContextFactory.tstypescript/rebalancer/src/interfaces/IExternalBridge.tstypescript/rebalancer/src/monitor/Monitor.tstypescript/rebalancer/src/service.tstypescript/rebalancer/src/test/lifiMocks.tstypescript/rebalancer/src/tracking/ActionTracker.tstypescript/rebalancer/src/utils/ExplorerClient.tstypescript/rebalancer/src/utils/blockTag.tstypescript/rebalancer/src/utils/bridgeUtils.test.tstypescript/rebalancer/src/utils/gasEstimation.tstypescript/rebalancer/src/utils/solanaKeyParser.test.tstypescript/rebalancer/src/utils/solanaKeyParser.tstypescript/rebalancer/src/utils/tokenUtils.tstypescript/rebalancer/src/utils/transactionUtils.tstypescript/sdk/src/providers/ProviderType.tstypescript/sdk/src/signers/svm/solana-web3js.test.tstypescript/sdk/src/signers/svm/solana-web3js.tstypescript/sdk/src/token/adapters/ITokenAdapter.tstypescript/sdk/src/token/adapters/SealevelTokenAdapter.tstypescript/sdk/src/warp/WarpCore.test.tstypescript/sdk/src/warp/WarpCore.ts
💤 Files with no reviewable changes (3)
- typescript/rebalancer/src/core/RebalancerService.test.ts
- typescript/cli/src/commands/warp.ts
- typescript/rebalancer-sim/src/runners/ProductionRebalancerRunner.ts
paulbalaji
reviewed
Mar 9, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Review Summary
Large, well-structured PR that adds mixed-mode rebalancing across EVM and Sealevel. The architecture is sound — per-protocol signer map, WarpCore-based transferRemote, mutex on LiFi, startup validation. Private key redaction is thorough, assert() usage follows codebase conventions, and the inventoryMultiProvider cleanup is complete.
However, there are three high-severity bugs in the LiFi chain ID translation layer that will break Solana bridging in production. The toLiFiChainId() translation was correctly added to the quote*() methods but not propagated to execute() protocol lookups, getStatus() polling, or the monitor's chain list. These are invisible in the all-EVM e2e tests.
High: Three Related Chain-ID Translation Bugs
- Monitor doesn't fetch balances for inventory source chains — see inline comment on
RebalancerContextFactory.ts execute()protocol lookup uses LiFi chain ID against registry-keyed map — see inline comment onLiFiBridge.tsgetStatus()sends Hyperlane domain IDs to LiFi API —toLiFiChainId()is applied inquote*()but not ingetStatus(). SincegetStatus()isn't in the diff, noting here:inventory_movementactions store Hyperlane domain IDs (Solana =1399811149),ActionTracker.syncInventoryMovementActions()passes them togetStatus()unchanged, and LiFi expects1151111081099710for Solana. Completed Solana bridges will returnNOT_FOUNDforever, leaving actions stuck asin_progress.
Medium: DRY Violation
The override executionType extraction pattern (typeof overrideConfig === 'object' && ... ? (overrideConfig as { executionType? }).executionType : undefined) is duplicated in getInventoryChainNames, getInventoryOriginChainNames, and the superRefine validation. Extract a shared helper like getOverrideExecutionType(overrideConfig, fallbackExecutionType).
Other Observations
sign→partialSigninKeypairSvmTransactionSigneris a behavioral change for all SVM callers (not just rebalancer). Correct for this use case and regression-tested, but worth noting the scope.- Monitor returns
balance: 0nsilently wheninventoryAddresseslacks an entry for a chain's protocol (Monitor.ts). This masks config errors per cycle — consider failing at startup validation. - CodeRabbit's finding about
@solana/web3.jsmissing from the pnpm catalog is valid and should be addressed. - CodeRabbit's finding about the monitor-only key enforcement (
service.ts:198-217) is also valid.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
typescript/rebalancer/src/bridges/LiFiBridge.ts (1)
17-22:⚠️ Potential issue | 🟡 MinorConsolidate duplicate imports from
@hyperlane-xyz/utils.Lines 17 and 22 both import from
@hyperlane-xyz/utils. The linter flagged this in a past review. Merge them into a single import statement.🔧 Consolidate imports
-import { ProtocolType, ensure0x } from '@hyperlane-xyz/utils'; import type { Logger } from 'pino'; import { type Chain, createWalletClient, http } from 'viem'; import { privateKeyToAccount } from 'viem/accounts'; import { arbitrum, base, mainnet, optimism } from 'viem/chains'; -import { assert } from '@hyperlane-xyz/utils'; +import { ProtocolType, assert, ensure0x } from '@hyperlane-xyz/utils';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/bridges/LiFiBridge.ts` around lines 17 - 22, Merge the two separate imports from `@hyperlane-xyz/utils` into a single import statement: remove the duplicate import lines and import ProtocolType, ensure0x, and assert together (e.g., import { ProtocolType, ensure0x, assert } from '@hyperlane-xyz/utils') at the top of LiFiBridge.ts where the current imports for ProtocolType/ensure0x and assert are declared so there is only one consolidated import for that module.
🧹 Nitpick comments (3)
typescript/rebalancer/src/config/types.ts (1)
356-364: Sealevel base58 regex could use a wee bit more strictness.The regex
/^[1-9A-HJ-NP-Za-km-z]{32,44}$/validates the base58 character set correctly, but Solana public keys are always exactly 32 bytes, which encodes to 43-44 base58 characters (never 32). Consider tightening to{43,44}for more precise validation.🔧 Tighter regex for Solana addresses
- if (!/^[1-9A-HJ-NP-Za-km-z]{32,44}$/.test(signerConfig.address)) { + if (!/^[1-9A-HJ-NP-Za-km-z]{43,44}$/.test(signerConfig.address)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/config/types.ts` around lines 356 - 364, The Solana (Sealevel) address regex in the ProtocolType.Sealevel branch is too permissive; update the validation for signerConfig.address used in the protocol === ProtocolType.Sealevel block (where ctx.addIssue is called for inventorySigners) to require 43–44 base58 chars instead of 32–44 (i.e. change the quantifier to {43,44}) and keep the existing error path/message in ctx.addIssue to reflect the stricter length check.typescript/rebalancer/src/service.ts (1)
161-171: Solana key parsing could benefit from explicit error handling.The
parseSolanaPrivateKeyandKeypair.fromSecretKeycalls could throw if the key format is invalid. While the outer try/catch at line 256 would catch it, a more specific error message here would help operators diagnose key configuration issues faster.🛡️ Add targeted error handling for Solana key parsing
} else if (protocol === ProtocolType.Sealevel) { - const keyBytes = parseSolanaPrivateKey(privateKey); - const keypair = Keypair.fromSecretKey(keyBytes); - derivedAddress = keypair.publicKey.toBase58(); + try { + const keyBytes = parseSolanaPrivateKey(privateKey); + const keypair = Keypair.fromSecretKey(keyBytes); + derivedAddress = keypair.publicKey.toBase58(); + } catch (err) { + throw new Error( + `Invalid Solana private key for HYP_INVENTORY_KEY_SEALEVEL: ${(err as Error).message}`, + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/service.ts` around lines 161 - 171, Wrap the Solana key handling for ProtocolType.Sealevel (the calls to parseSolanaPrivateKey and Keypair.fromSecretKey that set derivedAddress) in a targeted try/catch so invalid key formats are caught and logged with a clear, specific message (use logger.error or logger.warn) that includes the protocol and the incoming privateKey identifier/context, then skip/continue on error; keep the existing continue behavior for unsupported protocols.typescript/rebalancer/src/e2e/mixed-weighted.e2e-test.ts (1)
136-144: Snapshot restoration looks fine, just watch that provider reset hack.The
Reflect.setcalls to reset_maxInternalBlockNumberand_internalBlockNumberare internal ethers provider state. This works for tests but relies on implementation details. If ethers updates, this might break silently. Might be worth a wee comment explaining why this is needed.📝 Add explanatory comment
await revertToSnapshot(provider, id); snapshotIds.set(chain, await snapshot(provider)); + // Reset ethers internal block cache to avoid stale block number after revert Reflect.set(provider, '_maxInternalBlockNumber', -1); Reflect.set(provider, '_internalBlockNumber', null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/e2e/mixed-weighted.e2e-test.ts` around lines 136 - 144, The test uses Reflect.set to mutate provider internals (_maxInternalBlockNumber and _internalBlockNumber) in the afterEach block (iterating localProviders) which relies on ethers implementation details; add a concise comment above those Reflect.set calls explaining why this hack is necessary (to reset ethers' internal block tracking between snapshots in these e2e tests), note the fragility (may break on ethers upgrades), and include a TODO to revisit if ethers exposes a public reset API so future maintainers understand the intent and risk.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@typescript/rebalancer/src/bridges/LiFiBridge.ts`:
- Around line 17-22: Merge the two separate imports from `@hyperlane-xyz/utils`
into a single import statement: remove the duplicate import lines and import
ProtocolType, ensure0x, and assert together (e.g., import { ProtocolType,
ensure0x, assert } from '@hyperlane-xyz/utils') at the top of LiFiBridge.ts
where the current imports for ProtocolType/ensure0x and assert are declared so
there is only one consolidated import for that module.
---
Nitpick comments:
In `@typescript/rebalancer/src/config/types.ts`:
- Around line 356-364: The Solana (Sealevel) address regex in the
ProtocolType.Sealevel branch is too permissive; update the validation for
signerConfig.address used in the protocol === ProtocolType.Sealevel block (where
ctx.addIssue is called for inventorySigners) to require 43–44 base58 chars
instead of 32–44 (i.e. change the quantifier to {43,44}) and keep the existing
error path/message in ctx.addIssue to reflect the stricter length check.
In `@typescript/rebalancer/src/e2e/mixed-weighted.e2e-test.ts`:
- Around line 136-144: The test uses Reflect.set to mutate provider internals
(_maxInternalBlockNumber and _internalBlockNumber) in the afterEach block
(iterating localProviders) which relies on ethers implementation details; add a
concise comment above those Reflect.set calls explaining why this hack is
necessary (to reset ethers' internal block tracking between snapshots in these
e2e tests), note the fragility (may break on ethers upgrades), and include a
TODO to revisit if ethers exposes a public reset API so future maintainers
understand the intent and risk.
In `@typescript/rebalancer/src/service.ts`:
- Around line 161-171: Wrap the Solana key handling for ProtocolType.Sealevel
(the calls to parseSolanaPrivateKey and Keypair.fromSecretKey that set
derivedAddress) in a targeted try/catch so invalid key formats are caught and
logged with a clear, specific message (use logger.error or logger.warn) that
includes the protocol and the incoming privateKey identifier/context, then
skip/continue on error; keep the existing continue behavior for unsupported
protocols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ad665c9-3fc8-4ad6-b175-558a72c20144
📒 Files selected for processing (6)
typescript/rebalancer/src/bridges/LiFiBridge.test.tstypescript/rebalancer/src/bridges/LiFiBridge.tstypescript/rebalancer/src/config/types.tstypescript/rebalancer/src/e2e/mixed-weighted.e2e-test.tstypescript/rebalancer/src/factories/RebalancerContextFactory.tstypescript/rebalancer/src/service.ts
Collaborator
Author
Addressing Review from @paulbalaji (review)All high-severity bugs and the DRY violation have been fixed. Two SDK-level concerns are deferred to a separate PR. High: Chain ID Translation Bugs — All Fixed
Medium: DRY Violation — FixedExtracted Other Observations — Addressed
Deferred to Separate SDK PR
|
Mo-Hussain
force-pushed
the
feat/mixed-rebalancer-mode-v3
branch
from
02e9673 to
511e02b
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
typescript/rebalancer/src/utils/gasEstimation.ts (1)
219-241:⚠️ Potential issue | 🟠 MajorMake this zero-fee path Sealevel-only.
A wee trap here: Line 220 sends every non-Ethereum origin down
gasCost = 0n. That hides unsupported protocols, and for native Sealevel balances it can overstatemaxTransferable. IfavailableInventory === requestedAmount + igpCost + tokenFeeCost, this branch still marks the full send viable even though the payer needs origin-chain fee balance; downstream these thresholds are used as routing gates. Gate this withassert(originProtocol === ProtocolType.Sealevel, ...), and reserve a small local fee floor or adapter-provided estimate before derivingmaxTransferable/minViableTransfer. Would add an exact-balance test for this path too.As per coding guidelines "Use
assert()from '@hyperlane-xyz/utils' for validating preconditions, invariants, and unexpected states rather than throwing custom errors" and "Do not add defensive fallbacks that mask bugs".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/gasEstimation.ts` around lines 219 - 241, The non-Ethereum branch currently sets gasCost = 0n for all non-Ethereum origins which masks unsupported protocols and can overstate maxTransferable; update the branch in the gas estimation logic (around originProtocol, ProtocolType.Ethereum, gasCost, maxTransferable, minViableTransfer, igpCost, tokenFeeCost, availableInventory, requestedAmount) to assert originProtocol === ProtocolType.Sealevel using assert() from '@hyperlane-xyz/utils', then compute a small reserved fee floor (or use an adapter-provided local fee estimate) and include that reserved fee when calculating totalCost/maxAfterReservation before deriving maxTransferable and minViableTransfer so the Sealevel-only path remains correct and does not hide other protocols.
🧹 Nitpick comments (5)
typescript/rebalancer/src/e2e/fixtures/routes.ts (1)
366-370: Naming doesn't quite fit in with the other ogr— entries here.Other ERC20 entries in this object follow the
ERC20_SIGNER_*pattern (e.g.,ERC20_SIGNER_PARTIAL_ANVIL2,ERC20_SIGNER_LOW_ALL). This one's calledERC20_INVENTORY_BALANCEDwhich strays from the convention. Also, same key already lives inBALANCE_PRESETSat line 236, which might confuse folks pokin' through tests.Consider renaming to
ERC20_SIGNER_BALANCEDfor consistency with its neighbors.🧅 Suggested rename
- ERC20_INVENTORY_BALANCED: { + ERC20_SIGNER_BALANCED: { anvil1: '5000000000', anvil2: '5000000000', anvil3: '5000000000', },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/e2e/fixtures/routes.ts` around lines 366 - 370, Rename the ERC20_INVENTORY_BALANCED key to ERC20_SIGNER_BALANCED to match the ERC20_SIGNER_* naming convention and avoid confusion with the existing BALANCE_PRESETS entry; update the object key in routes.ts (symbol ERC20_INVENTORY_BALANCED) and search-and-replace any references/usages to ERC20_SIGNER_BALANCED throughout tests/fixtures, and reconcile/remove the duplicate entry in BALANCE_PRESETS so only the consistent ERC20_SIGNER_BALANCED name is present.typescript/rebalancer/src/utils/solanaKeyParser.ts (1)
97-116: Control flow looks correct but could be clearer.When base58 decoding fails, the inner catch (line 101-103) swallows the error and falls through to line 114, which throws with a "failed to match supported format" message. This is the intended behavior but takes a moment to trace through the nested try/catch structure.
Consider a minor refactor to make the flow more explicit:
♻️ Optional: explicit early return pattern
- try { - const decoded = bs58.decode(trimmed); - const bytes = Array.from(decoded); - return toSecretKey(bytes); - } catch { - // bs58 decode failed, fall through to outer catch - } - } catch (error) { - if ( - error instanceof Error && - error.message.includes(SOLANA_KEY_ERROR_MESSAGE) - ) { - throw error; - } - throwInvalidSolanaKey('Failed to parse key input.'); - } - - throwInvalidSolanaKey( - 'Failed to match supported format (JSON array, comma-separated bytes, or base58 key).', - ); + const decoded = bs58.decode(trimmed); + const bytes = Array.from(decoded); + return toSecretKey(bytes); + } catch (error) { + if ( + error instanceof Error && + error.message.includes(SOLANA_KEY_ERROR_MESSAGE) + ) { + throw error; + } + // All formats failed + throwInvalidSolanaKey( + 'Failed to match supported format (JSON array, comma-separated bytes, or base58 key).', + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/solanaKeyParser.ts` around lines 97 - 116, Refactor the nested try/catch around bs58.decode to an explicit, clearer control flow: replace the inner try/catch by calling a small parsing step (e.g., parseBase58) or perform bs58.decode and, on failure, explicitly set a local flag/variable to indicate "not base58" instead of silently swallowing the error; if decode succeeds convert bytes via toSecretKey and return, otherwise let the outer logic continue to other format checks and ultimately call throwInvalidSolanaKey as before. Keep the outer catch behavior intact (preserve checking SOLANA_KEY_ERROR_MESSAGE) and reference the existing symbols bs58.decode, toSecretKey, throwInvalidSolanaKey, and SOLANA_KEY_ERROR_MESSAGE when updating the code so behavior remains identical but control flow is clearer.typescript/rebalancer/src/bridges/LiFiBridge.ts (1)
86-100: Drop the exception probe intoBase58SolanaKey— letparseSolanaPrivateKeyhandle all the formats.The silent
try/catchonbs58.decodeis using exceptions to steer control flow.parseSolanaPrivateKey()already normalizes everything (JSON arrays, comma-separated bytes, base58) with proper error handling, so just feed it the input and re-encode:Suggested simplification
function toBase58SolanaKey(rawKey: string): string { const bytes = parseSolanaPrivateKey(rawKey); return bs58.encode(bytes); }Per guidelines: don't use exceptions for control flow, always log or re-throw errors instead of swallowing them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/bridges/LiFiBridge.ts` around lines 86 - 100, The try/catch in toBase58SolanaKey is using exceptions for control flow and silently swallowing errors; remove the bs58.decode probe and instead call parseSolanaPrivateKey directly (pass the trimmed rawKey) to normalize all formats, then return bs58.encode(bytes); ensure to drop the silent catch so parseSolanaPrivateKey handles validation/errors and they propagate or are handled there.typescript/rebalancer/src/core/InventoryRebalancer.test.ts (1)
157-167: Minor inconsistency:categoryuses string literal whiletypeuses enum.Line 159 uses the string
'transfer'while line 160 correctly usesProviderType.EthersV5. For consistency with the enum usage pattern (and matching the new test at line 357 which usesWarpTxCategory.Transfer), consider updating this.🔧 Suggested fix
warpCore.getTransferRemoteTxs.resolves([ { - category: 'transfer', + category: WarpTxCategory.Transfer, type: ProviderType.EthersV5, transaction: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/core/InventoryRebalancer.test.ts` around lines 157 - 167, Test object in the getTransferRemoteTxs stub uses a string literal for the category ('transfer') which is inconsistent with enum usage elsewhere; update the stub to use the WarpTxCategory enum (e.g., WarpTxCategory.Transfer) instead of the raw string so it matches the other tests and the new test at line 357; locate the getTransferRemoteTxs stub in InventoryRebalancer.test.ts and replace the category value with WarpTxCategory.Transfer while keeping the rest of the transaction object and ProviderType.EthersV5 unchanged.typescript/rebalancer/src/factories/RebalancerContextFactory.ts (1)
520-540: Consider extracting the duplicatedmergedSignersconstruction.Both the early-return path (lines 520-540) and the normal path (lines 562-581) build
mergedSignersidentically, then createInventoryRebalancerwith nearly the same arguments. Moving this to a small helper would make it easier to maintain and reduce the risk of the two paths drifting apart.That said, this isn't blocking — if you'd rather keep things inline for now, that's fine too. Just something to keep in mind for future cleanup.
Also applies to: 562-581
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/factories/RebalancerContextFactory.ts` around lines 520 - 540, The duplicated construction of mergedSigners and the subsequent InventoryRebalancer instantiation should be extracted into a small helper to avoid drift: create a private/helper function (e.g., buildInventoryRebalancer or buildMergedSignersAndRebalancer) that takes inventorySigners, inventoryChains, actionTracker, externalBridgeRegistryOverride (and accesses this.inventorySignerKeysByProtocol, this.warpCore, this.multiProvider, this.logger) and returns the InventoryRebalancer instance (or returns { mergedSigners } and a separate factory call); then replace both inline blocks that currently build mergedSigners and call new InventoryRebalancer with a single call to that helper to centralize the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@typescript/rebalancer/src/config/types.ts`:
- Around line 137-145: The inventorySigners schema currently uses
z.record(z.nativeEnum(ProtocolType), ...) which requires every ProtocolType key;
change it to use a partial record so keys are optional — replace the
z.record(...) call for inventorySigners with
z.partialRecord(z.nativeEnum(ProtocolType), z.union([z.object({ address:
z.string(), key: z.string().optional() }), z.string().transform((address) => ({
address }))])).optional() so the schema accepts partial protocol configs while
keeping the same value shape and .optional() wrapper.
In `@typescript/rebalancer/src/service.ts`:
- Around line 222-232: The current replacement of the entire signer map with
inventorySigners loses address-only/YAML signers; instead build a merged signer
map and pass that into the RebalancerConfig constructor: create mergedSigners by
starting from the existing rebalancerConfig's signer map and then for each
protocol key merge/overlay inventorySigners[protocol] into it (so existing
protocol maps are preserved and only env-backed entries are added/overlaid),
then construct mergedRebalancerConfig using mergedSigners (use the same
RebalancerConfig(...) call but with mergedSigners instead of inventorySigners).
---
Duplicate comments:
In `@typescript/rebalancer/src/utils/gasEstimation.ts`:
- Around line 219-241: The non-Ethereum branch currently sets gasCost = 0n for
all non-Ethereum origins which masks unsupported protocols and can overstate
maxTransferable; update the branch in the gas estimation logic (around
originProtocol, ProtocolType.Ethereum, gasCost, maxTransferable,
minViableTransfer, igpCost, tokenFeeCost, availableInventory, requestedAmount)
to assert originProtocol === ProtocolType.Sealevel using assert() from
'@hyperlane-xyz/utils', then compute a small reserved fee floor (or use an
adapter-provided local fee estimate) and include that reserved fee when
calculating totalCost/maxAfterReservation before deriving maxTransferable and
minViableTransfer so the Sealevel-only path remains correct and does not hide
other protocols.
---
Nitpick comments:
In `@typescript/rebalancer/src/bridges/LiFiBridge.ts`:
- Around line 86-100: The try/catch in toBase58SolanaKey is using exceptions for
control flow and silently swallowing errors; remove the bs58.decode probe and
instead call parseSolanaPrivateKey directly (pass the trimmed rawKey) to
normalize all formats, then return bs58.encode(bytes); ensure to drop the silent
catch so parseSolanaPrivateKey handles validation/errors and they propagate or
are handled there.
In `@typescript/rebalancer/src/core/InventoryRebalancer.test.ts`:
- Around line 157-167: Test object in the getTransferRemoteTxs stub uses a
string literal for the category ('transfer') which is inconsistent with enum
usage elsewhere; update the stub to use the WarpTxCategory enum (e.g.,
WarpTxCategory.Transfer) instead of the raw string so it matches the other tests
and the new test at line 357; locate the getTransferRemoteTxs stub in
InventoryRebalancer.test.ts and replace the category value with
WarpTxCategory.Transfer while keeping the rest of the transaction object and
ProviderType.EthersV5 unchanged.
In `@typescript/rebalancer/src/e2e/fixtures/routes.ts`:
- Around line 366-370: Rename the ERC20_INVENTORY_BALANCED key to
ERC20_SIGNER_BALANCED to match the ERC20_SIGNER_* naming convention and avoid
confusion with the existing BALANCE_PRESETS entry; update the object key in
routes.ts (symbol ERC20_INVENTORY_BALANCED) and search-and-replace any
references/usages to ERC20_SIGNER_BALANCED throughout tests/fixtures, and
reconcile/remove the duplicate entry in BALANCE_PRESETS so only the consistent
ERC20_SIGNER_BALANCED name is present.
In `@typescript/rebalancer/src/factories/RebalancerContextFactory.ts`:
- Around line 520-540: The duplicated construction of mergedSigners and the
subsequent InventoryRebalancer instantiation should be extracted into a small
helper to avoid drift: create a private/helper function (e.g.,
buildInventoryRebalancer or buildMergedSignersAndRebalancer) that takes
inventorySigners, inventoryChains, actionTracker, externalBridgeRegistryOverride
(and accesses this.inventorySignerKeysByProtocol, this.warpCore,
this.multiProvider, this.logger) and returns the InventoryRebalancer instance
(or returns { mergedSigners } and a separate factory call); then replace both
inline blocks that currently build mergedSigners and call new
InventoryRebalancer with a single call to that helper to centralize the logic.
In `@typescript/rebalancer/src/utils/solanaKeyParser.ts`:
- Around line 97-116: Refactor the nested try/catch around bs58.decode to an
explicit, clearer control flow: replace the inner try/catch by calling a small
parsing step (e.g., parseBase58) or perform bs58.decode and, on failure,
explicitly set a local flag/variable to indicate "not base58" instead of
silently swallowing the error; if decode succeeds convert bytes via toSecretKey
and return, otherwise let the outer logic continue to other format checks and
ultimately call throwInvalidSolanaKey as before. Keep the outer catch behavior
intact (preserve checking SOLANA_KEY_ERROR_MESSAGE) and reference the existing
symbols bs58.decode, toSecretKey, throwInvalidSolanaKey, and
SOLANA_KEY_ERROR_MESSAGE when updating the code so behavior remains identical
but control flow is clearer.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59ceaad9-e74a-4813-9025-46843e89e68b
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
.changeset/mixed-rebalancer-mode.md.changeset/sdk-solana-extra-signers.mdtypescript/cli/src/commands/warp.tstypescript/infra/config/environments/mainnet3/rebalancer/USDC/eclipsemainnet-config.yamltypescript/rebalancer-sim/src/runners/ProductionRebalancerRunner.tstypescript/rebalancer/package.jsontypescript/rebalancer/src/bridges/LiFiBridge.test.tstypescript/rebalancer/src/bridges/LiFiBridge.tstypescript/rebalancer/src/config/RebalancerConfig.test.tstypescript/rebalancer/src/config/RebalancerConfig.tstypescript/rebalancer/src/config/types.tstypescript/rebalancer/src/core/InventoryRebalancer.test.tstypescript/rebalancer/src/core/InventoryRebalancer.tstypescript/rebalancer/src/core/RebalancerService.test.tstypescript/rebalancer/src/core/RebalancerService.tstypescript/rebalancer/src/e2e/fixtures/routes.tstypescript/rebalancer/src/e2e/harness/MixedLocalDeploymentManager.tstypescript/rebalancer/src/e2e/harness/MockExternalBridge.tstypescript/rebalancer/src/e2e/harness/TestRebalancer.tstypescript/rebalancer/src/e2e/mixed-weighted.e2e-test.tstypescript/rebalancer/src/factories/RebalancerContextFactory.test.tstypescript/rebalancer/src/factories/RebalancerContextFactory.tstypescript/rebalancer/src/interfaces/IExternalBridge.tstypescript/rebalancer/src/monitor/Monitor.tstypescript/rebalancer/src/service.tstypescript/rebalancer/src/test/lifiMocks.tstypescript/rebalancer/src/tracking/ActionTracker.tstypescript/rebalancer/src/utils/ExplorerClient.tstypescript/rebalancer/src/utils/blockTag.tstypescript/rebalancer/src/utils/bridgeUtils.test.tstypescript/rebalancer/src/utils/gasEstimation.tstypescript/rebalancer/src/utils/solanaKeyParser.test.tstypescript/rebalancer/src/utils/solanaKeyParser.tstypescript/rebalancer/src/utils/tokenUtils.tstypescript/rebalancer/src/utils/transactionUtils.tstypescript/sdk/src/providers/ProviderType.tstypescript/sdk/src/signers/svm/solana-web3js.test.tstypescript/sdk/src/signers/svm/solana-web3js.tstypescript/sdk/src/token/adapters/ITokenAdapter.tstypescript/sdk/src/token/adapters/SealevelTokenAdapter.tstypescript/sdk/src/warp/WarpCore.test.tstypescript/sdk/src/warp/WarpCore.ts
💤 Files with no reviewable changes (3)
- typescript/rebalancer/src/core/RebalancerService.test.ts
- typescript/cli/src/commands/warp.ts
- typescript/rebalancer-sim/src/runners/ProductionRebalancerRunner.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- typescript/rebalancer/src/utils/bridgeUtils.test.ts
- typescript/rebalancer/src/utils/ExplorerClient.ts
- typescript/rebalancer/src/utils/tokenUtils.ts
- typescript/sdk/src/token/adapters/SealevelTokenAdapter.ts
- typescript/rebalancer/src/e2e/mixed-weighted.e2e-test.ts
- typescript/sdk/src/providers/ProviderType.ts
- typescript/sdk/src/warp/WarpCore.test.ts
- .changeset/mixed-rebalancer-mode.md
- typescript/sdk/src/signers/svm/solana-web3js.test.ts
- typescript/rebalancer/src/utils/transactionUtils.ts
- typescript/rebalancer/src/tracking/ActionTracker.ts
- typescript/rebalancer/package.json
- typescript/rebalancer/src/test/lifiMocks.ts
- typescript/rebalancer/src/utils/solanaKeyParser.test.ts
- typescript/rebalancer/src/config/RebalancerConfig.ts
- .changeset/sdk-solana-extra-signers.md
… non-EVM chains LiFi uses different chain identifiers than Hyperlane for non-EVM chains. For Solana, Hyperlane domain ID 1399811149 was being sent to LiFi which expects 1151111081099710, causing quote requests to fail with 'toChain must be equal to one of the allowed values'. Added HYPERLANE_TO_LIFI_CHAIN_IDS mapping and toLiFiChainId() translation at the LiFi API boundary in both quoteBySpendingAmount() and quoteByReceivingAmount(). EVM chains pass through unchanged.
…gners in WarpCore test
Mo-Hussain
force-pushed
the
feat/mixed-rebalancer-mode-v3
branch
from
511e02b to
34956cb
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
typescript/rebalancer/src/service.ts (1)
222-232:⚠️ Potential issue | 🟠 MajorThis still drops address-only signers from YAML, ogre.
The merge logic passes only
inventorySigners(env-backed entries) to the constructor, which replaces the entire map. Any address-only signer from YAML gets lost, breaking mixed monitor-only or partially keyed setups.Suggested fix - merge per-protocol instead of replace
+ // Merge runtime keys into config, preserving address-only YAML entries + const mergedInventorySigners: Partial< + Record<ProtocolType, InventorySignerConfig> + > = { ...(rebalancerConfig.inventorySigners ?? {}) }; + for (const protocol of Object.values(ProtocolType)) { + const runtimeSigner = inventorySigners[protocol]; + if (runtimeSigner) { + mergedInventorySigners[protocol] = { + ...mergedInventorySigners[protocol], + ...runtimeSigner, + }; + } + } + const mergedRebalancerConfig = - Object.keys(inventorySigners).length > 0 + Object.keys(mergedInventorySigners).length > 0 ? new RebalancerConfig( rebalancerConfig.warpRouteId, rebalancerConfig.strategyConfig, rebalancerConfig.intentTTL, - inventorySigners, + mergedInventorySigners, rebalancerConfig.externalBridges, ) : rebalancerConfig;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/service.ts` around lines 222 - 232, The current merge creates a new RebalancerConfig using only inventorySigners which replaces and drops address-only signers from the original rebalancerConfig; instead, build a merged signers map that preserves existing rebalancerConfig's signers and overlays inventorySigners (inventory entries override same-key entries but do not remove other protocol/address-only entries) and pass that merged map into the new RebalancerConfig constructor; update the creation of mergedRebalancerConfig to compute mergedSigners = merge(rebalancerConfig.signers, inventorySigners) and use mergedSigners in the RebalancerConfig call so YAML-only address signers are retained.typescript/rebalancer/src/config/types.ts (1)
137-145:⚠️ Potential issue | 🟠 Major
inventorySignersshould usez.partialRecord(...).Looks like the old exhaustiveness snag is still here: on Zod 4,
z.record(z.nativeEnum(...), ...)requires everyProtocolTypekey, so a config with onlyethereumor onlysealevelsigner data gets rejected even though the TS type isPartial<Record<ProtocolType, ...>>.Patch
inventorySigners: z - .record( + .partialRecord( z.nativeEnum(ProtocolType), z.union([ z.object({ address: z.string(), key: z.string().optional() }), z.string().transform((address) => ({ address })), ]), )In Zod 4, does `z.record(z.nativeEnum(MyEnum), valueSchema)` require every enum member key to be present, and should `Partial<Record<MyEnum, T>>` use `z.partialRecord(...)` instead?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/config/types.ts` around lines 137 - 145, The schema inventorySigners currently uses z.record(z.nativeEnum(ProtocolType), ...) which on Zod v4 enforces all ProtocolType keys; replace it with z.partialRecord(z.nativeEnum(ProtocolType), z.union([z.object({ address: z.string(), key: z.string().optional() }), z.string().transform((address) => ({ address }))])) so keys are optional per the TS type (you can keep .optional() on the whole field if you still want the entire map to be optional); update the schema definition referencing inventorySigners and ProtocolType to use z.partialRecord instead of z.record.
🧹 Nitpick comments (4)
typescript/rebalancer/src/utils/bridgeUtils.test.ts (1)
122-171: These two tests are doin' the same thing, like two donkeys pullin' the same cart.Test at line 122 ("should apply override with executionType: inventory and externalBridge") and test at line 147 ("should allow override to change executionType from movableCollateral to inventory") have identical config structures and assertions. Both verify that an inventory override with
ExternalBridgeType.LiFiwins over a movableCollateral base. Consider consolidating into one test or differentiate their intent.♻️ Suggested consolidation
- it('should apply override with executionType: inventory and externalBridge', () => { - const bridges: ChainMap<BridgeConfigWithOverride> = { - chain1: { - executionType: ExecutionType.MovableCollateral, - bridge: '0x1234567890123456789012345678901234567890', - override: { - chain2: { - executionType: ExecutionType.Inventory, - externalBridge: ExternalBridgeType.LiFi, - }, - }, - }, - chain2: { - executionType: ExecutionType.MovableCollateral, - bridge: '0x0987654321098765432109876543210987654321', - }, - }; - - const result = getBridgeConfig(bridges, 'chain1', 'chain2'); - - expect(result.executionType).to.equal(ExecutionType.Inventory); - assert(isInventoryConfig(result)); - expect(result.externalBridge).to.equal(ExternalBridgeType.LiFi); - }); - - it('should allow override to change executionType from movableCollateral to inventory', () => { + it('should allow override to change executionType from movableCollateral to inventory', () => { const bridges: ChainMap<BridgeConfigWithOverride> = { chain1: { executionType: ExecutionType.MovableCollateral, bridge: '0x1234567890123456789012345678901234567890', override: { chain2: { executionType: ExecutionType.Inventory, externalBridge: ExternalBridgeType.LiFi, }, }, }, chain2: { executionType: ExecutionType.MovableCollateral, bridge: '0x0987654321098765432109876543210987654321', }, }; const result = getBridgeConfig(bridges, 'chain1', 'chain2'); - // Override should win: executionType should be inventory, not movableCollateral + // Override wins: executionType becomes inventory with externalBridge expect(result.executionType).to.equal(ExecutionType.Inventory); assert(isInventoryConfig(result)); expect(result.externalBridge).to.equal(ExternalBridgeType.LiFi); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/bridgeUtils.test.ts` around lines 122 - 171, Two tests in bridgeUtils.test.ts duplicate the same scenario: both use the same bridges fixture and assertions that getBridgeConfig returns ExecutionType.Inventory with ExternalBridgeType.LiFi (they reference getBridgeConfig, ExecutionType.Inventory, ExternalBridgeType.LiFi, and isInventoryConfig). Fix by consolidating them into a single test (keep one of the it(...) blocks and remove the other) or change the second test to cover a different intent (for example assert that an override to a different chain without an override keeps MovableCollateral, or test a different externalBridge/invalid override path) so the two cases are distinct; update the test name accordingly and keep references to getBridgeConfig, isInventoryConfig, and the ExecutionType/ExternalBridgeType constants.typescript/infra/config/environments/mainnet3/rebalancer/USDC/eclipsemainnet-config.yaml (1)
18-21: All these override blocks look the same, don't they?Every EVM chain has the identical
override.solanamainnetblock. Now, I ain't sayin' it's wrong — it works just fine — but if this config grows, you might consider YAML anchors to keep your swamp tidy. Something like:# Define once at the top _solanamainnet_override: &solanamainnet_override solanamainnet: executionType: inventory externalBridge: lifi # Then reference it override: <<: *solanamainnet_overrideNot urgent, just a thought for when more chains show up at the door.
Also applies to: 30-33, 42-45, 54-57, 66-69, 78-81, 90-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/infra/config/environments/mainnet3/rebalancer/USDC/eclipsemainnet-config.yaml` around lines 18 - 21, Several override.blocks repeat the same override.solanamainnet content; consolidate by defining a YAML anchor (e.g., _solanamainnet_override with &solanamainnet_override) that contains solanamainnet: executionType: inventory and externalBridge: lifi, and replace each duplicate override block with a merge/reference (<<: *solanamainnet_override) so all occurrences reuse the single anchored definition; update every duplicate override (the ones at the ranges mentioned) to reference that anchor.typescript/rebalancer/src/utils/tokenUtils.ts (1)
22-35: Add a direct Sealevel regression test.This changes the native path for
SealevelHypNative, but there’s no visible test here proving it stays native whileSealevelHypCollateraldoes not. One small unit test aroundisNativeTokenStandard()andgetExternalBridgeTokenAddress()would pin this down. As per coding guidelines "Include tests for new functionality, especially edge cases".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/utils/tokenUtils.ts` around lines 22 - 35, Add unit tests that assert SealevelHypNative remains treated as native and SealevelHypCollateral does not: write tests targeting isNativeTokenStandard(standard) using the REBALANCER_NATIVE_STANDARDS/PROTOCOL_TO_HYP_NATIVE_STANDARD mapping to verify that TokenStandard.SealevelHypNative returns true and TokenStandard.SealevelHypCollateral returns false, and add a complementary test for getExternalBridgeTokenAddress() to ensure it returns the expected external address (or undefined/empty) for the same two Sealevel standards; reference the functions isNativeTokenStandard and getExternalBridgeTokenAddress to locate where to add these tests.typescript/rebalancer/src/monitor/Monitor.ts (1)
269-270: Drop the non-null assertion here.The guard at line 254 already narrows
this.inventoryConfig, sothis.inventoryConfig!is just dead weight and breaks the repo’s TS rule. Cache it after the guard and readinventoryAddressesfrom that.Possible fix
private async fetchInventoryBalances(): Promise<ChainMap<bigint>> { if (!this.inventoryConfig) return {}; + const { inventoryAddresses } = this.inventoryConfig; const balances: ChainMap<bigint> = {}; @@ - const address = - this.inventoryConfig!.inventoryAddresses[token.protocol]; + const address = inventoryAddresses[token.protocol];As per coding guidelines, "Do not use non-null assertions (
!) in TypeScript; use proper null checks (if (x != null),??, optional chaining)`."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/monitor/Monitor.ts` around lines 269 - 270, The code uses a non-null assertion on this.inventoryConfig when deriving address; instead, after the existing guard that narrows this.inventoryConfig (referenced in Monitor.ts around the check at line 254), cache the narrowed value into a local const (e.g., const inventoryConfig = this.inventoryConfig) and then read inventoryConfig.inventoryAddresses[token.protocol] into address without using `!`; this removes the dead non-null assertion and satisfies the TS rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@typescript/rebalancer/src/bridges/LiFiBridge.ts`:
- Around line 535-556: The updateRouteHook and later fallback logic only inspect
steps[0], which misses cases where the bridge step is later in
RouteExtended.steps; update the logic in executeRoute's updateRouteHook (and the
subsequent txHash fallback and transferId extraction code paths) to iterate all
RouteExtended.steps, deterministically select the cross-chain/bridge step (the
step whose type/action/tool indicates the bridge/cross-chain transfer), then
read execution.process entries from that selected LiFi step to extract txHash
and transferId; ensure you reference RouteExtended.steps[],
LiFiStepExtended.execution.process, txHash, transferId, and the
executeRoute/updateRouteHook handlers when applying the fix.
- Around line 130-132: The instance field _executeLock on LiFiBridge currently
serializes only a single instance, but lifiConfig.setProviders mutates a
process-global SDK singleton so concurrent LiFiBridge.executeRoute calls can
race and cause the wrong signer; make the lock process-wide by moving the
Promise<void> lock out of the class into module scope (e.g., a top-level
executeLock = Promise.resolve()) and replace usages of this._executeLock with
the module-level lock, ensuring the critical section that calls
lifiConfig.setProviders and related provider setup in executeRoute is awaited
under that lock; alternatively, refactor executeRoute to avoid calling
lifiConfig.setProviders per execution and instead compute or clone a provider
configuration without mutating SDK-global state (references: LiFiBridge,
_executeLock, executeRoute, lifiConfig.setProviders).
In `@typescript/rebalancer/src/core/InventoryRebalancer.test.ts`:
- Around line 145-155: The test stub currently returns ProtocolType.Ethereum and
an EthersV5 provider so the SOLANA_CHAIN path never exercises the new Sealevel
flow; update the stubs so warpCore.multiProvider.getChainMetadata() returns
ProtocolType.Sealevel for SOLANA_CHAIN, change the fake transfer transaction
type from ProviderType.EthersV5 to ProviderType.SolanaWeb3, and add a stub for
getSolanaWeb3Provider() that returns a mock Solana web3 provider/receipt
suitable for the Sealevel signer/receipt flow (replace the getEthersV5Provider()
usage in that test). Also mirror the same changes in the other test block around
lines 345-375 to cover the added edge case.
In `@typescript/rebalancer/src/factories/RebalancerContextFactory.ts`:
- Around line 460-494: The startup currently only asserts that inventory
keys/addresses exist but doesn’t reject unsupported protocols; update
RebalancerContextFactory where requiredProtocols is computed to assert that
every protocol in requiredProtocols is in the supported set (e.g., only
'ethereum' and 'sealevel'), and fail with a clear message naming the unsupported
protocol and affected chains (use requiredProtocols, chainsForProtocol for
context). Use assert() to enforce this invariant before proceeding to
key/address checks so configs for unsupported protocols (which later break in
InventoryRebalancer.buildSignerAccountConfig() / getTransactionReceipt()) are
rejected at startup.
In `@typescript/rebalancer/src/utils/tokenUtils.ts`:
- Around line 14-25: Update the stale JSDoc for getExternalBridgeTokenAddress to
remove EVM-specific wording and reflect that REBALANCER_NATIVE_STANDARDS now
includes both Ethereum and Sealevel; change bullets that mention only
EvmHypNative/EvmHypCollateral to a generalized description (e.g.,
"native/hyp-collateral standards for supported protocols such as Ethereum and
Sealevel" or "protocol-agnostic native and collateral standards used by the
rebalancer") and ensure any examples or behavior notes reference
REBALANCER_NATIVE_STANDARDS and ProtocolType.Sealevel as well as
ProtocolType.Ethereum so the doc matches the actual helper logic.
---
Duplicate comments:
In `@typescript/rebalancer/src/config/types.ts`:
- Around line 137-145: The schema inventorySigners currently uses
z.record(z.nativeEnum(ProtocolType), ...) which on Zod v4 enforces all
ProtocolType keys; replace it with z.partialRecord(z.nativeEnum(ProtocolType),
z.union([z.object({ address: z.string(), key: z.string().optional() }),
z.string().transform((address) => ({ address }))])) so keys are optional per the
TS type (you can keep .optional() on the whole field if you still want the
entire map to be optional); update the schema definition referencing
inventorySigners and ProtocolType to use z.partialRecord instead of z.record.
In `@typescript/rebalancer/src/service.ts`:
- Around line 222-232: The current merge creates a new RebalancerConfig using
only inventorySigners which replaces and drops address-only signers from the
original rebalancerConfig; instead, build a merged signers map that preserves
existing rebalancerConfig's signers and overlays inventorySigners (inventory
entries override same-key entries but do not remove other protocol/address-only
entries) and pass that merged map into the new RebalancerConfig constructor;
update the creation of mergedRebalancerConfig to compute mergedSigners =
merge(rebalancerConfig.signers, inventorySigners) and use mergedSigners in the
RebalancerConfig call so YAML-only address signers are retained.
---
Nitpick comments:
In
`@typescript/infra/config/environments/mainnet3/rebalancer/USDC/eclipsemainnet-config.yaml`:
- Around line 18-21: Several override.blocks repeat the same
override.solanamainnet content; consolidate by defining a YAML anchor (e.g.,
_solanamainnet_override with &solanamainnet_override) that contains
solanamainnet: executionType: inventory and externalBridge: lifi, and replace
each duplicate override block with a merge/reference (<<:
*solanamainnet_override) so all occurrences reuse the single anchored
definition; update every duplicate override (the ones at the ranges mentioned)
to reference that anchor.
In `@typescript/rebalancer/src/monitor/Monitor.ts`:
- Around line 269-270: The code uses a non-null assertion on
this.inventoryConfig when deriving address; instead, after the existing guard
that narrows this.inventoryConfig (referenced in Monitor.ts around the check at
line 254), cache the narrowed value into a local const (e.g., const
inventoryConfig = this.inventoryConfig) and then read
inventoryConfig.inventoryAddresses[token.protocol] into address without using
`!`; this removes the dead non-null assertion and satisfies the TS rule.
In `@typescript/rebalancer/src/utils/bridgeUtils.test.ts`:
- Around line 122-171: Two tests in bridgeUtils.test.ts duplicate the same
scenario: both use the same bridges fixture and assertions that getBridgeConfig
returns ExecutionType.Inventory with ExternalBridgeType.LiFi (they reference
getBridgeConfig, ExecutionType.Inventory, ExternalBridgeType.LiFi, and
isInventoryConfig). Fix by consolidating them into a single test (keep one of
the it(...) blocks and remove the other) or change the second test to cover a
different intent (for example assert that an override to a different chain
without an override keeps MovableCollateral, or test a different
externalBridge/invalid override path) so the two cases are distinct; update the
test name accordingly and keep references to getBridgeConfig, isInventoryConfig,
and the ExecutionType/ExternalBridgeType constants.
In `@typescript/rebalancer/src/utils/tokenUtils.ts`:
- Around line 22-35: Add unit tests that assert SealevelHypNative remains
treated as native and SealevelHypCollateral does not: write tests targeting
isNativeTokenStandard(standard) using the
REBALANCER_NATIVE_STANDARDS/PROTOCOL_TO_HYP_NATIVE_STANDARD mapping to verify
that TokenStandard.SealevelHypNative returns true and
TokenStandard.SealevelHypCollateral returns false, and add a complementary test
for getExternalBridgeTokenAddress() to ensure it returns the expected external
address (or undefined/empty) for the same two Sealevel standards; reference the
functions isNativeTokenStandard and getExternalBridgeTokenAddress to locate
where to add these tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d82c25a5-b215-46cc-8ccf-4a7c2f420d8e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (42)
.changeset/mixed-rebalancer-mode.md.changeset/sdk-solana-extra-signers.mdtypescript/cli/src/commands/warp.tstypescript/infra/config/environments/mainnet3/rebalancer/USDC/eclipsemainnet-config.yamltypescript/rebalancer-sim/src/runners/ProductionRebalancerRunner.tstypescript/rebalancer/package.jsontypescript/rebalancer/src/bridges/LiFiBridge.test.tstypescript/rebalancer/src/bridges/LiFiBridge.tstypescript/rebalancer/src/config/RebalancerConfig.test.tstypescript/rebalancer/src/config/RebalancerConfig.tstypescript/rebalancer/src/config/types.tstypescript/rebalancer/src/core/InventoryRebalancer.test.tstypescript/rebalancer/src/core/InventoryRebalancer.tstypescript/rebalancer/src/core/RebalancerService.test.tstypescript/rebalancer/src/core/RebalancerService.tstypescript/rebalancer/src/e2e/fixtures/routes.tstypescript/rebalancer/src/e2e/harness/MixedLocalDeploymentManager.tstypescript/rebalancer/src/e2e/harness/MockExternalBridge.tstypescript/rebalancer/src/e2e/harness/TestRebalancer.tstypescript/rebalancer/src/e2e/mixed-weighted.e2e-test.tstypescript/rebalancer/src/factories/RebalancerContextFactory.test.tstypescript/rebalancer/src/factories/RebalancerContextFactory.tstypescript/rebalancer/src/interfaces/IExternalBridge.tstypescript/rebalancer/src/monitor/Monitor.tstypescript/rebalancer/src/service.tstypescript/rebalancer/src/test/lifiMocks.tstypescript/rebalancer/src/tracking/ActionTracker.tstypescript/rebalancer/src/utils/ExplorerClient.tstypescript/rebalancer/src/utils/blockTag.tstypescript/rebalancer/src/utils/bridgeUtils.test.tstypescript/rebalancer/src/utils/gasEstimation.tstypescript/rebalancer/src/utils/solanaKeyParser.test.tstypescript/rebalancer/src/utils/solanaKeyParser.tstypescript/rebalancer/src/utils/tokenUtils.tstypescript/rebalancer/src/utils/transactionUtils.tstypescript/sdk/src/providers/ProviderType.tstypescript/sdk/src/signers/svm/solana-web3js.test.tstypescript/sdk/src/signers/svm/solana-web3js.tstypescript/sdk/src/token/adapters/ITokenAdapter.tstypescript/sdk/src/token/adapters/SealevelTokenAdapter.tstypescript/sdk/src/warp/WarpCore.test.tstypescript/sdk/src/warp/WarpCore.ts
💤 Files with no reviewable changes (3)
- typescript/rebalancer/src/core/RebalancerService.test.ts
- typescript/rebalancer-sim/src/runners/ProductionRebalancerRunner.ts
- typescript/cli/src/commands/warp.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- typescript/rebalancer/src/e2e/fixtures/routes.ts
- typescript/rebalancer/src/utils/blockTag.ts
- typescript/rebalancer/src/e2e/harness/MixedLocalDeploymentManager.ts
- typescript/rebalancer/src/test/lifiMocks.ts
- typescript/sdk/src/warp/WarpCore.ts
- typescript/rebalancer/src/utils/solanaKeyParser.ts
- typescript/rebalancer/src/tracking/ActionTracker.ts
- typescript/sdk/src/token/adapters/SealevelTokenAdapter.ts
- typescript/sdk/src/warp/WarpCore.test.ts
- .changeset/mixed-rebalancer-mode.md
- typescript/rebalancer/src/utils/solanaKeyParser.test.ts
- typescript/rebalancer/src/config/RebalancerConfig.ts
- .changeset/sdk-solana-extra-signers.md
- typescript/rebalancer/src/interfaces/IExternalBridge.ts
- typescript/sdk/src/providers/ProviderType.ts
- typescript/sdk/src/token/adapters/ITokenAdapter.ts
paulbalaji
reviewed
Mar 10, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Review Summary
3 findings. Accounted for all existing discussion — Paul's chain-ID mapping / monitor-chain / DRY comments are fixed in head, tkporter's as any concern is fixed by putting extraSigners on SolanaWeb3Transaction. Did not re-raise CodeRabbit's z.partialRecord() thread (repo pins Zod 3, so the Zod 4 exhaustiveness warning is a false positive). Did not re-raise the Ethereum-only ActionTracker exclusion — ExplorerClient is hex/bytea-only, so Sealevel addresses would be wrong there.
Review is from diff/code inspection against PR head 34956cb.
| # | Severity | File | Finding |
|---|---|---|---|
| 1 | Major | LiFiBridge.ts |
steps[0] extraction records wrong tx hash for multi-step routes |
| 2 | Major | InventoryRebalancer.test.ts, mixed-weighted.e2e-test.ts |
Sealevel path is effectively untested |
| 3 | Medium | RebalancerContextFactory.ts |
Unsupported inventory protocols not rejected at startup |
See inline comments for details.
…artup Add startup assertion in RebalancerContextFactory that validates inventory protocols are either Ethereum or Sealevel. Unsupported protocols (e.g., Cosmos) are rejected with a clear error message listing affected chains. Includes corresponding test case verifying Cosmos protocol rejection.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
typescript/rebalancer/src/bridges/LiFiBridge.ts (2)
537-552:⚠️ Potential issue | 🟠 MajorDon't pull bridge tracking data from
steps[0]only.LiFi routes can be swap → bridge → swap. Reading
txHashandtransferIdfrom the first step can latch onto a swap hash, thengetStatus()polls the wrong transfer. Walk the executed steps and pick the actual cross-chain step instead.Can `@lifi/sdk` `RouteExtended.steps` contain multiple steps, and which step's `process.txHash` should be used for bridge status tracking?Also applies to: 561-589
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/bridges/LiFiBridge.ts` around lines 537 - 552, The code currently reads txHash (and transferId elsewhere) only from updatedRoute.steps[0], which can be a swap step and cause tracking the wrong transaction; instead, iterate all updatedRoute.steps, inspect each step.execution.process entries, and select the process associated with the cross-chain/bridge step (e.g., where step.type indicates a bridge/transfer or where process contains bridge-specific fields such as txHash for transfer processes), assigning txHash/transferId from that process; update both the updateRouteHook implementation shown (function updateRouteHook) and the similar logic in the other block that handles status polling (the other updateRouteHook-like section) to walk all steps and pick the bridge step rather than assuming steps[0].
129-129:⚠️ Potential issue | 🟠 MajorMake the LiFi execution lock process-wide.
lifiConfig.setProviders()mutates SDK-global state, but_executeLockonly serializes oneLiFiBridgeinstance. Two bridge instances in the same process can still swap providers mid-flight and sign with the wrong wallet.Does `@lifi/sdk` `config.setProviders()` mutate a global singleton shared across `executeRoute()` calls?Also applies to: 202-253, 512-518
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/bridges/LiFiBridge.ts` at line 129, The instance-scoped Promise lock _executeLock in LiFiBridge allows concurrent LiFi SDK provider mutations across multiple bridge instances; replace it with a process-wide lock (e.g., a module-level/shared Mutex/Promise chain variable like LI_FI_EXECUTE_LOCK) and use that single lock wherever the class currently awaits _executeLock (notably around calls to lifiConfig.setProviders() and executeRoute-related logic in LiFiBridge). Ensure all places that previously referenced the instance field (_executeLock) now reference the shared lock so provider swapping and signing are serialized across the entire process.typescript/rebalancer/src/core/InventoryRebalancer.test.ts (1)
368-377:⚠️ Potential issue | 🟠 MajorThis still ducks the Sealevel signer flow.
Stubbing
sendAndConfirmInventoryTx()means this test never touchesbuildSignerAccountConfig(),getSignerForChain(), receipt parsing, or the Solana provider stub below. Right now it only proves the branch picks that method, not that the new IMultiProtocolSigner/Solana path actually works.As per coding guidelines, Include tests for new functionality, especially edge cases.
Also applies to: 380-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/core/InventoryRebalancer.test.ts` around lines 368 - 377, The test currently stubs InventoryRebalancer.prototype.sendAndConfirmInventoryTx which bypasses the Sealevel signer flow; remove that stub and let sendAndConfirmInventoryTx execute so the test covers buildSignerAccountConfig(), getSignerForChain(), IMultiProtocolSigner/Solana handling and receipt parsing. Instead, stub lower-level dependencies: mock getSignerForChain to return a fake IMultiProtocolSigner that implements the Solana signing methods, ensure buildSignerAccountConfig (if used) returns the expected config, and keep the warpCore.multiProvider.getSolanaWeb3Provider stub that returns getTransaction with meta.logMessages so receipt parsing executes; do not stub sendAndConfirmInventoryTx itself so the full Solana path is exercised.
🧹 Nitpick comments (1)
typescript/rebalancer/src/bridges/LiFiBridge.ts (1)
493-496: Don't fall back to Ethereum when protocol lookup misses.If
getProtocolTypeForChainId()returnsundefined, this line validates the Ethereum key and keeps going with a masked metadata/mapping bug. AssertfromProtocolfirst, then readprivateKeys[fromProtocol].As per coding guidelines, Do not add defensive fallbacks that mask bugs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typescript/rebalancer/src/bridges/LiFiBridge.ts` around lines 493 - 496, The code currently falls back to ProtocolType.Ethereum when getProtocolTypeForChainId(fromChain) returns undefined, masking a metadata/mapping bug; change the logic in LiFiBridge around getProtocolTypeForChainId, i.e. assert that fromProtocol is defined first (e.g., assert(fromProtocol, `Unknown protocol for chain ${fromChain}`)), then access privateKeys[fromProtocol] and assert that privateKeys[fromProtocol] exists (with a clear message about missing private key), instead of using the fallback privateKeys[fromProtocol ?? ProtocolType.Ethereum].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@typescript/rebalancer/src/bridges/LiFiBridge.ts`:
- Around line 537-552: The code currently reads txHash (and transferId
elsewhere) only from updatedRoute.steps[0], which can be a swap step and cause
tracking the wrong transaction; instead, iterate all updatedRoute.steps, inspect
each step.execution.process entries, and select the process associated with the
cross-chain/bridge step (e.g., where step.type indicates a bridge/transfer or
where process contains bridge-specific fields such as txHash for transfer
processes), assigning txHash/transferId from that process; update both the
updateRouteHook implementation shown (function updateRouteHook) and the similar
logic in the other block that handles status polling (the other
updateRouteHook-like section) to walk all steps and pick the bridge step rather
than assuming steps[0].
- Line 129: The instance-scoped Promise lock _executeLock in LiFiBridge allows
concurrent LiFi SDK provider mutations across multiple bridge instances; replace
it with a process-wide lock (e.g., a module-level/shared Mutex/Promise chain
variable like LI_FI_EXECUTE_LOCK) and use that single lock wherever the class
currently awaits _executeLock (notably around calls to lifiConfig.setProviders()
and executeRoute-related logic in LiFiBridge). Ensure all places that previously
referenced the instance field (_executeLock) now reference the shared lock so
provider swapping and signing are serialized across the entire process.
In `@typescript/rebalancer/src/core/InventoryRebalancer.test.ts`:
- Around line 368-377: The test currently stubs
InventoryRebalancer.prototype.sendAndConfirmInventoryTx which bypasses the
Sealevel signer flow; remove that stub and let sendAndConfirmInventoryTx execute
so the test covers buildSignerAccountConfig(), getSignerForChain(),
IMultiProtocolSigner/Solana handling and receipt parsing. Instead, stub
lower-level dependencies: mock getSignerForChain to return a fake
IMultiProtocolSigner that implements the Solana signing methods, ensure
buildSignerAccountConfig (if used) returns the expected config, and keep the
warpCore.multiProvider.getSolanaWeb3Provider stub that returns getTransaction
with meta.logMessages so receipt parsing executes; do not stub
sendAndConfirmInventoryTx itself so the full Solana path is exercised.
---
Nitpick comments:
In `@typescript/rebalancer/src/bridges/LiFiBridge.ts`:
- Around line 493-496: The code currently falls back to ProtocolType.Ethereum
when getProtocolTypeForChainId(fromChain) returns undefined, masking a
metadata/mapping bug; change the logic in LiFiBridge around
getProtocolTypeForChainId, i.e. assert that fromProtocol is defined first (e.g.,
assert(fromProtocol, `Unknown protocol for chain ${fromChain}`)), then access
privateKeys[fromProtocol] and assert that privateKeys[fromProtocol] exists (with
a clear message about missing private key), instead of using the fallback
privateKeys[fromProtocol ?? ProtocolType.Ethereum].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8fcb268-6d92-4d9a-a609-0795e53e4b45
📒 Files selected for processing (6)
typescript/rebalancer/src/bridges/LiFiBridge.tstypescript/rebalancer/src/core/InventoryRebalancer.test.tstypescript/rebalancer/src/factories/RebalancerContextFactory.test.tstypescript/rebalancer/src/factories/RebalancerContextFactory.tstypescript/rebalancer/src/service.tstypescript/rebalancer/src/utils/tokenUtils.ts
paulbalaji
reviewed
Mar 10, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
Follow-up review (22e9684)
6 new commits since last review. Re-assessed all 3 findings.
Finding #3 (Medium: unsupported protocols) — Resolved ✓
SUPPORTED_INVENTORY_PROTOCOLS set + assert in 41f6b4a1. Test added. Clean.
Finding #2 (Major: Sealevel untested) — Partially resolved
Test in 92350ec2 now correctly stubs SOLANA_CHAIN as ProtocolType.Sealevel, returns ProviderType.SolanaWeb3 txs, and asserts routing to sendAndConfirmInventoryTx instead of multiProvider.sendTransaction. This verifies the dispatch routing, which was the worst gap.
The internal Sealevel logic (buildSignerAccountConfig, getTransactionReceipt, parseMessageDispatchLogs) is still stubbed out, and the e2e remains EVM-only — but deferring cross-VM e2e to a follow-up is reasonable given the Solana validator harness requirement.
Finding #1 (Major: steps[0]) — Accepted
Author's response is that the rebalancer only requests same-token bridge quotes (no swap legs), so steps[0] is always the bridge step. Verified: getQuote params use the same token on both sides, so LiFi returns a single bridge step. The assumption holds for current usage. Not a bug.
Other fixes in this batch
09bd933— consolidated duplicate@hyperlane-xyz/utilsimport ✓c1763fd— updated stale JSDoc in tokenUtils ✓a8a27ab— service.ts now merges runtime keys with YAML config instead of replacing ✓22e9684— removed useless fallback in inventorySigners spread ✓
No new findings. The outstanding items (cross-VM e2e, Keypair in ITokenAdapter) are acknowledged deferrals for follow-up PRs.
Mo-Hussain
commented
Mar 10, 2026
Collaborator
Author
Mo-Hussain
left a comment
Mo-Hussain
left a comment
There was a problem hiding this comment.
The internal Sealevel logic (
buildSignerAccountConfig,getTransactionReceipt,parseMessageDispatchLogs) is still stubbed out, and the e2e remains EVM-only — but deferring cross-VM e2e to a follow-up is reasonable given the Solana validator harness requirement.
Cross-VM e2e is in progress in #8311.
paulbalaji
approved these changes
Mar 10, 2026
Collaborator
paulbalaji
left a comment
paulbalaji
left a comment
There was a problem hiding this comment.
All findings from our review are resolved or accepted. Cross-VM e2e and Keypair in ITokenAdapter are acknowledged deferrals.
Contributor
⚙️ Node Service Docker Images Built Successfully
Full image paths |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds mixed-mode rebalancing that enables simultaneous movable collateral (EVM) and inventory (multi-VM) execution within a single rebalancer configuration. This is the core enabler for cross-protocol rebalancing across EVM and Sealevel chains.
Rebalancer — Mixed Mode & Multi-VM Support
inventorySignerreplaced withinventorySigners: Partial<Record<ProtocolType, string>>map, supporting EVM + Sealevel keys simultaneouslytransferRemoteuses WarpCoregetTransferRemoteTxs()for multi-VM compatibility, protocol-aware receipt parsing for message ID extraction, andSealevelCoreAdapter.parseMessageDispatchLogsfor SealevelKeypairWalletAdapter, Hyperlane domain ID → LiFi chain ID translation, and mutex around configure+execute to prevent race conditionsparseSolanaPrivateKey()utility with strict 64-byte validation, base58 normalization for LiFi adapterSDK Changes
extraSignersfor Solana transferRemote: AddedextraSigners?: Keypair[]toSolanaWeb3TransactionandTransferRemoteParams, enabling proper keypair threading through the typed transaction pipeline instead of__hyperlaneExtraSignershackKeypairfor SolanaWeb3 transfers;SealevelHypTokenAdapterconsumes itKeypairSvmTransactionSigner.signTransactionusespartialSignto preserve extra signer signatures across blockhash resubmitsCLI & Infra
inventoryMultiProviderarg fromRebalancerServiceconstructor in CLI and rebalancer-simTests
mixed-weighted.e2e-test.ts) withMixedLocalDeploymentManagerharnessInventoryRebalancer,RebalancerService, andRebalancerContextFactoryas anycastsSummary by CodeRabbit
New Features
Bug Fixes / Validation
Tests