feat(sdk-core): add automatic signature cleanup for Express TSS signing #7256
Conversation
|
@claude review |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an API gap in the Express TSS signing endpoint where re-signing partially signed Full TxRequests would fail due to stale signature shares. The solution adds automatic signature cleanup before signing operations.
- Adds signature share cleanup logic to detect and clean partial signatures before signing
- Updates Express TSS signing endpoint to use the new cleanup method
- Provides comprehensive test coverage for the signature cleanup scenarios
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| modules/sdk-core/src/bitgo/wallet/wallet.ts | Adds ensureCleanSigSharesAndSignTransaction method with signature cleanup logic |
| modules/express/src/clientRoutes.ts | Updates Express TSS signing to use new cleanup method |
| modules/bitgo/test/v2/unit/wallet.ts | Adds comprehensive unit tests for signature cleanup functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| isPartiallySigned = txRequest.transactions.some((tx) => (tx.signatureShares ?? []).length > 0); | ||
| } else if (txRequest.messages && txRequest.messages.length > 0) { | ||
| isPartiallySigned = txRequest.messages.some((msg) => (msg.signatureShares ?? []).length > 0); |
There was a problem hiding this comment.
The partial signature detection logic is duplicated between transactions and messages. Consider extracting this into a helper function to reduce code duplication and improve maintainability.
Copilot uses AI. Check for mistakes.
| ): Promise<SignedTransaction | TxRequest> { | ||
| const txRequestId = params.txRequestId || params.txPrebuild?.txRequestId; | ||
|
|
||
| if (txRequestId && this.tssUtils && this.multisigType() === 'tss') { |
There was a problem hiding this comment.
can we also separate the cleaning part in different method. It think it will make the method ensureCleanSigSharesAndSignTransaction more readable.
dpkjnr
force-pushed
the
delete-sigshare-on-signTss
branch
2 times, most recently
from
b8a43f0 to
ae9c08f
Compare
dpkjnr
force-pushed
the
delete-sigshare-on-signTss
branch
from
ae9c08f to
f8fcd9e
Compare
| let isPartiallySigned = false; | ||
|
|
||
| if (txRequest.transactions && txRequest.transactions.length > 0) { | ||
| isPartiallySigned = txRequest.transactions.some((tx) => (tx.signatureShares ?? []).length > 0); |
There was a problem hiding this comment.
Are we not considering any transaction state for this?
There was a problem hiding this comment.
I saw something similar in wallet-platform also, would there be any case where sigShares are there but state is postSign or something where we should not delete sig shares?
| // Create mock wallet with ensureCleanSigSharesAndSignTransaction method | ||
| const mockWallet = { | ||
| signTransaction: sinon.stub().resolves(mockFullySignedResponse), | ||
| ensureCleanSigSharesAndSignTransaction: sinon.stub().resolves(mockFullySignedResponse), |
There was a problem hiding this comment.
The current tests target signTransaction. Please add tests that perform cleanup of any existing transaction and then sign the transaction, to validate the business logic in ensureCleanSigSharesAndSignTransaction.
modules/bitgo/test/v2/unit/wallet.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| describe('ensureCleanSigSharesAndSignTransaction', function () { |
There was a problem hiding this comment.
I think these tests should go in test/unit/typedRoutes/walletTxSignTss.ts, since we are calling ensureCleanSigSharesAndSignTransaction from the handler handleV2SignTSSWalletTx.
There was a problem hiding this comment.
Added those before your PR was merged, let me move these to walletTxSignTss
Fixes Express API gap where re-signing partially signed Full TxRequests would fail. The /signtxtss route now automatically detects and cleans up partial signature shares before attempting to sign. Changes: - Add ensureCleanSigSharesAndSignTransaction() to Wallet class - Update Express handleV2SignTSSWalletTx() to use new method - Add comprehensive unit tests for signature cleanup logic - Check signature shares presence instead of transaction state Benefits: - Fixes re-signing failures for partially signed transactions - Backward compatible with existing signing flows - No API changes required - Keeps tssUtils properly encapsulated Ticket: COIN-5989
dpkjnr
force-pushed
the
delete-sigshare-on-signTss
branch
from
f8fcd9e to
71b889c
Compare
5 participants
Fixes Express API gap where re-signing partially signed Full TxRequests would fail. The /signtxtss route now automatically detects and cleans up partial signature shares before attempting to sign.
Changes:
Benefits:
Ticket: COIN-5989