refactor: cap tx size flups #3967
Conversation
ninabarbakadze
requested review from
staheri14 and
evan-forbes
and removed request for
a team
|
📝 WalkthroughWalkthroughThe changes in this pull request involve renaming the transaction size constant from 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
app/ante/max_tx_size.go (1)
27-30: LGTM! Consider a minor improvement in the error message.The changes are consistent with the PR objectives. The renaming from
MaxTxBytestoMaxTxSizeimproves clarity by distinguishing between the mempool and application parameters.Consider updating the error message to be more specific:
- return ctx, fmt.Errorf("tx size %d bytes is larger than the application's configured threshold of %d bytes. Please reduce the size by %d bytes", currentTxSize, maxTxSize, bytesOverLimit) + return ctx, fmt.Errorf("tx size %d bytes exceeds the maximum allowed size of %d bytes. Please reduce the size by %d bytes", currentTxSize, maxTxSize, bytesOverLimit)This change makes the error message more direct and easier to understand.
pkg/appconsts/versioned_consts.go (1)
45-46: LGTM! Consider adding a comment for the unused parameter.The renaming from
MaxTxBytestoMaxTxSizeis correct and aligns with the PR objectives. The function logic remains unchanged, which is appropriate.Consider adding a comment explaining the purpose of the unused
uint64parameter, as it might not be immediately clear to other developers why it's there. For example:// MaxTxSize returns the maximum transaction size. // The uint64 parameter is unused but kept for version compatibility. func MaxTxSize(_ uint64) int { return v3.MaxTxSize }pkg/appconsts/versioned_consts_test.go (1)
70-73: LGTM! Consider updating the version comment for consistency.The changes correctly implement the renaming of "MaxTxBytes" to "MaxTxSize" as per the PR objectives. This improves clarity by distinguishing between mempool and application parameters.
For consistency with other test cases, consider updating the version comment in the test case name:
- name: "MaxTxSize v3", + name: "MaxTxSize v3.0",This aligns with the naming convention used in other test cases (e.g., "SubtreeRootThreshold v3").
app/test/prepare_proposal_test.go (1)
234-238: LGTM: New test case added for MaxTxSize limitThe new test case is a valuable addition, ensuring that both regular transactions and blob transactions are properly handled when they exceed the MaxTxSize limit. This is consistent with the changes made to the MaxTxSize parameter.
Consider adding an assertion to verify that the pruned transactions are indeed larger than the MaxTxSize limit. This would make the test more robust and self-documenting.
app/test/process_proposal_test.go (1)
Line range hint
352-362: LGTM: Test cases updated with new terminologyThe test case names and descriptions have been correctly updated to use "max tx size" instead of "max tx bytes", maintaining consistency with the renamed constant. This change improves the clarity and alignment of the test cases with the updated codebase.
Consider adding a comment above these test cases to explain the significance of testing transactions larger than the configured max tx size. This could provide additional context for future developers working on this code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- app/ante/max_tx_size.go (1 hunks)
- app/ante/max_tx_size_test.go (1 hunks)
- app/test/prepare_proposal_test.go (2 hunks)
- app/test/process_proposal_test.go (4 hunks)
- app/validate_txs.go (0 hunks)
- pkg/appconsts/v3/app_consts.go (1 hunks)
- pkg/appconsts/versioned_consts.go (1 hunks)
- pkg/appconsts/versioned_consts_test.go (1 hunks)
- specs/src/ante_handler_v3.md (1 hunks)
💤 Files with no reviewable changes (1)
- app/validate_txs.go
✅ Files skipped from review due to trivial changes (1)
- pkg/appconsts/v3/app_consts.go
🧰 Additional context used
🔇 Additional comments (12)
pkg/appconsts/versioned_consts.go (1)
45-46: Verify consistency of renaming across the codebaseThe renaming from
MaxTxBytestoMaxTxSizeappears consistent within this file. However, it's important to ensure this change is applied consistently across the entire codebase.Please run the following script to check for any remaining instances of
MaxTxBytesthat might need to be updated:If any instances are found, please update them to maintain consistency with this change.
app/ante/max_tx_size_test.go (5)
27-28: LGTM: Consistent terminology updateThe change from "max tx bytes" to "max tx size" in the test case name and the use of
v3.MaxTxSizeinstead ofv3.MaxTxBytesis consistent with the PR objectives. This update improves clarity and maintains the original test logic.
34-35: LGTM: Consistent terminology updateThe change from "max tx bytes" to "max tx size" in the test case name and the use of
v3.MaxTxSizeinstead ofv3.MaxTxBytesis consistent with the PR objectives. This update maintains the original test logic while improving clarity.
41-42: LGTM: Consistent terminology updateThe change from "max tx bytes" to "max tx size" in the test case name and the use of
v3.MaxTxSizeinstead ofv3.MaxTxBytesis consistent with the PR objectives. This update improves clarity while maintaining the original test logic.
49-49: LGTM: Consistent update and important backwards compatibility testThe use of
v3.MaxTxSizeinstead ofv3.MaxTxBytesis consistent with the PR objectives. This test case is particularly important as it verifies that the new size limit only applies to v3 and above, ensuring backwards compatibility with earlier versions.
Line range hint
1-79: Summary: Successful renaming and maintained test integrityThe changes in this file consistently update the terminology from "max tx bytes" to "max tx size" across all test cases. This renaming aligns with the PR objectives and improves clarity in the codebase. Importantly, the logic of the tests remains unchanged, ensuring that the functionality is still correctly tested. The update includes:
- Renaming test case descriptions
- Updating
txSizevalues to usev3.MaxTxSizeinstead ofv3.MaxTxBytes- Maintaining the existing test logic for various scenarios (under threshold, over threshold, equal to threshold, and version compatibility)
These changes contribute to a more consistent and clear codebase while preserving the integrity of the tests.
specs/src/ante_handler_v3.md (1)
6-6: Approved: Terminology update from MaxTxBytes to MaxTxSizeThe change from "MaxTxBytes" to "MaxTxSize" in the documentation is consistent with the PR objectives and improves clarity in distinguishing between mempool and application parameters.
To ensure consistency across the documentation, please run the following script:
This will help identify any instances where the old terminology might still be in use or where the new terminology might be inconsistently applied.
✅ Verification successful
Verified: Terminology update from MaxTxBytes to MaxTxSize
The terminology update from "MaxTxBytes" to "MaxTxSize" has been successfully verified across the documentation. No remaining instances of "MaxTxBytes" were found, ensuring consistency and clarity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of "MaxTxBytes" in the documentation # and verify that "MaxTxSize" is used consistently. echo "Checking for any remaining instances of 'MaxTxBytes':" rg --type md "MaxTxBytes" specs/ echo "Verifying consistent usage of 'MaxTxSize':" rg --type md "MaxTxSize" specs/Length of output: 409
app/test/prepare_proposal_test.go (2)
163-163: LGTM: Comment updated correctlyThe comment has been appropriately updated to reflect the change from "MaxTxBytes" to "MaxTxSize", which is consistent with the PR objectives.
166-166: LGTM: Comment updated correctlyThe comment has been appropriately updated to reflect the change from "MaxTxBytes" to "MaxTxSize", which is consistent with the PR objectives.
app/test/process_proposal_test.go (3)
57-57: LGTM: Consistent renaming of MaxTxBytes to MaxTxSizeThe change from
MaxTxBytestoMaxTxSizeis consistent with the PR objectives and improves clarity in distinguishing between mempool and application parameters.
60-61: LGTM: Updated comment to reflect new terminologyThe comment has been appropriately updated to use "max tx size" instead of "max tx bytes", maintaining consistency with the renamed constant.
71-72: LGTM: Consistent terminology update in commentThe comment has been correctly updated to use "max tx size limit" instead of "max tx bytes limit", aligning with the renamed constant and maintaining consistency throughout the test file.
Overview
MaxTxBytestoMaxTxSizein order to better distinguish between the mempool and the application paramctx.WithTxBytesfor the transaction twice in one function due to two prs working on similar changes