Skip to content

feat(sdk-coin-polyx): add v8 staking builders#9164

Merged
abhijit0943 merged 1 commit into
masterfrom
abhijitmadhusudan496/si-915-staking-4-bitgojs-v8-staking-builders-83cd
Jul 2, 2026
Merged

feat(sdk-coin-polyx): add v8 staking builders#9164
abhijit0943 merged 1 commit into
masterfrom
abhijitmadhusudan496/si-915-staking-4-bitgojs-v8-staking-builders-83cd

Conversation

@abhijit0943

Copy link
Copy Markdown
Contributor

Summary

Polymesh v8 changed staking.bond from bond(controller, value, payee) to bond(value, payee) (the stash is its own controller). The existing BatchStakingBuilder always passes controller and uses v7 metadata, so a v8 new-stake (utility.batchAll([bond, nominate])) encodes the wrong extrinsic.

This adds V8BatchStakingBuilder — building batchAll([bond, nominate]) with the 2-arg v8 bond against v8 chain metadata — plus thin v8 metadata wrappers for the staking operations whose call shape is unchanged. The v7 builders are kept untouched as the Flipt rollback path and marked [CLEANUP-V8-OLD].

Changes

  • V8BatchStakingBuilderbond({ value, payee }) (no controller) inside batchAll([bond, nominate]), v8 material.
  • Thin v8 wrappers (v8 material only, logic identical to v7): V8BondExtraBuilder, V8UnbondBuilder, V8BatchUnstakingBuilder, V8WithdrawUnbondedBuilder, V8NominateBuilder.
  • iface.ts — new V8BondArgs { value, payee }; BondArgs kept and marked [CLEANUP-V8-OLD].
  • txnSchema.tsV8BatchTransactionSchema (bond/batch validation without controller).
  • FactorygetV8*() methods for each new builder; from() now routes v8-encoded staking transactions (which cannot be decoded with v7 material) to the matching v8 builder.
  • [CLEANUP-V8-OLD] comments on the v7 staking builders.

Testing

  • New v8BatchStakingBuilder.ts unit tests: v8 material loads (specVersion 8000000), controller setter is absent, both-leg validation, decode/validate round-trip (bond has value only, no controller), from() routing, and smoke builds for each wrapper.
  • The built batchAll method hex is asserted byte-for-byte equal to the testnet sandbox tx (block 24829135) built with the same params.
  • Existing v7 BatchStakingBuilder tests unchanged and still pass. yarn lint / yarn build clean. (The pre-existing Polyx: suite failure is an unrelated BITGOJS_TEST_PASSWORD env requirement.)

@linear-code

linear-code Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

SI-915

Polymesh v8 changed staking.bond from bond(controller, value, payee) to
bond(value, payee) — the stash is its own controller. Add V8BatchStakingBuilder
that builds utility.batchAll([bond, nominate]) with the 2-arg v8 bond against
v8 chain metadata, plus thin v8 metadata wrappers for bondExtra, unbond, batch
unstaking, withdrawUnbonded, and nominate (logic unchanged, v8 material only).

The v7 BatchStakingBuilder and sibling builders are retained as the Flipt
rollback path and marked [CLEANUP-V8-OLD]. The factory exposes getV8* methods
and from() now routes v8-encoded staking transactions (which cannot be decoded
with v7 material) to the matching v8 builder.

Verified the built batchAll method hex is byte-for-byte identical to the
testnet sandbox tx (block 24829135) and decodes to bond({ value, payee }) with
no controller at specVersion 8000000.

Ticket: SI-915
@abhijit0943 abhijit0943 force-pushed the abhijitmadhusudan496/si-915-staking-4-bitgojs-v8-staking-builders-83cd branch from 9e6697c to da400f4 Compare July 2, 2026 07:16
@abhijit0943

Copy link
Copy Markdown
Contributor Author

@claude review this PR

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @abhijit0943's task —— View job


PR Review: v8 Staking Builders for Polymesh

I've completed my review of this PR which adds v8 staking builders for Polymesh. Overall, this is a well-structured and thoroughly implemented change that properly handles the v7→v8 migration. Here are my findings:

Strengths

  1. Excellent Architecture & Design

    • Clean separation between v7 and v8 builders with proper inheritance
    • Thin v8 wrappers minimize code duplication while ensuring correct material usage
    • Smart factory routing with fallback error handling (tryGetV8Builder)
    • Proper preservation of v7 builders for rollback capability
  2. Comprehensive Implementation

    • Correctly handles the core v8 change: bond(controller, value, payee)bond(value, payee)
    • All validation schemas updated appropriately (V8BatchTransactionSchema, V8BondValidationObject)
    • Complete set of v8 builders covering all staking operations
    • Robust decode/encode handling for different data formats
  3. Excellent Test Coverage

    • Tests verify v8 material usage (specVersion: 8000000)
    • Validates absence of controller setter in v8 builders ✓
    • Round-trip testing ensures decode/validate/build cycle works
    • Byte-for-byte verification against real testnet transaction
    • Factory routing tests ensure correct builder selection
  4. Production Readiness

    • Clear [CLEANUP-V8-OLD] markers for future maintenance
    • Proper error handling and validation at all levels
    • Type safety maintained throughout

🔍 Technical Review Points

V8BatchStakingBuilder.ts:67-74 - The type assertion here is somewhat awkward:

const bondCall = methods.staking.bond(
  {
    value: this._amount,
    payee: this._payee || 'Staked',
  } as unknown as { controller: string; value: string; payee: string | { Account: string } },
  baseTxInfo.baseTxInfo,
  baseTxInfo.options
);

This works but suggests the underlying methods.staking.bond signature hasn't been updated for v8. Consider if the substrate txwrapper methods should be updated to support both signatures.

Factory Integration (transactionBuilderFactory.ts:178-184) - The v8 fallback logic is well-implemented. The approach of catching v7 decode errors and retrying with v8 material is pragmatic and handles the migration cleanly.

Test Quality - The tests are particularly strong. I especially appreciate:

  • Real testnet verification: Line 95 validates against actual on-chain transaction hex
  • Material validation: Lines 36-42 confirm v8 specVersion usage
  • Interface validation: Lines 61-65 verify controller methods are absent

📋 Minor Observations

  1. Consistency: All v8 wrapper files follow identical patterns (15 lines each), which is good for maintainability
  2. Documentation: JSDoc comments clearly explain the v7→v8 changes and rollback strategy
  3. Schema Validation: Proper separation of v7/v8 validation objects prevents cross-contamination

🎯 Deployment Considerations

The PR includes proper safeguards for production deployment:

  • V7 builders preserved for Flipt rollback capability
  • Factory gracefully handles both v7 and v8 encoded transactions
  • Clear separation allows for gradual migration

Final Assessment

This is an excellent implementation that:

  • ✅ Correctly handles the Polymesh v8 staking API changes
  • ✅ Maintains backward compatibility and rollback capability
  • ✅ Includes comprehensive tests with real-world validation
  • ✅ Follows good architectural patterns
  • ✅ Is ready for production deployment

Recommendation: APPROVE - This PR demonstrates thorough engineering practices and is ready to merge.


Job Run | Branch: abhijitmadhusudan496/si-915-staking-4-bitgojs-v8-staking-builders-83cd

@abhijit0943 abhijit0943 marked this pull request as ready for review July 2, 2026 07:20
@abhijit0943 abhijit0943 requested a review from a team as a code owner July 2, 2026 07:20

@Gautam2305 Gautam2305 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@abhijit0943 abhijit0943 merged commit 32e2d71 into master Jul 2, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants