Get target rate per minute from smart contract #610
Conversation
WalkthroughThis pull request introduces 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) 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (11)
🚧 Files skipped from review as they are similar to previous changes (9)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🔇 Additional comments (12)
✨ Finishing Touches
🪧 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 (
|
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.
neekolas
force-pushed
the
03-10-calculate_congestion_from_last_5_minutes_of_data
branch
from
3bb547d to
0355ea6
Compare
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
from
b4baa55 to
2f6df04
Compare
neekolas
force-pushed
the
03-10-calculate_congestion_from_last_5_minutes_of_data
branch
from
0355ea6 to
ca7f929
Compare
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
from
2f6df04 to
834af9a
Compare
neekolas
force-pushed
the
03-10-calculate_congestion_from_last_5_minutes_of_data
branch
from
ca7f929 to
ba4f95a
Compare
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
from
834af9a to
0b0a1be
Compare
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
from
0b0a1be to
8172dbd
Compare
neekolas
force-pushed
the
03-10-calculate_congestion_from_last_5_minutes_of_data
branch
from
ba4f95a to
5d90acd
Compare
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
from
8172dbd to
abbddbf
Compare
neekolas
force-pushed
the
03-10-calculate_congestion_from_last_5_minutes_of_data
branch
from
5d90acd to
06a43a9
Compare
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
2 times, most recently
from
fb029de to
750124e
Compare
neekolas
force-pushed
the
03-10-calculate_congestion_from_last_5_minutes_of_data
branch
2 times, most recently
from
1349721 to
8ba74c4
Compare
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
2 times, most recently
from
32f3403 to
e1b10e8
Compare
neekolas
force-pushed
the
03-10-calculate_congestion_from_last_5_minutes_of_data
branch
from
8ba74c4 to
a7b5cff
Compare
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
2 times, most recently
from
1b4d781 to
6d866c9
Compare
| Rates({ | ||
| messageFee: messageFee, | ||
| storageFee: storageFee, | ||
| congestionFee: congestionFee, | ||
| startTime: startTime, | ||
| targetRatePerMinute: targetRatePerMinute | ||
| }) |
There was a problem hiding this comment.
nit: I believe we're storing the target rate in Rates to help with the calculations when attesting to a report, is the assumption correct?
I'm ok with this, it just seems a bit off at first as the target rate could be a different entity that changes independently from the fees.
There was a problem hiding this comment.
I thought about giving the target rate its own smart contract. But having them all update in sync (at the same start time) makes the system a little cleaner and easier to model. One "fee epoch" instead of multiple.
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
from
6d866c9 to
8cdd72b
Compare
neekolas
force-pushed
the
03-10-calculate_congestion_from_last_5_minutes_of_data
branch
from
a7b5cff to
aec9180
Compare
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
from
8cdd72b to
dc91f64
Compare
neekolas
force-pushed
the
03-10-calculate_congestion_from_last_5_minutes_of_data
branch
from
aec9180 to
13db382
Compare
neekolas
changed the base branch from
03-10-calculate_congestion_from_last_5_minutes_of_data
to
graphite-base/610
neekolas
force-pushed
the
03-10-get_target_rate_per_minute_from_smart_contract
branch
from
dc91f64 to
73db2ee
Compare
neekolas
deleted the
03-10-get_target_rate_per_minute_from_smart_contract
branch
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