Get target rate per minute from smart contract #610
Conversation
WalkthroughThis pull request adds a new field, Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Application
participant Admin as RatesAdmin
participant RM as RatesManager Contract
participant BC as Blockchain
CLI->>Admin: Prepare rates including TargetRatePerMinute via CLI options
Admin->>RM: Call AddRates(messageFee, storageFee, congestionFee, startTime, targetRatePerMinute)
RM-->>BC: Execute transaction with all rate parameters
BC-->>RM: Return transaction confirmation
RM->>Admin: Confirm rates addition (emit RatesAdded with targetRatePerMinute)
Admin->>CLI: Return success/failure status
Possibly related PRs
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)Error: can't load config: the Go language version (go1.23) used to build golangci-lint is lower than the targeted Go version (1.24) ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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
🧹 Nitpick comments (5)
contracts/src/RatesManager.sol (5)
18-18: Inconsistent ordering of fields in event declarationThe order of parameters in the
RatesAddedevent (targetRatePerMinutebeforestartTime) doesn't match the order in theRatesstruct declaration (wherestartTimeis beforetargetRatePerMinute). This inconsistency could lead to confusion during maintenance.Consider keeping the order consistent between the event parameters and struct fields:
- event RatesAdded(uint64 messageFee, uint64 storageFee, uint64 congestionFee, uint64 targetRatePerMinute, uint64 startTime); + event RatesAdded(uint64 messageFee, uint64 storageFee, uint64 congestionFee, uint64 startTime, uint64 targetRatePerMinute);
35-35: Missing documentation for new fieldThe
targetRatePerMinutefield has been added without any documentation explaining its purpose or constraints.Consider adding a comment to explain what this field represents and any constraints on its values.
77-78: Parameter order inconsistencyThe parameter order in the function signature (
startTimethentargetRatePerMinute) doesn't match the order in the struct initialization in lines 91-92 (wherestartTimeis assigned beforetargetRatePerMinute).To maintain consistency, consider either:
- function addRates(uint64 messageFee, uint64 storageFee, uint64 congestionFee, uint64 startTime, uint64 targetRatePerMinute) + function addRates(uint64 messageFee, uint64 storageFee, uint64 congestionFee, uint64 targetRatePerMinute, uint64 startTime)Or rearrange the field assignments in the struct initialization to match the parameter order.
86-94: Inconsistent field order in struct initializationThe order of fields in this struct initialization doesn't match the order of parameters in the function signature. In the function parameters,
startTimecomes beforetargetRatePerMinute, but in the initialization,startTimeis assigned beforetargetRatePerMinute.To maintain consistency, consider rearranging the field assignments to match the parameter order:
Rates({ messageFee: messageFee, storageFee: storageFee, congestionFee: congestionFee, - startTime: startTime, - targetRatePerMinute: targetRatePerMinute + targetRatePerMinute: targetRatePerMinute, + startTime: startTime })This would match the parameter order in the function signature.
96-96: Event emission parameter order inconsistencyThe order of parameters in the
RatesAddedevent emission (targetRatePerMinutebeforestartTime) is inconsistent with the parameter order in the function signature (wherestartTimeis beforetargetRatePerMinute).Consider adjusting the event emission to maintain consistent parameter ordering:
- emit RatesAdded(messageFee, storageFee, congestionFee, targetRatePerMinute, startTime); + emit RatesAdded(messageFee, storageFee, congestionFee, startTime, targetRatePerMinute);This would match the parameter order in the function signature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/cli/main.go(1 hunks)contracts/pkg/ratesmanager/RatesManager.go(8 hunks)contracts/src/RatesManager.sol(4 hunks)contracts/test/RatesManager.t.sol(3 hunks)pkg/blockchain/ratesAdmin.go(1 hunks)pkg/blockchain/ratesAdmin_test.go(1 hunks)pkg/config/cliOptions.go(1 hunks)pkg/fees/contractRates.go(1 hunks)pkg/fees/contractRates_test.go(2 hunks)pkg/fees/interface.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Test
contracts/test/RatesManager.t.sol
[error] 1-1: Tests failed: 3 tests failed (testAddRatesChronologicalOrder, testAddRatesValid, testGetRatesPagination) with errors related to InvalidStartTime and log mismatch.
🪛 GitHub Actions: Solidity
contracts/test/RatesManager.t.sol
[error] 1-1: Test failed: InvalidStartTime() in testAddRatesChronologicalOrder()
[error] 1-1: Test failed: log != expected log in testAddRatesValid()
[error] 1-1: Test failed: InvalidStartTime() in testGetRatesPagination()
🔇 Additional comments (21)
pkg/config/cliOptions.go (1)
46-46: LGTM - Field added for target rate per minuteThe addition of the
TargetRatefield to theAddRatesOptionsstruct is clean and follows the same pattern as other rate-related fields in the struct.pkg/fees/interface.go (1)
12-15: LGTM - New field for target rate per minute addedThe
TargetRatePerMinutefield has been properly added to theRatesstruct with an appropriate type and descriptive comment. The formatting changes to align the field types and comments also improve readability.pkg/blockchain/ratesAdmin_test.go (1)
39-45: LGTM - Test updated with target rate per minuteThe test has been properly updated to include the new
TargetRatePerMinutefield with a reasonable test value of100 * 60(which appears to convert a per-second rate to per-minute).pkg/fees/contractRates.go (1)
211-214: LGTM - Transform function updated with target rate per minuteThe
transformRatesfunction has been properly updated to include the newTargetRatePerMinutefield when creating aRatesstruct from the contract data.contracts/test/RatesManager.t.sol (3)
22-22: Good addition of targetRatePerMinute constantThe new constant is appropriately defined with a clear value calculation (100 * 60).
65-65: Function call parameters updated correctlyThe direct call to ratesManager.addRates includes the new targetRatePerMinute parameter, which is consistent with other changes.
41-41:✅ Verification successful
Verify event signature matches implementation
The expected event now includes targetRatePerMinute. Ensure this matches the actual event definition in the contract.
🏁 Script executed:
#!/bin/bash # Verify the RatesAdded event signature in the RatesManager contract grep -A 3 "event RatesAdded" contracts/src/RatesManager.solLength of output: 302
Verified: RatesAdded event signature is correctly updated
The event definition in
contracts/src/RatesManager.solmatches the emitted parameters in the test file. The expectedtargetRatePerMinuteis present and its type aligns with the implementation.cmd/cli/main.go (1)
425-431:✅ Verification successful
Properly updated struct initialization with new field
The RatesManagerRates struct initialization now includes the TargetRatePerMinute field, which is set from options.AddRates.TargetRate. This is consistent with the changes in other files.
🏁 Script executed:
#!/bin/bash # Verify that the TargetRate option is defined in the AddRatesOptions struct grep -A 10 "type AddRatesOptions struct" pkg/config/cliOptions.goLength of output: 807
Updated Struct Initialization Verified
The updated initialization in
cmd/cli/main.gocorrectly maps theTargetRatefromoptions.AddRatesto the newTargetRatePerMinutefield in theRatesManagerRatesstruct. Verification confirms that theTargetRatefield is properly defined in theAddRatesOptionsstruct inpkg/config/cliOptions.go.No further changes are necessary.
pkg/fees/contractRates_test.go (2)
34-42: Good update to test helper functionThe buildRates function has been properly updated to include the new TargetRatePerMinute field with a consistent value (100 * 60).
58-59: Good test coverage for new fieldThe test assertions for the new TargetRatePerMinute field ensure that the value is correctly set in the rates fetched from the contract.
pkg/blockchain/ratesAdmin.go (1)
54-69: Function implementation updated correctlyThe AddRates method has been properly updated to include the new rates.TargetRatePerMinute parameter in the contract call, which is consistent with the changes in other files.
contracts/pkg/ratesmanager/RatesManager.go (10)
33-39: Struct update looks goodThe
RatesManagerRatesstruct has been correctly updated to include the newTargetRatePerMinutefield in line with the Solidity contract changes.
43-43: ABI string updated correctlyThe ABI string has been properly updated to reflect the changes in the contract interface including the new parameter.
538-543: Function signature update looks goodThe
AddRatesfunction signature has been correctly updated to include the newtargetRatePerMinuteparameter, matching the changes in the Solidity contract.
547-550: Session method signature update looks goodThe
RatesManagerSession.AddRatesmethod signature has been properly updated to match the contract changes.
554-557: TransactorSession method signature update looks goodThe
RatesManagerTransactorSession.AddRatesmethod signature has been properly updated to match the contract changes.
1043-1049: Event struct update looks goodThe
RatesManagerRatesAddedevent struct has been correctly updated to include the newTargetRatePerMinutefield.
1051-1054: Event filter method update looks goodThe
FilterRatesAddedmethod comment has been updated correctly to reflect the new event signature.
1063-1066: Event watch method update looks goodThe
WatchRatesAddedmethod comment has been updated correctly to reflect the new event signature.
1100-1103: Event parse method update looks goodThe
ParseRatesAddedmethod comment has been updated correctly to reflect the new event signature.
1-2: Be cautious with auto-generated codeThis file is auto-generated and manual changes could be overwritten. Ensure that the source code generation templates have been updated to include the new field, as indicated at the top of the file.
Verify that the code generation process has been updated to include the new field by checking if there are changes to the generation templates or configurations in the PR.
| @@ -30,14 +31,14 @@ contract RatesTest is Test { | |||
| } | |||
|
|
|||
| function addRate(uint64 startTime) internal { | |||
| ratesManager.addRates(messageFee, storageFee, congestionFee, startTime); | |||
| ratesManager.addRates(messageFee, storageFee, congestionFee, targetRatePerMinute, startTime); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify function signature matches contract implementation
The addRate function has been updated to include targetRatePerMinute parameter. The pipeline failures suggest there might be issues with parameter ordering or event signatures.
🏁 Script executed:
#!/bin/bash
# Verify the addRates function signature in the RatesManager contract
grep -A 5 "function addRates" contracts/src/RatesManager.solLength of output: 380
Action: Correct parameter ordering in addRates call
The function signature in contracts/src/RatesManager.sol is defined as:
function addRates(
uint64 messageFee,
uint64 storageFee,
uint64 congestionFee,
uint64 startTime,
uint64 targetRatePerMinute
)However, the test in contracts/test/RatesManager.t.sol (line 34) calls the function as follows:
ratesManager.addRates(messageFee, storageFee, congestionFee, targetRatePerMinute, startTime);This is passing targetRatePerMinute before startTime, which does not match the contract implementation. Please update the test call to:
ratesManager.addRates(messageFee, storageFee, congestionFee, startTime, targetRatePerMinute);This should resolve the pipeline failures.
TL;DR
Added target rate per minute parameter to the rates manager contract and related components.
What changed?
TargetRatePerMinutefield to theRatesManagerRatesstructaddRates()function signature to accept the new parameterRatesAddedevent to include target rate--target-rateflagHow to test?
Why make this change?
To support rate limiting functionality by allowing configuration of target message processing rates per minute for each node. This enables better control over network congestion and resource utilization.
Summary by CodeRabbit
New Features
Tests