fix(x/auth): Add fees on batch sign #18564
Conversation
WalkthroughWalkthroughThe changes primarily address a bug in the Cosmos SDK related to the incorrect calculation of total fees when batch signing transactions with the Changes
Assessment against linked issues
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (2)
- CHANGELOG.md (1 hunks)
- x/auth/client/cli/tx_sign.go (3 hunks)
Additional comments: 7
CHANGELOG.md (1)
- 72-72: The changelog entries correctly summarize the bug fixes made in the respective pull requests.
x/auth/client/cli/tx_sign.go (6)
- 130-140: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [120-137]
The implementation of fee accumulation using the
feeAmountvariable and setting it in thetxBuilderappears to be correct and aligns with the summary provided.
- 130-140: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [132-153]
The logic for incrementing the gas limit and setting it in the
txBuilderis correctly implemented.
147-150: The memo, feeGranter, and feePayer are being set in the
txBuilderfrom the command flags as expected.159-160: The call to
sigTxOrMultisigis correctly passing the necessary parameters, and error handling is implemented.160-160: The signed transaction or signature is correctly being printed to the command's output.
146-160: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [120-160]
Overall, the changes made to the
makeSignBatchCmdfunction for handling fees in batch transactions are correctly implemented and should improve the functionality as described in the summary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments: 1
CHANGELOG.md (1)
- 72-72: The changelog entry for the server config fix correctly specifies the use of a comma
,as the delimiter, addressing the previous comment.
yes, happy to add them, I couldn't find any test file where makeSignBatchCmd (or similar) is being tested. |
Yes pls :) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/auth/client/cli/tx_sign.go (3 hunks)
Additional comments: 5
x/auth/client/cli/tx_sign.go (5)
117-123: The introduction of the
appendMessagesToSingleTxflag and the subsequent logic to accumulate fees and gas limits for batch signing is a good enhancement. It allows for the creation of a single signed transaction from multiple messages, which can be more efficient in certain scenarios.130-136: The logic to increment the gas limit and accumulate the fees for each transaction in the batch is correctly implemented. This ensures that the resulting single transaction has the correct gas limit and fee amount.
146-155: The code correctly sets the transaction builder's memo, fee granter, fee payer, gas limit, and fee amount based on the command flags and the accumulated values. This ensures that the single signed transaction is constructed with the correct parameters.
158-159: The call to
sigTxOrMultisigto sign the transaction is appropriate. It handles both regular and multisig accounts, which is necessary for a flexible signing command.158-159: The code correctly marshals the signed transaction or signature to JSON and prints it to STDOUT or the specified output file. This is the expected behavior for a sign-batch command.
julienrbrt
changed the title
There was a problem hiding this comment.
Nice work and thanks for the fix @raynaudoe! I've added some suggestions to the code but also please send a test that'll catch this regression if it ever creeps up again: the lack of a test means that even if the logic radically changes or is wrong, we'd never catch the problem until someone catches it in production. Thank you
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/auth/CHANGELOG.md (1 hunks)
- x/auth/client/cli/tx_sign.go (3 hunks)
Additional comments: 4
x/auth/CHANGELOG.md (1)
- 34-36: The changelog entry for the bug fix related to batch signing fees is correctly formatted and placed under the "Bug Fixes" section with the appropriate tag and GitHub issue reference.
x/auth/client/cli/tx_sign.go (3)
117-123: The initialization of
totalFees,txBuilder,msgs, andnewGasLimitis correct and sets up the necessary variables for aggregating transaction fees and messages.130-140: The logic for aggregating the gas limit and fees from individual transactions is correct. However, it's important to ensure that the
sdk.Coinstype used fortotalFeescan handle the addition of fees correctly without any overflow or other issues.147-161: The setting of transaction properties such as memo, fee granter, fee payer, gas limit, and fee amount is done correctly. However, it's important to verify that the
SetFeeAmountmethod correctly handles thesdk.Coinstype and that thetotalFeesvariable contains the correct aggregated fee amount.
There was a problem hiding this comment.
Thank you @raynaudoe, LGTM with some comments and also please submit that regression test.
Yes, I'm currently working on those tests |
Allow batch sign helper function to receive a slice of txs filenames
Done! |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- tests/integration/auth/client/cli/suite_test.go (3 hunks)
- x/auth/CHANGELOG.md (1 hunks)
- x/auth/client/cli/tx_sign.go (3 hunks)
- x/auth/client/testutil/helpers.go (1 hunks)
Additional comments: 10
tests/integration/auth/client/cli/suite_test.go (4)
159-171: The changes to the
TestCLISignBatchfunction reflect the necessary adjustments for testing the new batch signing functionality with different flags. The home directory replacement and the execution of theTxSignBatchExecfunction with various combinations of flags are consistent with the expected behavior for testing the batch signing feature.174-229: The new test function
TestCLISignBatchTotalFeesis a valuable addition to the test suite. It properly tests the batch signing functionality by creating multiple transactions, writing them to separate files, and verifying that the expected total fee matches the actual total fee after batch signing. This ensures that the total fee calculation for batched transactions is correct.796-802: The changes in the
TestSignBatchMultisigfunction correctly reflect the updatedTxSignBatchExecfunction signature, which now accepts a slice of strings representing file names. This allows the function to handle multiple transaction files, which is necessary for testing batch signing with multisig.806-812: The continuation of the
TestSignBatchMultisigfunction with the signing of the batch file with a second account is also correctly implemented. The test ensures that the batch signing process works with multiple signers, which is a critical aspect of multisig functionality.x/auth/CHANGELOG.md (1)
- 36-36: The changelog entry correctly summarizes the bug fix and includes the appropriate link to the pull request.
x/auth/client/cli/tx_sign.go (4)
117-123: The initialization of variables for combining messages into a single transaction and calculating total fees appears to be correctly implemented.
130-142: The logic for incrementing the gas limit, aggregating fees, and appending messages for batch transactions is correctly implemented. Ensure that the gas limit and fees are calculated correctly and that the messages are compatible when combined.
148-162: The code correctly sets transaction properties and signs the transaction. Verify that the properties such as memo, fee granter, and fee payer are being set as intended and that the signing process is secure and error-free.
148-162: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [117-162]
The changes made to
x/auth/client/cli/tx_sign.goare consistent with the summaries provided, addressing the correct aggregation of fees for batch-signed transactions and setting the appropriate transaction properties before signing.x/auth/client/testutil/helpers.go (1)
- 66-68: The changes to
TxSignBatchExecto accept multiple filenames are correctly implemented and match the summary description. Ensure that all calls to this function are updated to pass a slice of filenames where necessary.
julienrbrt
added
the
backport/v0.50.x
(cherry picked from commit 7bdbc46) # Conflicts: # tests/e2e/auth/suite.go # x/auth/CHANGELOG.md # x/auth/client/cli/tx_sign.go
Description
Closes: #18064
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
Bug Fixes
Documentation
Tests