Migrate to V4 rates contract #833
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
""" WalkthroughThis update introduces a new CLI command "add-rates" for setting fee rates, extends the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant RatesAdmin
participant ParameterContract
participant RatesContract
User->>CLI: invoke "add-rates" with fees
CLI->>RatesAdmin: AddRates(fees)
RatesAdmin->>ParameterContract: set message, storage, congestion, target rates
ParameterContract-->>RatesAdmin: emit ParameterSet events
RatesAdmin->>RatesContract: UpdateRates(fees)
RatesContract-->>RatesAdmin: emit RatesUpdated event
RatesAdmin-->>CLI: success or error
CLI-->>User: output result
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.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
| return err | ||
| } | ||
|
|
||
| fromIndex := big.NewInt(0) |
There was a problem hiding this comment.
not starting from 0 is an optimization that was not possible before I believe.
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![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 (3)
pkg/blockchain/registryAdmin.go (1)
258-258: Verify logging consistency with stored value.The code stores a
uint8value in the last byte of the 32-byte array (line 241:value[31] = limit), but the logging decodes the entire 32-byte array as auint64. While this may be technically correct for the new encoding scheme, ensure this logging approach accurately represents the stored value to avoid confusion in logs.pkg/fees/contractRates.go (1)
113-140: Good pagination logic with minor optimization opportunity.The pagination implementation correctly uses persistent
currentIndexto enable incremental loading. However, line 140 has a redundant assignment sincebig.Int.Add()modifies the receiver in-place.Consider simplifying line 140:
- c.currentIndex = c.currentIndex.Add(c.currentIndex, toFetch) + c.currentIndex.Add(c.currentIndex, toFetch)Note: This implementation assumes rates are append-only (never modified or removed). This is reasonable for typical rate contracts where rates represent historical periods.
pkg/blockchain/ratesAdmin.go (1)
93-101: Minor: Error message accuracy.The error messages say "must be positive" but the validation allows zero values. Consider updating for clarity.
if rates.MessageFee < 0 { - return nil, errors.New("rates.messageFee must be positive") + return nil, errors.New("rates.messageFee must be non-negative") } if rates.StorageFee < 0 { - return nil, errors.New("rates.storageFee must be positive") + return nil, errors.New("rates.storageFee must be non-negative") } if rates.CongestionFee < 0 { - return nil, errors.New("rates.congestionFee must be positive") + return nil, errors.New("rates.congestionFee must be non-negative") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/cli/main.go(13 hunks)pkg/blockchain/ratesAdmin.go(2 hunks)pkg/blockchain/ratesAdmin_test.go(1 hunks)pkg/blockchain/registryAdmin.go(1 hunks)pkg/config/cliOptions.go(2 hunks)pkg/fees/contractRates.go(3 hunks)pkg/fees/contractRates_test.go(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/fees/contractRates_test.go (1)
pkg/abi/rateregistry/RateRegistry.go (1)
IRateRegistryRates(33-39)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Code Review
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Upgrade Tests
- GitHub Check: Test (Node)
🔇 Additional comments (21)
pkg/config/cliOptions.go (2)
14-18: LGTM! Consistent pattern implementation.The new
RateRegistryAdminOptionsstruct follows the same pattern asNodeRegistryAdminOptions, maintaining consistency in the codebase.
60-65: LGTM! Improved structure and validation.The refactoring properly embeds the admin options and makes all fee fields required, which improves input validation and follows CLI best practices.
pkg/fees/contractRates_test.go (3)
30-30: LGTM! Proper initialization for new pagination field.The addition of
currentIndex: big.NewInt(0)correctly initializes the new pagination state field.
57-57: LGTM! Appropriate test method changes.Changing from
fetcher.Start()tofetcher.refreshData()allows direct testing of the data fetching logic without the background goroutine, which is more appropriate for unit tests.Also applies to: 83-83, 109-109, 144-144, 162-162, 194-194, 199-199
177-201: LGTM! Comprehensive test for incremental loading.The new
TestCanContinuetest effectively validates the incremental loading behavior when the total rate count increases, ensuring the pagination logic works correctly across multiple refresh cycles.pkg/fees/contractRates.go (2)
50-50: LGTM! Proper field addition for pagination state.The addition of the
currentIndexfield and its initialization enables persistent pagination state across multiple refresh calls.Also applies to: 74-74
111-111: LGTM! Preserves existing rates during incremental loading.Appending existing rates ensures that previously loaded data is preserved when fetching additional rates incrementally.
pkg/blockchain/ratesAdmin_test.go (6)
15-34: LGTM! Well-structured test helper.The helper function properly sets up the test environment with appropriate error handling.
36-48: LGTM! Good coverage of the happy path.The test validates successful rate addition with reasonable values.
50-67: LGTM! Comprehensive validation testing.The test properly validates that negative fees are rejected with appropriate error messages.
69-79: LGTM! Validates zero rates are acceptable.Good edge case testing - zero rates should be allowed as they may represent free operations.
81-91: LGTM! Excellent boundary value testing.Tests maximum values to ensure the system handles large numbers without overflow issues.
93-108: LGTM! Good idempotency testing.Tests that rates can be added multiple times without errors, which is important for system resilience.
cmd/cli/main.go (4)
9-9: LGTM! Appropriate imports for fee validation and currency conversion.The imports support the new rate management functionality.
Also applies to: 16-17
103-105: LGTM! CLI command properly integrated.The new "add-rates" command follows the existing CLI pattern.
Also applies to: 650-652
665-699: LGTM! Clear function naming and consistent parameter usage.The rename to
setupNodeRegistryAdminbetter distinguishes it from the newsetupRateRegistryAdminfunction.
701-736: LGTM! Well-structured helper function.The function properly sets up the rate registry admin with appropriate error handling.
pkg/blockchain/ratesAdmin.go (4)
20-25: LGTM! Well-defined parameter key constants.The constants follow a consistent naming pattern and clearly identify rate registry parameters.
32-69: LGTM! Proper multi-contract initialization.The struct and constructor correctly support the two-contract architecture for parameter and rate management.
71-174: LGTM! Robust two-phase transaction implementation.The method properly handles the parameter setting and rate update sequence with comprehensive event logging and graceful "NoChange" error handling.
176-184: LGTM! Correct encoding/decoding implementation.The helper functions properly handle uint64 values in 32-byte arrays using big-endian encoding, which is standard for Ethereum parameter storage.
Add V4 rates contract support by implementing
|
Implement V4 rates contract integration by adding rate management functionality to CLI and blockchain packages
add-ratesCLI command in main.go for managing fee rates through the rate registryRatesAdminstruct in ratesAdmin.go with parameter contract integration and rate validationcurrentIndextracking📍Where to Start
Start with the
addRatesfunction in main.go which implements the new CLI command functionality and shows the high-level flow of rate management.Macroscope summarized f7e022b.
Summary by CodeRabbit