Proposal integrity and sendmax - #4208
Conversation
There was a problem hiding this comment.
Pull request overview
Improves transaction proposal integrity across Bitcore Wallet Client/Service by verifying client-specified fixed fees, explicit UTXO inputs, and XRP destination tags, while also correcting send-max fee-level forwarding and DOGE send-max input reporting.
Changes:
- Forward
feeLevelintogetSendMaxInfoduringsendMaxtransaction creation to preserve the requested fee policy. - Fix DOGE send-max to return only the inputs actually selected within the maximum transaction size limit.
- Strengthen BWC proposal verification for fixed fees, explicit inputs (by outpoint), and XRP destination tags; update related test fixtures to use canonical
vout.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/bitcore-wallet-service/test/integration/server.test.ts | Adds integration coverage for feeLevel forwarding in send-max and DOGE max-tx-size input reporting. |
| packages/bitcore-wallet-service/src/lib/server.ts | Forwards feeLevel into getSendMaxInfo during send-max option validation. |
| packages/bitcore-wallet-service/src/lib/chain/doge/index.ts | Returns txp.inputs (selected subset) rather than the full candidate input set for DOGE send-max. |
| packages/bitcore-wallet-client/test/verifier.test.ts | Adds unit tests for verifying fixed fees, explicit input outpoints/values, and XRP destination tags. |
| packages/bitcore-wallet-client/test/helpers.ts | Updates UTXO fixture field to vout for outpoint verification consistency. |
| packages/bitcore-wallet-client/src/lib/verifier.ts | Implements normalization and equality checks to enforce proposal integrity for fees, explicit inputs, and destination tags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
kajoseph
left a comment
kajoseph
left a comment
There was a problem hiding this comment.
I have some questions about the handling of large, unsafe numbers (applicable with EVM)
| if (typeof value === 'number') { | ||
| return Number.isSafeInteger(value) && value >= 0 ? BigInt(value) : null; | ||
| } | ||
| if (typeof value === 'string' && /^(0|[1-9]\d*)$/.test(value)) { |
There was a problem hiding this comment.
What about hex strings (assume 0x prefix)?
There was a problem hiding this comment.
Good catch. I will add in support for that.
| private static normalizeAtomicValue(value) { | ||
| if (typeof value === 'bigint') return value >= 0n ? value : null; | ||
| if (typeof value === 'number') { | ||
| return Number.isSafeInteger(value) && value >= 0 ? BigInt(value) : null; |
There was a problem hiding this comment.
return null if is a very large, unsafe number? what are the implications of that?
There was a problem hiding this comment.
My thought was this: An unsafe JavaScript number may already be rounded before the verifier receives it, so converting it to BigInt could preserve the wrong value. Returning null makes verification fail safely. Large values would need to be sent as a bigint or decimal/hex string to be compared exactly.
There was a problem hiding this comment.
When creating a txp, if I give a fee of any unsafe integer (e.g. 12345678901234567), this function will automatically return null which means the upstream checkProposalCreation will automatically fail (even if the unsafe integers happen to equate).
In practice, I'm not sure how common this actually is to pass in a specific fee (vs feeLevel or feePerKb) and also as a Number. And this probably would only happen with EVM (which uses much larger numbers than other chains)
| } | ||
|
|
||
| private static optionalAtomicValuesEqual(value1, value2) { | ||
| const value1Missing = value1 === undefined || value1 === null; |
There was a problem hiding this comment.
can be shortened to const value1Missing = value1 == null;
Description
Ensures transaction proposals preserve client-requested fixed fees, explicit UTXO inputs, and XRP destination tags. Also fixes send-max fee-level forwarding and DOGE input reporting.
Update the test UTXO fixture to use BWS’s canonical vout field for input outpoint verification.
Changelog
Checklist