fix(x/tx): add feePayer as signer #22311
Conversation
JulianToledano
requested review from
kocubinski,
testinginprod,
aaronc and
a team
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request update the changelog for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
This comment has been minimized.
This comment has been minimized.
|
Not a big fan of setting this kind of logic in a decoder to be honest. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
x/tx/CHANGELOG.md (1)
39-39: New changelog entry looks good, but consider adding more details.The new entry follows the correct format and is placed in the appropriate "Unreleased" section. The link to the pull request (#22311) is correct. However, consider expanding the description to provide more context about the fix.
Consider updating the entry to something like:
* [#22311](https://github.com/cosmos/cosmos-sdk/pull/22311) Fix security issue by adding feePayer as a required signer for transactions with a specified fee payer.This provides more context about the nature and importance of the fix.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/decode/decode.go (1 hunks)
- x/tx/decode/decode_test.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/tx/decode/decode.go
🧰 Additional context used
📓 Path-based instructions (2)
x/tx/CHANGELOG.md (1)
Pattern
**/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"x/tx/decode/decode_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (2)
x/tx/decode/decode_test.go (1)
Line range hint
1-165: Overall test structure is robust, but consider enhancing coverageThe test file is well-structured and follows good Go testing practices. It uses table-driven tests, mock objects, and appropriate assertions. However, given the recent change to the
Payerfield, consider the following improvements:
- Expand the test cases to cover various fee payer scenarios (empty, non-empty, invalid).
- Add more granular assertions to verify the correct handling of the
Payerfield in decoded transactions.- Consider adding benchmarks to measure the performance impact of decoding transactions with different fee payer configurations.
These enhancements will further improve the robustness and coverage of the decode package tests.
x/tx/CHANGELOG.md (1)
Line range hint
1-186: Overall changelog structure and content are well-maintained.The changelog follows best practices:
- Clear organization by version, with the most recent changes at the top
- Proper grouping of changes by type (Features, Improvements, Bug Fixes, etc.)
- Consistent formatting and linking to pull requests
- Clear indication of Semantic Versioning compliance
x/tx/decode/decode_test.go
Outdated
| @@ -94,7 +94,7 @@ func TestDecode(t *testing.T) { | |||
| Fee: &txv1beta1.Fee{ | |||
| Amount: []*basev1beta1.Coin{{Amount: "100", Denom: "denom"}}, | |||
| GasLimit: 100, | |||
| Payer: "payer", | |||
| Payer: "", | |||
There was a problem hiding this comment.
Update test cases to cover empty fee payer scenario
The Payer field in the Fee structure has been changed to an empty string. This modification alters the test scenario, potentially affecting how the decoder handles transactions without a specified fee payer.
To ensure comprehensive test coverage:
- Add a new test case that explicitly checks the behavior when the fee payer is an empty string.
- Update existing test cases to verify that the decoder correctly handles both empty and non-empty fee payer scenarios.
- Consider adding assertions to verify the decoded transaction's fee payer field matches the expected value (empty or non-empty) after decoding.
Example of an additional test case:
{
name: "empty fee payer",
msg: &bankv1beta1.MsgSend{},
checkFn: func(t *testing.T, decodedTx *decode.Tx) {
require.Empty(t, decodedTx.AuthInfo.Fee.Payer, "Expected empty fee payer")
// Add more assertions as needed
},
},Update the test loop to include the new checkFn:
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
// ... (existing test setup)
decodeTx, err := decoder.Decode(txBytes)
if tc.error != "" {
require.EqualError(t, err, tc.error)
return
}
require.NoError(t, err)
// ... (existing assertions)
if tc.checkFn != nil {
tc.checkFn(t, decodeTx)
}
})
}These changes will help ensure that the decoder correctly handles transactions with empty fee payers, improving the overall test coverage.
julienrbrt
added
the
backport/v0.52.x
julienrbrt
removed
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
x/tx/decode/decode_test.go (3)
73-81: LGTM: Comprehensive "happy path" test caseThe updated "happy path" test case now includes detailed
authInfowith fee information, providing a more thorough test of the basic scenario. The emptyPayerfield correctly tests the default behavior without a fee payer.Consider adding a comment explaining that the empty
Payerfield represents the default scenario without a fee payer, to enhance code readability.
99-111: Good addition: "invalid feePayer" test caseThe new test case for an invalid fee payer effectively checks the decoder's behavior when provided with an incorrect fee payer format. This enhances the overall test coverage.
Consider adding a brief comment explaining why "payer" is an invalid input (i.e., the decoder expects a hexadecimal string). This would improve the test's readability and make the intent clearer.
113-125: Excellent addition: "valid feePayer" test caseThe new test case for a valid fee payer effectively checks the decoder's behavior when provided with a correct fee payer format. The use of a hexadecimal string for the
Payerand the increasednumSignerscorrectly test the new functionality.To improve clarity:
- Consider adding a comment explaining the hexadecimal string's significance (i.e., it represents a valid address).
- You might want to extract the hexadecimal string to a named constant at the top of the test file, which could be reused in other test cases if needed.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- x/tx/decode/decode_test.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/tx/decode/decode_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (3)
x/tx/decode/decode_test.go (3)
64-68: Improved test case structureThe updated test case structure with
authInfoandnumSignersfields enhances the test coverage, particularly for the new fee payer functionality. This change allows for more comprehensive testing of theAuthInfocomponent and explicit verification of the number of signers.Also applies to: 82-82
141-141: LGTM: Consistent test execution updatesThe changes to the test execution logic correctly incorporate the new
authInfofield and verify the number of signers. These modifications ensure that the new fee payer functionality is properly tested and validated.Also applies to: 153-153
Line range hint
1-200: Overall assessment: Improved test coverage and structureThe changes to this test file significantly enhance the test coverage for the new fee payer functionality. The updated test case structure and new test cases effectively validate various scenarios, including happy path, invalid fee payer, and valid fee payer cases.
To further improve the test suite:
- Consider adding edge cases, such as testing with multiple fee payers or extremely large/small fee amounts.
- Ensure that all error scenarios are covered, including potential issues with fee granter functionality.
- If not already present in other test files, consider adding integration tests that cover the interaction between the decoder and other components of the system.
These additions would provide even more comprehensive coverage of the new functionality and potential edge cases.
To verify the completeness of the test coverage, you can run the following command:
This will generate a coverage report, allowing you to identify any areas that may need additional testing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
x/tx/decode/decode_test.go (4)
73-82: LGTM: Detailed "happy path" test caseThe updated "happy path" test case now includes a detailed
authInfostructure, which allows for more precise testing of the decoder's behavior. The emptyPayerfield correctly tests the default scenario where no specific fee payer is set.Consider adding an assertion to verify that the decoded transaction's fee payer field is empty after decoding:
require.Empty(t, decodeTx.AuthInfo.Fee.Payer, "Expected empty fee payer in happy path")This additional check would ensure that the decoder correctly handles the empty fee payer scenario.
99-111: LGTM: New test case for invalid fee payerThe addition of the "invalid feePayer" test case is valuable for verifying the decoder's behavior when an invalid fee payer is provided. The test correctly expects an error when trying to decode a non-hexadecimal string.
Consider adding a comment explaining why "payer" is an invalid input, to improve test readability:
Payer: "payer", // Invalid: not a hexadecimal stringThis comment would clarify the purpose of this test case for future developers.
113-125: LGTM: New test case for valid fee payerThe addition of the "valid feePayer" test case is excellent for verifying the decoder's behavior with a valid fee payer address. The test correctly expects an increase in the number of signers, indicating that the fee payer is added as an additional signer.
To further strengthen this test, consider adding an assertion to verify that the decoded fee payer matches the input:
require.Equal(t, tc.authInfo.Fee.Payer, decodeTx.AuthInfo.Fee.Payer, "Decoded fee payer should match input")This additional check would ensure that the decoder correctly preserves the fee payer information during the decoding process.
126-141: LGTM: New test case for identical message signer and fee payerThe addition of the "same msg signer and feePayer" test case is valuable for verifying the decoder's behavior when the message signer and fee payer are the same entity. The test correctly expects no increase in the number of signers, indicating that duplicate signers are not added.
To further strengthen this test, consider adding an assertion to verify that the decoded signers list contains only one entry:
require.Len(t, decodeTx.Signers, 1, "Expected only one signer when message signer and fee payer are the same")This additional check would ensure that the decoder correctly handles duplicate signers and doesn't add them multiple times to the signers list.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- x/tx/decode/decode_test.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/tx/decode/decode_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (2)
x/tx/decode/decode_test.go (2)
64-68: LGTM: Enhanced test case structureThe updated test case structure with
authInfoandnumSignersfields allows for more comprehensive testing of theAuthInfostructure and signer verification. This change aligns well with the new focus on fee payer handling in the decoder.
64-141: Overall assessment: Improved test coverage for fee payer handlingThe changes to this test file significantly enhance the coverage of fee payer handling scenarios in the decoder. The new test cases effectively cover:
- Default behavior with no fee payer
- Invalid fee payer input
- Valid fee payer different from the message signer
- Fee payer identical to the message signer
These additions will help ensure the robustness of the decoder's fee payer handling logic. The test structure is clear and consistent, making it easy to understand the purpose of each test case.
To further improve the tests:
- Consider adding assertions to verify the contents of the decoded
AuthInfostructure in each test case.- Add comments explaining the purpose of hexadecimal address strings used in the tests.
Overall, these changes represent a significant improvement in the test suite's ability to verify the decoder's behavior with regard to fee payers.
done |
|
@testinginprod or @kocubinski can we can we get one of your approval here? |
|
I'm not a fan on how the table test reads. rather than having arbitrary Auth Info which I think makes the specific cases harder to identify, i would have preferred to have tables about the specific test: eg: test{ |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
x/tx/decode/decode_test.go (2)
71-99: Add test cases for additional edge casesWhile the current test cases cover basic scenarios, consider adding the following cases to ensure comprehensive coverage of the security fix:
- Multiple signers with fee payer
- Invalid fee payer address format
- Empty fee payer string (as suggested in previous review)
- Fee payer with empty signatures
Example test case:
{ name: "empty fee payer string", msg: &bankv1beta1.MsgSend{}, feePayer: "", expectedSigners: 1, }, { name: "fee payer with empty signatures", msg: &bankv1beta1.MsgSend{}, feePayer: "636f736d6f733168363935356b3836397a72306770383975717034337a373263393033666d35647a366b75306c", expectedSigners: 2, // Override signatures in the test setup }
120-120: Enhance assertions for fee payer validationWhile checking the number of signers is good, consider adding more specific assertions to validate the fee payer security fix:
- Verify fee payer is actually in the signers list when specified
- Verify the order of signers (if it matters)
- Validate the fee payer address format in the decoded transaction
Example assertions:
if tc.feePayer != "" { require.Contains(t, decodeTx.Signers, tc.feePayer, "fee payer should be in signers list") }Also applies to: 135-135
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (1)
- x/tx/decode/decode_test.go (3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
x/tx/decode/decode_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (2)
x/tx/decode/decode_test.go (2)
64-68: LGTM: Test structure properly updated for fee payer validationThe test case structure has been appropriately enhanced to include
feePayerandexpectedSignersfields, which are essential for validating the security fix that prevents unauthorized fee payments.
71-99: Verify test coverage against PR objectivesThe test cases appropriately cover the security concern mentioned in the PR objectives about unauthorized fee payments. However, let's verify the coverage of all scenarios:
|
I think this should be backported to 0.52 @julienrbrt |
No need, x/tx is tagged for main from v1. |
* main: (157 commits) feat(core): add ConfigMap type (#22361) test: migrate e2e/genutil to systemtest (#22325) feat(accounts): re-introduce bundler (#21562) feat(log): add slog-backed Logger type (#22347) fix(x/tx): add feePayer as signer (#22311) test: migrate e2e/baseapp to integration tests (#22357) test: add x/circuit system tests (#22331) test: migrate e2e/tx tests to systemtest (#22152) refactor(simdv2): allow non-comet server components (#22351) build(deps): Bump rtCamp/action-slack-notify from 2.3.1 to 2.3.2 (#22352) fix(server/v2): respect context cancellation in start command (#22346) chore(simdv2): allow overriding the --home flag (#22348) feat(server/v2): add SimulateWithState to AppManager (#22335) docs(x/accounts): improve comments (#22339) chore: remove unused local variables (#22340) test: x/accounts systemtests (#22320) chore(client): use command's configured output (#22334) perf(log): use sonic json lib (#22233) build(deps): bump x/tx (#22327) fix(x/accounts/lockup): fix proto path (#22319) ...
Description
Currently anyone can set a feePayer and the fees'll be deducted from its balance without the need of his sign.
For example:
Also currently is not possible to do the double sign of this tx to set feePayers signature:
After this fix, the tx will fail when trying to send without feePayers signature
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...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
feePayeras a signer and organized existing entries chronologically.