fix(sdk-coin-polyx): route v8 batchAll(bond,nominate) parse to v8 builder

[abhijit0943]

What

TransactionBuilderFactory.getBuilder() routed every batchAll([bond, nominate]) to the v7 BatchStakingBuilder on the decode-success path. When the transaction is decoded against v8 material, the bond call carries no controller (v8 changed bond(controller, value, payee) → bond(value, payee)), so v7 validateBondArgs crashed with Cannot read properties of undefined (reading 'id') while accessing args.controller.id.

Why

tryGetV8Builder() only runs when v7 decode() throws. Unlike v8 transfers (whose shifted call indices make v7 decode fail), v8 staking batchAll can decode without throwing, so the v8 fallback never fired and the tx was misrouted — breaking POLYX v8 stake dry-runs on staging.

How

In the batchAll([bond, nominate]) branch, inspect the decoded bond args: route to V8BatchStakingBuilder when no controller is present, and keep v7 bond+nominate on BatchStakingBuilder. Scoped to the factory only — no builder implementation changes.

Testing

• New test reproduces the staging incident: with v8 material set, factory.from() on a v8 batchAll([bond, nominate]) returns V8BatchStakingBuilder instead of crashing.
• New regression test confirms a v7 bond+nominate tx still routes to BatchStakingBuilder.
• yarn tsc, lint, and the polyx staking test files pass.

Linked to SI-981.

[linear-code]

SI-981

[abhijit0943]

@claude review

[github-actions]

Claude finished @abhijit0943's task —— View job

---

Code Review for PR #9220 ✅

Reviewing the fix for v8 batchAll(bond,nominate) routing in POLYX SDK.

Todo List:

• Read and analyze the factory changes in transactionBuilderFactory.ts
• Review the test additions in batchStakingBuilder.ts
• Review the test additions in v8BatchStakingBuilder.ts
• Analyze the overall fix approach and potential edge cases
• Provide comprehensive review feedback

Code Review Analysis

✅ Overall Assessment: LGTM - Well-designed fix

The PR addresses a specific routing bug where v8 batchAll([bond, nominate]) transactions were incorrectly routed to v7 builders, causing crashes due to the missing controller parameter in v8 bond calls.

📝 Core Changes Review

1. Factory Logic Enhancement (transactionBuilderFactory.ts:223-233)

const bondArgs = args.calls[0].args as { controller?: unknown };
if (bondArgs.controller === undefined) {
  return this.getV8BatchStakingBuilder();
}

✅ Strengths:

• Precise detection: Uses presence/absence of controller parameter to distinguish v7 vs v8 bond calls
• Scoped fix: Only affects the specific bond + nominate pattern, no broader impact
• Well-documented: Excellent inline comments explaining the v8 API change and routing logic
• Maintains backwards compatibility: v7 transactions with controller still route correctly

✅ Type Safety: The type assertion as { controller?: unknown } is safe since args.calls[0].args is already validated as existing bond call arguments.

🧪 Test Coverage Review

2. v7 Regression Test (batchStakingBuilder.ts)

it('factory.from routes a v7 bond+nominate transaction to BatchStakingBuilder (SI-981 regression)', ...)

✅ Good coverage: Ensures v7 transactions with controller still route to the correct v7 builder

3. v8 Routing Test (v8BatchStakingBuilder.ts)

it('factory.from routes to V8BatchStakingBuilder on the v8 decode-success path (SI-981)', ...)

✅ Reproduces the bug: Test directly addresses the original staging incident with v8 material
✅ Clear test setup: Uses v8Material and v8Factory to simulate the exact conditions

🔍 Technical Analysis

Problem Understanding:

• v8 changed bond(controller, value, payee) → bond(value, payee)
• v8 transactions can decode successfully with v7 material (unlike transfers)
• This caused v8 bond calls to be routed to v7 builders which expect controller.id

Solution Approach:

• Detect v8 bond calls by checking for missing controller parameter
• Route to appropriate builder based on parameter presence
• Maintains existing fallback logic for other transaction types

⚠️ Minor Considerations

1. Edge Case: The fix assumes that any bond call without controller is v8. While this is currently true, future versions could potentially introduce other bond variations.

2. Error Handling: The existing try-catch structure is maintained, ensuring that if the v8 detection logic fails, the original error path is preserved.

📊 Code Quality

✅ Documentation: Excellent inline comments explaining the API change and routing decision
✅ Maintainability: Clear separation of v7/v8 logic with minimal code duplication
✅ Performance: Minimal overhead - only adds a property check for bond+nominate patterns
✅ Testability: Both positive and negative test cases provided

🎯 Recommendation: APPROVE

This is a well-crafted, surgical fix that:

• Addresses the specific routing bug without broader architectural changes
• Maintains backward compatibility with v7 transactions
• Includes comprehensive test coverage for both v7 and v8 scenarios
• Has clear documentation explaining the reasoning
• Follows the existing factory pattern consistently

The fix is production-ready and should resolve the staging dry-run failures mentioned in SI-981.

---

Job: https://github.com/BitGo/BitGoJS/actions/runs/11622253873
Branch: abhijitmadhusudan496/si-981-fix-sdk-factory-routing-for-v8-batchallbond-nominate-6963