[Doddanna17]

Context

To register on the mainnet dummy pox-5 (SP3JVSE4ZB5GMDJP5XBPX7ZB1BG3QDFW7RCEQJDMY.pox-5-dummy-v1), Wallet Platform builds the register-for-bond contract call through this SDK's contract builder — which hard-rejects any contract that is not the canonical pox-4 / pox-5 / send-many-memo. So the register transaction cannot be built at all: not on the staking-service intent path, not on /tx/build, and for any wallet type (custody/hot). Surfaced while reviewing BitGo/bitgo-microservices#63791 (SI-2104 / SI-2089).

Change

• New opt-in ContractBuilder.allowedStakingContracts([{ name, address }]): the caller supplies the staking contract it is configured with, and the builder accepts that name/address in addition to the canonical set.
• contractName / contractAddress accept the caller-supplied contract; functionName skips the canonical ABI check for it (its arguments are pre-built Clarity values supplied by the caller).
• Empty by default — no behaviour change unless a caller opts in. No @bitgo/statics change, and no per-contract hardcoding, so a future staking contract (or a test/dummy one) needs no SDK change — only a value passed by the caller.

Why it fails today

• contractAddress requires network.stakingContractAddress / sendmanymemoContractAddress / sbtcWithdrawalContractAddress → rejects SP3JVSE4ZB5GMDJP5XBPX7ZB1BG3QDFW7RCEQJDMY (Invalid contract address).
• contractName requires VALID_STAKING_CONTRACT_NAMES (pox-4, pox-5) or send-many-memo → rejects pox-5-dummy-v1 (Only pox-4, pox-5, and send-many-memo contracts supported).

Both throw InvalidParameterValueError inside buildContractCall, so Wallet Platform's build fails closed.

Tests

• Added to test/unit/transactionBuilder/contractBuilder.ts:
  • a caller-allowlisted non-canonical contract builds (pox-5-dummy-v1 / SP3JVSE…, register-for-bond);
  • a non-allowlisted contract name still throws;
  • a non-allowlisted contract address still throws.
• Unit tests require BITGOJS_TEST_PASSWORD (a CI secret) — they could not be run locally (the module's fixtures are encrypted); CI must validate.

Verification

Pending CI (unit tests).

Related

• SI-2106 (this) · SI-2104 (WP config) · BitGo/bitgo-microservices#63791 · SI-2089 (dummy prod testing) · SI-2098 (staking-service reads)
• Companion WP change (separate PR): call allowedStakingContracts(...) in buildContractCall with config.v2[chain].pox5StakingContractName/Address, and let verifyStackingContractCall accept the configured name.

[linear-code]

SI-2106

[github-actions]

⚠️ Unit tests are failing on Node 26.x (Current release line, non-blocking). This is not an LTS version yet, so it does not block merge, but it signals an incompatibility to fix before Node 26.x becomes LTS.

View run

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[shubham-damkondwar]

@claude review

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[Doddanna17]

Thanks — both findings addressed in ff423aef65.

🔴 Rehydration / signing (initBuilder) — fixed. initBuilder no longer re-runs the canonical-contract checks: super.initBuilder has already set the contract fields from the payload, so re-validating there only re-applied an empty allowlist and threw. factory.from(hex) (used by signTransaction, explainTransaction and the fee re-build) now re-parses an allowlisted contract cleanly. Added a round-trip test (build → factory.from(tx.toBroadcastFormat()) → asserts the contract address/name survive).

🟡 Name/address as a pair — fixed. The allowlist is now matched as a pair: allowedAddressForName / allowedNameForAddress mean an allowlisted name only passes with the address it was allowlisted with (and vice versa), so it can no longer be combined into an unconfigured contract principal (e.g. SP000…Q6VF78.pox-5-dummy-v1). The function-name ABI skip now applies only when the name is allowlisted (its address is enforced separately). Added a pair-mismatch test (allowlisted name + canonical address → rejected).

Net behaviour: canonical pox-4 / pox-5 / send-many-memo unchanged; a caller opting in gets the configured contract, with the pair enforced.

Note: the module's unit tests need BITGOJS_TEST_PASSWORD (CI secret) so they can't be run locally — CI will validate the added cases.

[Doddanna17]

Reworked to option A (thread the allowlist through the rehydration path) so validation is identical on every path — no relaxation. Current head f0168fd3a7.

What changed vs the previous push

• The allowlist is now threaded everywhere instead of skipped on rebuild:
  • ContractBuilder takes it via the constructor (allowedStakingContracts(...) setter kept for the build path);
  • TransactionBuilderFactory.from(hex, { allowedStakingContracts }) → getContractBuilder(tx, allowlist);
  • Stx.signTransaction / explainTransaction / the recovery rebuild pass the coin's setAllowedStakingContracts(...) value.
• initBuilder restores the original canonical checks — they now run on every path (build and rebuild). A non-allowlisted contract is still rejected on parse: the added test asserts factory.from(hex) without the allowlist throws.
• An allowlisted name is validated against the canonical pox-5 function set (the dummy is an ABI-identical copy), rather than skipping the check.

Net: canonical pox-4 / pox-5 / send-many-memo validation and flows are unchanged; the only addition is that an explicitly allowlisted (name, address) pair is accepted — and its values come from the caller's config, so a future contract needs no SDK change.

Standards: eslint --quiet clean (exit 0), prettier --check clean; no new eslint warnings (the 12 reported are pre-existing lines).

Tests: allowlisted build, allowlist-threaded round-trip, no-allowlist rebuild still throws, pair-mismatch rejected, non-pox-5 function rejected. (Unit tests need BITGOJS_TEST_PASSWORD — CI secret — so CI validates.)

[Doddanna17]

@claude, Kindly review and share the approval status as comment.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[Vijay-Jagannathan]

Threading the allowlist through factory.from(hex, { allowedStakingContracts }) fixes the rehydration/sign/explain problem from the earlier review, and the canonical path is unchanged. I'm requesting changes for one thing: the (name, address) pair is not enforced in both call orders, and the PR description and tests say it is. Details are inline.

Blocking:

• Pair enforcement depends on call order (contractBuilder.ts, inline).

Non-blocking:

• The PR description is stale. It still says functionName "skips the canonical ABI check" for the allowlisted contract, but the code now validates against VALID_POX5_CONTRACT_FUNCTION_NAMES. Please update it.
• The fee re-build in WP that calls factory.from(hex) needs to pass the allowlist too. That is outside this PR, but the companion WP PR should cover it.
• CI is the only validation for the new tests, since they need BITGOJS_TEST_PASSWORD. Please confirm the contractBuilder unit tests are green before merge.

[Doddanna17]

Addressed each finding in bdcec0d73a.

🔴 contractBuilder.ts — the pair check was order-dependent
Fixed. The pair is now validated on both setters once both fields are known (isContractPairValid()), so it no longer depends on call order, and an unset field defers to that check instead of passing:

• contractAddress() — validates the address, then if _contractName is set requires a valid pair;
• contractName() — validates the name, then if _contractAddress is set requires a valid pair.
  Both of your examples now throw (Contract name and address are not an allowlisted pair):
• allowlisted name → canonical address (the SP000…2Q6VF78.pox-5-dummy-v1 case);
• allowlisted address → canonical name.

🟡 contractBuilder.ts — unset treated as a match / first-entry lookup
Fixed. Removed the (!this._contractAddress || …) shortcut (the unset case now defers to the final pair check above), and allowedStakingContracts() rejects duplicate names (Duplicate staking contract name: …), so the "find returns only the first entry" ambiguity can't arise. Matching is on the full (name, address) pair.

🟡 stx.ts — shared/instance state
Fixed. Removed the instance field and setAllowedStakingContracts(). The allowlist is now an explicit option — StxSignTransactionOptions.allowedStakingContracts / ExplainTransactionOptions.allowedStakingContracts — passed straight into factory.from(hex, { allowedStakingContracts }). No shared state; the WP companion passes it per call. (Both option types are local to this module.)

🟡 test — avoid any / implementation-coupled assertions
Fixed. The round-trip test now asserts on the observable (await reparsed.build()).toJson().payload (contractAddress / contractName) instead of the private _contractAddress / _contractName, and the no-allowlist rebuild pins ParseTransactionError.

🔴 test — add the reversed cases
Fixed. Added the two reversed-order cases (allowlisted name → canonical address; allowlisted address → canonical name), both asserting a throw, plus a duplicate-name case.

Net: canonical pox-4 / pox-5 / send-many-memo validation and flows are unchanged; only an explicitly allowlisted (name, address) pair is accepted — order-independently, with no instance state. eslint --quiet and prettier --check are clean. (Unit tests need BITGOJS_TEST_PASSWORD — CI secret — so CI validates the added cases.)

[Vijay-Jagannathan]

Re-review of 0c7950cb44. All blocking findings from my previous review are resolved, so I'm approving.

• Pair check is order-independent. contractAddress() and contractName() both call isContractPairValid() once the other field is set. Walking through both bypasses I flagged:
  • Allowlisted name, then canonical address: isCanonicalName is false and the allowlist pair does not match, so it throws.
  • Allowlisted address, then pox-5: isCanonicalAddress is false, so it falls through to the allowlist, name does not match, and it throws.
  • Canonical name with canonical address (including the existing pox-4 with the send-many address) is unchanged.
• Duplicate allowlist names are rejected in allowedStakingContracts().
• No shared state: the allowlist is now an explicit allowedStakingContracts param on signTransaction and explainTransaction. The Stx instance setter is gone and verifySbtcWithdrawTransaction is back to the plain from(rawTx).
• Tests now cover both reversed orders and duplicate names. The rehydration test asserts ParseTransactionError and a rebuilt payload instead of private fields.

Before merge:

• The PR description (Change section, bullet 2) still says functionName "skips the canonical ABI check". It now validates against the pox-5 function set. Please correct it.
• The unit-test jobs were still pending when I looked. Please confirm they are green; the new tests need BITGOJS_TEST_PASSWORD, so CI is the only validation.
• The WP companion PR must pass allowedStakingContracts on every sign/explain/fee-rebuild call. Omitting it fails closed with ParseTransactionError.

The two inline comments are optional.