refactor: rename limits from tx to msg #3984
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve renaming constants and updating logic related to transaction limits in the context of message handling. Specifically, constants 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 (3)
app/validate_txs.go (2)
48-48: LGTM! Consider updating the error message for consistency.The changes correctly implement the new message-based limit and align with the PR objectives. The variable renaming and logic updates improve clarity and consistency.
Consider updating the error message on line 61 for consistency:
- logger.Debug("skipping tx because the max non PFB message count was reached", "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash())) + logger.Debug("skipping tx because the max non-PFB message count was reached", "tx", tmbytes.HexBytes(coretypes.Tx(tx).Hash()))This change makes the error message consistent with the new terminology and improves readability.
Also applies to: 60-61, 64-64
93-93: LGTM! Consider minor improvements for consistency.The changes correctly implement the new message-based limit and align with the PR objectives. The variable renaming and logic updates improve clarity and consistency.
Consider the following minor improvements for consistency:
- Update the error message on line 105:
- logger.Debug("skipping tx because the max pfb message count was reached", "tx", tmbytes.HexBytes(coretypes.Tx(tx.Tx).Hash())) + logger.Debug("skipping tx because the max PFB message count was reached", "tx", tmbytes.HexBytes(coretypes.Tx(tx.Tx).Hash()))
- For consistency with the non-PFB case, consider adding a hyphen:
- logger.Debug("skipping tx because the max PFB message count was reached", "tx", tmbytes.HexBytes(coretypes.Tx(tx.Tx).Hash())) + logger.Debug("skipping tx because the max PFB-message count was reached", "tx", tmbytes.HexBytes(coretypes.Tx(tx.Tx).Hash()))These changes improve consistency in terminology and readability across the codebase.
Also applies to: 104-105, 108-108
app/test/prepare_proposal_test.go (1)
362-363: LGTM: Test case updated correctly, minor improvement suggestedThe test case has been properly updated to use
appconsts.MaxPFBMessages. The division bynumberOfMsgsPerTxcorrectly accounts for multiple messages per transaction.For improved clarity, consider adding a comment explaining the division:
// Divide by numberOfMsgsPerTx to ensure the total number of messages doesn't exceed MaxPFBMessages expectedTransactions: multiPFBsPerTxs[:appconsts.MaxPFBMessages/numberOfMsgsPerTx],
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- app/test/prepare_proposal_test.go (5 hunks)
- app/validate_txs.go (4 hunks)
- pkg/appconsts/global_consts.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (7)
pkg/appconsts/global_consts.go (1)
71-79: Approve the renaming of constants for improved clarity.The renaming of
NonPFBTransactionCaptoMaxNonPFBMessagesandPFBTransactionCaptoMaxPFBMessagesimproves code clarity and aligns with the PR objectives. The updated comments accurately reflect the new constant names.Let's verify the impact of these changes on the rest of the codebase:
✅ Verification successful
Verified the renaming of constants and ensured no remaining references to old names. The new constants are used appropriately across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to the old constant names and verify the usage of new constant names. # Test 1: Search for any remaining references to the old constant names echo "Searching for remaining references to old constant names:" rg --type go "NonPFBTransactionCap|PFBTransactionCap" # Test 2: Verify the usage of new constant names echo "Verifying usage of new constant names:" rg --type go "MaxNonPFBMessages|MaxPFBMessages"Length of output: 2031
app/validate_txs.go (1)
Line range hint
1-150: Overall changes look good. Verify related tests and documentation.The changes successfully implement the PR objectives by renaming limits from "tx" to "msg" and updating the associated logic. These modifications improve code clarity and consistency across the file.
To ensure completeness, please run the following script to check for any related tests or documentation that might need updating:
Please review the output of this script and update any related tests or documentation as necessary to maintain consistency with the changes made in this file.
✅ Verification successful
Related tests and documentation are up to date. No further action required.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for related tests and documentation that might need updating # Search for test files that might need updating echo "Searching for related test files:" fd --type f --extension go --exec grep -l 'PFBTransactionCap|NonPFBTransactionCap' {} # Search for documentation files that might need updating echo "Searching for related documentation files:" fd --type f --extension md --exec grep -l 'PFBTransactionCap|NonPFBTransactionCap' {} # Search for any remaining occurrences of old terminology echo "Searching for any remaining occurrences of old terminology:" rg 'PFBTransactionCap|NonPFBTransactionCap'Length of output: 526
Script:
#!/bin/bash # Description: Enhanced search for related tests and documentation that might need updating # Define search patterns with case insensitivity and possible variations PATTERNS='(?i)PFBTransactionCap|NonPFBTransactionCap|PFB Msg Cap|NonPFB Msg Cap' # Search for test files that might need updating echo "Searching for related test files with enhanced patterns:" fd --type f --extension go --exec grep -Ei -l "$PATTERNS" {} # Search for documentation files that might need updating echo "Searching for related documentation files with enhanced patterns:" fd --type f --extension md --exec grep -Ei -l "$PATTERNS" {} # Search for any remaining occurrences of old or varied terminology echo "Searching for any remaining occurrences of old or varied terminology:" rg -i 'PFBTransactionCap|NonPFBTransactionCap|PFB Msg Cap|NonPFB Msg Cap'Length of output: 18301
Script:
#!/bin/bash # Description: Corrected search for related tests and documentation that might need updating # Define search patterns without inline case-insensitivity PATTERNS='PFBTransactionCap|NonPFBTransactionCap|PFB Msg Cap|NonPFB Msg Cap' # Search for test files that might need updating echo "Searching for related test files with corrected patterns:" fd --type f --extension go --exec grep -Ei -l "$PATTERNS" {} # Search for documentation files that might need updating echo "Searching for related documentation files with corrected patterns:" fd --type f --extension md --exec grep -Ei -l "$PATTERNS" {} # Search for any remaining occurrences of old or varied terminology echo "Searching for any remaining occurrences of old or varied terminology:" rg -i 'PFBTransactionCap|NonPFBTransactionCap|PFB Msg Cap|NonPFB Msg Cap'Length of output: 813
app/test/prepare_proposal_test.go (5)
300-302: LGTM: Constant renamed and test case improvedThe change from
PFBTransactionCaptoMaxPFBMessagesaligns with the PR objective. Additionally, creating 500 more PFBs than the maximum allowed improves the test case by ensuring the capping behavior is properly tested.
336-338: LGTM: Constant renamed and test case improvedThe change from
NonPFBTransactionCaptoMaxNonPFBMessagesaligns with the PR objective. The test case is improved by creating 500 more MsgSends than the maximum allowed, ensuring thorough testing of the capping behavior.
357-358: LGTM: Test case updated to use new constantThe test case has been correctly updated to use
appconsts.MaxPFBMessages, ensuring consistency with the renamed constant. The capping behavior is properly tested by checking that the number of transactions is limited to the maximum allowed.
367-368: LGTM: Test cases updated consistentlyAll test cases have been correctly updated to use the new constants
appconsts.MaxNonPFBMessagesandappconsts.MaxPFBMessages. The changes are consistent across different scenarios, ensuring comprehensive testing of the capping behavior for both PFB and non-PFB messages.Also applies to: 379-380, 394-396
Line range hint
300-396: Summary: Successful implementation of constant renaming and test improvementsThe changes in this file successfully implement the renaming of constants from "tx" to "msg" as per the PR objectives. Additionally, the test cases have been improved to better validate the capping behavior for both PFB and non-PFB messages. The modifications ensure that the
PrepareProposalfunction is thoroughly tested with the new message-based limits.Key improvements:
- Consistent use of
MaxPFBMessagesandMaxNonPFBMessagesconstants.- Test cases now create more messages than the maximum allowed, ensuring proper capping.
- Correct handling of scenarios with multiple messages per transaction.
These changes enhance the test coverage and align the code with the intended terminology shift from transactions to messages.
|
Thoughts @evan-forbes ? It's technically breaking but it's a really small constant rename and there are no downstream users of the old constants (yet). |
|
I think its fine to include |
Motivation: celestiaorg/CIPs#220 (comment) IMO we should either include this in v3.0.0-rc0 or close it b/c not worth it to break these constant names after that. (cherry picked from commit f217064)
Motivation: celestiaorg/CIPs#220 (comment)
IMO we should either include this in v3.0.0-rc0 or close it b/c not worth it to break these constant names after that.