fix: bump polkadot 16 for tDOT recovery signatures - #9562
Conversation
Ticket: WCI-1466 westmint ChargeAssetTxPayment is tip + Option<Location>. Types 14.1.1 encodes a double Option so MPC signs a payload the node rejects (BadProof). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @vibhavgo's task —— View job Code Review for PR #9562Reviewing the Polkadot dependency bump and tDOT recovery signature fixes. Progress
Review SummaryThis PR successfully addresses the stated goals of bumping Polkadot dependencies to v16.x and fixing tDOT recovery issues. The implementation is well-structured and the changes are appropriate for the problem being solved. Positive AspectsDependency Management:
Type Safety Implementation:
Backwards Compatibility:
Test Coverage:
Technical Implementation ReviewType Guard Logic (dot.ts:74-88): function isEddsaSigningMaterial(value: unknown): value is EddsaSigningMaterial {
// Proper null/type checking before property access
if (typeof value !== 'object' || value === null) return false;
if (!('version' in value)) return false;
// Version-specific validation with appropriate property checks
if (value.version === 'v1') {
return 'userPrv' in value && typeof value.userPrv === 'string';
}
if (value.version === 'v2') {
return 'encryptedUserKey' in value && typeof value.encryptedUserKey === 'string';
}
return false;
}This is a robust implementation that follows TypeScript best practices for type guards. Integration in Recovery Logic: Minor Suggestions
Test Quality (transferBuilder.ts)The new test assertion for ChargeAsset
Alignment with PR Goals✅ Polkadot v16 Dependency Bump: Successfully implemented across all relevant packages ConclusionThis is a well-implemented fix that addresses the specific technical requirements while maintaining code quality and backwards compatibility. The dependency version management is thorough, the type safety improvements are robust, and the test coverage appropriately validates the new functionality. Recommendation: ✅ Approve - Ready for merge after successful CI validation. Job Run: Link | Branch: feat/sdk-coin-dot/fix-recovery-payload |
Summary
tDOT Non-BitGo recovery in Wallet Recovery Wizard was producing extrinsics the Asset Hub Westend (westmint) node rejected as
InvalidTransaction::BadProof. Native-fee recoveries must sign a payload whoseChargeAssetTxPaymentextra istip + Option<AssetId>withAssetIdan XCM Location, and whoseCheckMetadataHashextra ismode = 0(opt out).@polkadot/api@14.1.1does not encode that payload the way the current runtime hashes it, so MPC signed the wrong bytes.This PR bumps the Polkadot JS / txwrapper stack that
sdk-coin-dotactually resolves at runtime, and makesrecover()ignore a non-signing-material second argument that WRW used to pass.Linear: WCI-1466
Confirmed after the local bump (same encoding this PR publishes): Asset Hub Westend extrinsic
0xef3a54…499f(balances.transfer_all, Success).Why each change
1. Bump
@polkadot/*16.5.6 + util 14.0.3 + txwrapper 7.5.3Needed because recover uses live
getMaterial()from Asset Hub Westend. The node hashes the SCALESignedPayload, including signed-extension extras. On Asset Hub, fee payment isChargeAssetTxPayment: extra is compacttipplusOption<T::AssetId>, andT::AssetIdis an XCM Location (native fees =None). Types 14.1.1 treatassetIdasTAssetConversion/ a doubleOption, soNonevsSome(Here)is wrong and the signature does not match →BadProof.Setting builder
assetId: '0x00'is not a fix: it encodesSome(Here)and fails with 1010 Inability to pay some fees. Keepmode: 0and omitassetId(native tip only).polkadot-js 16.x is the line that ships Metadata v16, Asset Hub type augmentation, and the ChargeAsset / CheckMetadataHash extras current westmint expects. util/keyring 14.0.3 is the matching
@polkadot/commonmajor for api 16.2. Root
resolutions/overrides(not onlysdk-coin-dot/package.json)Needed because BitGoJS yarn
resolutionswere still@polkadot/api@14.1.1. A rootyarnwould hoist 14.x back into the published DOT package even if the module declared 16. The published tarball must resolve api 16.5.6 at runtime.tao/polyx/abstract-substratestill declare 14.1.1; they now hoist 16 via resolutions.3.
recover(params, precomputedMaterial?: EddsaSigningMaterial)type guardNeeded because
recoverConsolidationspasses real{ version: 'v1' | 'v2', ... }as arg 2 so the keycard is not decrypted on every scan. WRW Electron historically calledbaseCoin.recover(params, openSSLBytes). Published 5.1.0 ignored arg 2. This source already has the second parameter;precomputedMaterial ?? decrypttreats a truthyArrayBufferas signing material andJSON.parse(undefined)throws.Public type stays
EddsaSigningMaterial?(same as ADA). RuntimeisEddsaSigningMaterialignores anything that is not v1userPrv/ v2encryptedUserKey. Do not putArrayBufferon the API.4. Tests on existing unsigned
transferAll/ recover sweep pathsNeeded because ChargeAsset
Nonemust stay encoded that way (regression for the 1010Some(Here)path). Assertions live on the sibling builder/recover tests, not a subclass that exposescreateBaseTxInfo.References (Polkadot / polkadot-js)
Asset Hub fee extra and Location-typed
assetId:ChargeAssetTxPayment/ XCM LocationassetIdon Asset Hub)Signed payload fields including
mode/CheckMetadataHash(opt out withmode = 0):Why the JS type packages must move with the runtime:
ChargeAssetTxPaymenton Westend/Kusama Asset Hub usesMultiLocation, not a compactu32CheckMetadataHashsigned extensionassetIdSCALE mismatch → node sees the wrong idTest Plan
yarn workspace @bitgo/sdk-coin-dot run unit-test(withBITGOJS_TEST_PASSWORD)npm installof the new@bitgo/sdk-coin-dotonly (no overlay /file:link)