solidity: adjust min and max sizes dynamically #451
Conversation
|
Warning Rate limit exceeded@fbac has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThe pull request modifies two smart contracts, Changes
Sequence DiagramsequenceDiagram
participant Admin
participant Contract
participant Validator
Admin->>Contract: setMinPayloadSize(newMin)
Contract-->>Admin: Validate newMin
alt Invalid Size
Contract->>Admin: Revert with InvalidMinPayloadSize
else Valid Size
Contract->>Contract: Update minPayloadSize
end
Admin->>Contract: setMaxPayloadSize(newMax)
Contract-->>Admin: Validate newMax
alt Invalid Size
Contract->>Admin: Revert with InvalidMaxPayloadSize
else Valid Size
Contract->>Contract: Update maxPayloadSize
end
Validator->>Contract: Check payload size
Contract-->>Validator: Validate against current min/max
Possibly related PRs
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
CodeRabbit Configuration File (
|
![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
🧹 Nitpick comments (5)
contracts/pkg/identityupdates/IdentityUpdates.go (2)
610-629: Consider adding parameter validation.While the contract handles validation, consider adding Go-level validation for the
_maxPayloadSizeparameter to fail fast and provide better error messages. This could prevent unnecessary contract calls with invalid values.func (_IdentityUpdates *IdentityUpdatesTransactor) SetMaxPayloadSize(opts *bind.TransactOpts, _maxPayloadSize *big.Int) (*types.Transaction, error) { + // Validate maxPayloadSize + if _maxPayloadSize == nil { + return nil, errors.New("maxPayloadSize cannot be nil") + } + if _maxPayloadSize.Sign() <= 0 { + return nil, errors.New("maxPayloadSize must be positive") + } return _IdentityUpdates.contract.Transact(opts, "setMaxPayloadSize", _maxPayloadSize) }
631-650: Consider adding parameter validation and size relationship check.Similar to SetMaxPayloadSize, consider adding Go-level validation. Additionally, if possible, add a check to ensure the min size doesn't exceed the max size.
func (_IdentityUpdates *IdentityUpdatesTransactor) SetMinPayloadSize(opts *bind.TransactOpts, _minPayloadSize *big.Int) (*types.Transaction, error) { + // Validate minPayloadSize + if _minPayloadSize == nil { + return nil, errors.New("minPayloadSize cannot be nil") + } + if _minPayloadSize.Sign() < 0 { + return nil, errors.New("minPayloadSize cannot be negative") + } + + // Get current max size for validation + maxSize, err := _IdentityUpdates.contract.Call(opts, &[]interface{}{}, "MAX_PAYLOAD_SIZE") + if err != nil { + return nil, fmt.Errorf("failed to get max size: %v", err) + } + if _minPayloadSize.Cmp(maxSize.(*big.Int)) > 0 { + return nil, errors.New("minPayloadSize cannot exceed maxPayloadSize") + } return _IdentityUpdates.contract.Transact(opts, "setMinPayloadSize", _minPayloadSize) }contracts/src/IdentityUpdates.sol (3)
25-26: Address naming convention warnings.The pipeline failures indicate naming convention issues with the state variables. Consider using mixedCase for public state variables.
- uint256 public MIN_PAYLOAD_SIZE; - uint256 public MAX_PAYLOAD_SIZE; + uint256 public minPayloadSize; + uint256 public maxPayloadSize;Also applies to: 29-32, 34-35
92-99: Consider adding events for payload size changes.The
setMinPayloadSizefunction modifies contract state but doesn't emit any events. Consider adding events to track these changes for better transparency and off-chain monitoring.+ event MinPayloadSizeUpdated(uint256 oldSize, uint256 newSize); function setMinPayloadSize(uint256 _minPayloadSize) public onlyRole(DEFAULT_ADMIN_ROLE) { require(_minPayloadSize < MAX_PAYLOAD_SIZE, InvalidMinPayloadSize()); require(_minPayloadSize > 0, InvalidMinPayloadSize()); + uint256 oldSize = MIN_PAYLOAD_SIZE; MIN_PAYLOAD_SIZE = _minPayloadSize; + emit MinPayloadSizeUpdated(oldSize, _minPayloadSize); }
101-108: Consider adding events for payload size changes.The
setMaxPayloadSizefunction modifies contract state but doesn't emit any events. Consider adding events to track these changes for better transparency and off-chain monitoring.+ event MaxPayloadSizeUpdated(uint256 oldSize, uint256 newSize); function setMaxPayloadSize(uint256 _maxPayloadSize) public onlyRole(DEFAULT_ADMIN_ROLE) { require(_maxPayloadSize > MIN_PAYLOAD_SIZE, InvalidMaxPayloadSize()); require(_maxPayloadSize <= DEFAULT_MAX_PAYLOAD_SIZE, InvalidMaxPayloadSize()); + uint256 oldSize = MAX_PAYLOAD_SIZE; MAX_PAYLOAD_SIZE = _maxPayloadSize; + emit MaxPayloadSizeUpdated(oldSize, _maxPayloadSize); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/pkg/groupmessages/GroupMessages.go(2 hunks)contracts/pkg/identityupdates/IdentityUpdates.go(2 hunks)contracts/src/GroupMessages.sol(3 hunks)contracts/src/IdentityUpdates.sol(3 hunks)contracts/test/GroupMessage.t.sol(5 hunks)contracts/test/IdentityUpdates.t.sol(8 hunks)
🧰 Additional context used
🪛 GitHub Actions: Solidity
contracts/src/IdentityUpdates.sol
[warning] 34-34: Variable MIN_PAYLOAD_SIZE is not in mixedCase naming convention
[warning] 35-35: Variable MAX_PAYLOAD_SIZE is not in mixedCase naming convention
[warning] 29-29: Unused state variable: DEFAULT_MIN_PAYLOAD_SIZE is never used in IdentityUpdates
[warning] 32-32: Unused state variable: DEFAULT_MAX_PAYLOAD_SIZE is never used in IdentityUpdates
[warning] 35-35: MAX_PAYLOAD_SIZE should be declared as constant
[warning] 34-34: MIN_PAYLOAD_SIZE should be declared as constant
contracts/src/GroupMessages.sol
[warning] 34-34: Variable MIN_PAYLOAD_SIZE is not in mixedCase naming convention
[warning] 35-35: Variable MAX_PAYLOAD_SIZE is not in mixedCase naming convention
[warning] 29-29: Unused state variable: DEFAULT_MIN_PAYLOAD_SIZE is never used in GroupMessages
[warning] 32-32: Unused state variable: DEFAULT_MAX_PAYLOAD_SIZE is never used in GroupMessages
[warning] 35-35: MAX_PAYLOAD_SIZE should be declared as constant
[warning] 34-34: MIN_PAYLOAD_SIZE should be declared as constant
🔇 Additional comments (8)
contracts/pkg/identityupdates/IdentityUpdates.go (2)
34-35: LGTM: ABI and binary updates look correct.The ABI has been properly updated to include the new payload size management functions and error types.
Line range hint
1-1: Note: This is an auto-generated file.While we've suggested some improvements, remember that this file is auto-generated. Any changes should be made to the generator or the Solidity contract instead of modifying this file directly.
contracts/src/GroupMessages.sol (1)
25-26: Same recommendations as IdentityUpdates.sol apply.The changes in this file mirror those in IdentityUpdates.sol. The same recommendations regarding naming conventions and event emissions apply here.
Also applies to: 29-32, 34-35, 92-109
contracts/test/GroupMessage.t.sol (1)
90-180: LGTM! Comprehensive test coverage for payload size management.The test suite thoroughly covers:
- Authorization checks for admin-only functions
- Validation of size constraints
- State updates verification
- Error cases for invalid sizes
contracts/test/IdentityUpdates.t.sol (1)
90-180: Same comprehensive test coverage as GroupMessage.t.sol.The test changes mirror those in GroupMessage.t.sol, providing equally thorough coverage.
contracts/pkg/groupmessages/GroupMessages.go (3)
34-35: LGTM! ABI and binary updates look good.The ABI has been properly updated to include the new methods and error types for payload size management. The error handling is well-structured with custom error types.
610-630: LGTM! SetMaxPayloadSize method bindings are well-implemented.The implementation follows Go-Ethereum contract binding patterns with proper context handling and type safety.
631-651: LGTM! SetMinPayloadSize method bindings are well-implemented.The implementation follows Go-Ethereum contract binding patterns. However, it's important to verify the validation logic in the underlying contract.
Run the following script to check the validation logic in the Solidity contract:
✅ Verification successful
Validation for setMinPayloadSize is properly implemented
The contract enforces proper bounds checking:
- Prevents zero or negative values
- Ensures minPayloadSize < maxPayloadSize (4194304)
- Includes comprehensive test coverage for edge cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify validation logic for min/max payload size settings # Search for validation checks in the contract ast-grep --pattern $'function setMinPayloadSize(uint256 _minPayloadSize) { $$$ require($$$, $$$); $$$ }' # Search for related error handling rg -A 2 'InvalidMinPayloadSize|InvalidMaxPayloadSize|InvalidPayloadSize'Length of output: 49123
fbac
force-pushed
the
fix/adjust-min-max-message-sizes
branch
from
1a51005 to
bfd18ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
contracts/pkg/groupmessages/GroupMessages.go (1)
610-651: Ensure set accessor coverageThe newly introduced setter methods (
SetMaxPayloadSizeandSetMinPayloadSize) correctly align with the contract's dynamic payload size logic. It's a good practice to add test coverage to confirm that out-of-range values revert as expected and that valid updates succeed.contracts/src/IdentityUpdates.sol (2)
91-99: Consider emitting an event on setMinPayloadSizeAlthough this logic is correct and includes proper validations, consider emitting an event (e.g.,
MinPayloadSizeUpdated(oldValue, newValue)) for better on-chain auditability and transparency.
100-108: Add event for setMaxPayloadSizeSimilar to
setMinPayloadSize, consider emitting a dedicated event that logs the old value and the new value insetMaxPayloadSize(). This is helpful for contract monitoring and potential debugging.contracts/src/GroupMessages.sol (2)
92-99: Optionally track updates to minPayloadSizeThis function’s validations look correct. Emitting a
MinPayloadSizeUpdatedevent can help track changes over time for analytics or auditing.
100-109: setMaxPayloadSize improvementSimilarly, consider adding an event to log
_maxPayloadSize. This helps maintain a transparent record of changes.contracts/test/GroupMessage.t.sol (1)
135-180: Fix typo in function name.The function name has incorrect casing.
- function testSetmaxPayloadSize() public { + function testSetMaxPayloadSize() public {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/pkg/groupmessages/GroupMessages.go(3 hunks)contracts/pkg/identityupdates/IdentityUpdates.go(3 hunks)contracts/src/GroupMessages.sol(4 hunks)contracts/src/IdentityUpdates.sol(4 hunks)contracts/test/GroupMessage.t.sol(5 hunks)contracts/test/IdentityUpdates.t.sol(8 hunks)
🔇 Additional comments (21)
contracts/pkg/identityupdates/IdentityUpdates.go (5)
34-35: No issues found with the updated ABI and bin content.
This appears to be a correct reflection of the new contract interface and compiled bytecode.
329-359: The getter formaxPayloadSizelooks good.
This auto-generated binding correctly retrieves the on-chain value as a*big.Int.
360-390: The getter forminPayloadSizelooks good.
No issues spotted; it follows the same pattern asmaxPayloadSizeand should work as intended.
610-629: The setter formaxPayloadSizeis correct.
This transactional binding matches the contract method signature accurately.
631-650: The setter forminPayloadSizeis correct.
It mirrors theSetMaxPayloadSizelogic and accurately aligns with the Solidity method.contracts/pkg/groupmessages/GroupMessages.go (3)
329-359: Getter method naming is consistentDeclaring and exposing
MaxPayloadSizeas a view function aligns with the contract changes. It follows Go naming conventions and matches the underlying contract’s method.
360-390: Check usage references forMinPayloadSizeThe addition of
MinPayloadSizealso follows the naming convention. Ensure all references to the minimum size throughout the codebase are updated accordingly to avoid confusion with any removed constants.✅ Verification successful
MinPayloadSize implementation is consistent and complete
The codebase shows proper implementation of
MinPayloadSizewith no remaining references to old constants. Both GroupMessages and IdentityUpdates contracts use consistent naming and validation patterns.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search codebase for references to old constants or usage of minPayloadSize. ast-grep --pattern $'MIN_PAYLOAD_SIZE' # Checking if any old references remain ast-grep --pattern $'MinPayloadSize($_)' # Checking usageLength of output: 187
Script:
#!/bin/bash # Broader search for payload size references and usage rg -i "payload.?size" --type go --type solidity -A 2 rg "MinPayloadSize" --type go --type solidity -A 2 rg "min.?payload" --type go --type solidity -A 2Length of output: 255
Script:
#!/bin/bash # Search for payload size references in Go and Solidity files rg -i "payload.?size" -g "*.{go,sol}" -A 3 rg "MinPayloadSize" -g "*.{go,sol}" -A 3 rg "min.?payload" -g "*.{go,sol}" -A 3 # Search for any configuration or constant definitions fd -e go -e sol -x grep -l "payload" {} \; | xargs catLength of output: 84084
34-35: Validate ABI and Binary StringsThese assignments to
GroupMessagesMetaData.ABIandGroupMessagesMetaData.Binappear correct. As a precaution, confirm that these match the actual on-chain or compiled artifacts to avoid runtime mismatches.You can run a content comparison script to confirm the ABI and binary strings stay in sync with the contract's compiled output:
✅ Verification successful
ABI and Binary Strings Match Source Code Implementation
The ABI string correctly represents all functions, events, and errors from the GroupMessages contract and its inherited OpenZeppelin contracts. The binary string contains valid EVM bytecode with proper initialization for upgradeable contracts. Both artifacts are consistent with the Solidity source code (version 0.8.28).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Compare ABI and BIN with compiled results under a typical build directory. # Attempt to locate and compare .abi/.bin files (exact file paths may differ): fd "GroupMessages.json" | while read file; do jq '.abi' "$file" jq '.bytecode' "$file" doneLength of output: 35
Script:
#!/bin/bash # Search for Solidity source files and build artifacts related to GroupMessages echo "=== Searching for Solidity files ===" fd -e sol . echo -e "\n=== Searching for build artifacts ===" # Common build artifact patterns fd -e json -e abi -e bin "GroupMessages" echo -e "\n=== Checking contract directory structure ===" find . -type f -path "*/contracts/*" -o -path "*/artifacts/*" | grep -i "groupmessages"Length of output: 1131
Script:
#!/bin/bash echo "=== GroupMessages.sol source ===" cat contracts/src/GroupMessages.sol echo -e "\n=== Checking compiler settings ===" # Look for compiler version and settings in source files grep -h "pragma" contracts/src/GroupMessages.sol grep -h "SPDX-License" contracts/src/GroupMessages.sol echo -e "\n=== Extracting function signatures ===" # Extract function declarations grep -A 1 "function" contracts/src/GroupMessages.sol echo -e "\n=== Extracting events ===" # Extract event declarations grep "event" contracts/src/GroupMessages.solLength of output: 6727
contracts/src/IdentityUpdates.sol (4)
25-26: New custom errors enhance clarityDeclaring
InvalidMaxPayloadSize()andInvalidMinPayloadSize()clarifies error conditions. This is preferable to generic revert messages for debugging and end-user clarity.
29-33: Switch from constants to mutable state variablesReplacing the static constants with
minPayloadSizeandmaxPayloadSizeadds flexibility. Verify that you do not inadvertently introduce a security gap allowing the admin to set zero or excessively large values if that is outside intended constraints.
54-56: Reasonable default sizesInitializing
minPayloadSize = 104andmaxPayloadSize = 4_194_304is consistent with typical constraints. Confirm these values align with your system’s throughput and storage goals.
79-80: Validation usage is correctYour
requirecondition usingminPayloadSizeandmaxPayloadSizeproperly enforces payload size constraints. The custom errorInvalidPayloadSizecaptures detailed info for debugging.contracts/src/GroupMessages.sol (4)
25-26: Custom errors improve debuggingIntroducing
InvalidMaxPayloadSizeandInvalidMinPayloadSizefosters clearer revert reasons, easing troubleshooting.
29-34: Mutable payload size variablesUsing
minPayloadSizeandmaxPayloadSizeinstead of constants provides necessary flexibility. Monitor possible misuse if an admin sets contradictory values (e.g., min > max).
54-56: Default initialization seems appropriateSetting
minPayloadSize = 78andmaxPayloadSize = 4_194_304is representative and consistent with the intended range. Confirm that 78 bytes is a minimum that meets your functional requirements.
80-81: Ensure thorough revert coverageThe
requirewithInvalidPayloadSizecorrectly prevents messages outside the allowed boundaries. Double-check that extreme edge cases (like exactly 78 or exactly 4,194,304 bytes) pass or revert as expected.contracts/test/GroupMessage.t.sol (2)
32-32: LGTM! Consistent use of dynamic payload size methods.The changes correctly replace hardcoded constants with dynamic method calls to
minPayloadSize()andmaxPayloadSize(), making the tests more maintainable and adaptable to configuration changes.Also applies to: 41-41, 50-50, 65-65, 80-80
90-133: LGTM! Comprehensive test coverage for minimum payload size adjustments.The test function thoroughly validates:
- Authorization checks
- Successful size updates
- Message validation with old and new sizes
- Invalid size boundaries
contracts/test/IdentityUpdates.t.sol (3)
32-32: LGTM! Consistent test structure with GroupMessages.The changes correctly mirror the dynamic payload size approach used in
GroupMessage.t.sol, maintaining consistency across contracts.Also applies to: 41-41, 50-50, 65-65, 80-80
90-133: LGTM! Comprehensive test coverage for payload size adjustments.The test functions thoroughly validate the same scenarios as in
GroupMessage.t.sol:
- Authorization checks
- Successful size updates
- Message validation with old and new sizes
- Invalid size boundaries
This consistency in test coverage between contracts is excellent.
Also applies to: 135-180
209-211: LGTM! Consistent error handling.The error handling for unauthorized access is consistent across all test functions, using the correct selector and role.
Also applies to: 222-224, 243-245, 253-257, 282-284
Related to xmtp/smart-contracts#29
Summary by CodeRabbit
New Features
Improvements
Testing