fix: txHex needs to be set for the txn in case of TSS#8789
Conversation
8f36134 to
1d5faf4
Compare
1d5faf4 to
99c4623
Compare
99c4623 to
f928cdb
Compare
pritam-gembali
left a comment
There was a problem hiding this comment.
The if (unsignedTx?.serializedTxHex) guard at modules/sdk-core/src/bitgo/wallet/wallet.ts:3534-3536 is a silent fallback. If the txRequest comes back in an unexpected shape, the assignment is skipped, no error is raised, and the prebuild is returned with txHex still missing — re-introducing the very bug this PR is fixing, just with an extra HTTP round-trip.
Concrete failure mode
Imagine the backend returns apiVersion: 'full' but transactions is [] (or its first element has no unsignedTx field — e.g. a future API rename):
- Build returns
{ txRequestId: 'req-1', stakingParams: {...} }with notxHex. - We enter the new branch, fetch the txRequest. ✓
txRequest.transactions?.[0]?.unsignedTx→undefined.if (unsignedTx?.serializedTxHex)→ false. Block silently skipped.- Prebuild is returned without
txHex. - Caller hits
prebuildAndSignTransaction→verifyTransactionatwallet.ts:2189, which seestxPrebuild?.txHexis undefined and either skips verification or throws something downstream that looks unrelated (e.g. a coin-specific verify failing onundefined.length).
The user sees the downstream error and has no breadcrumb pointing back to the real cause seven steps earlier. Debugging means re-running with a debugger.
Suggested fix
if (!unsignedTx?.serializedTxHex) {
throw new Error(
`Expected serializedTxHex on TSS resource management prebuild for txRequestId ${prebuild.txRequestId}`
);
}
prebuild = _.extend({}, prebuild, { txHex: unsignedTx.serializedTxHex });This matches the assertive style already used at wallet.ts:4193-4202 in prebuildTransactionTxRequests, where missing/malformed txRequest shapes throw explicitly.
Test to update
The new test should leave txHex undefined when txRequest has no serializedTxHex in modules/sdk-core/test/unit/bitgo/wallet/resourceManagement.ts pins the silent-skip behavior as intended. Once that test merges, future reviewers will assume the silent fallback is by design — making it harder to harden later. Please flip it to expect a thrown error:
await wallet.buildResourceDelegations({ delegations: [delegations[0]] })
.should.be.rejectedWith(/Expected serializedTxHex/);Happy to approve once the throw is in place and the test is updated.
TICKET: CHALO-349
f928cdb to
11e1a85
Compare
TICKET: CHALO-349