Add admin functions for rates contract #575
Conversation
WalkthroughThis pull request introduces new functionality to interact with a RatesManager smart contract. A new file defines a Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant RA as RatesAdmin
participant RM as RatesManager Contract
participant Tx as Transaction Pool
Client->>RA: Call AddRates(ctx, rates)
RA->>RM: Invoke AddRates method with tx options
RM-->>Tx: Submit transaction for confirmation
Tx--)RM: Return transaction status
RA-->>Client: Respond with transaction result/error
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
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 (
|
|
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: 0
🧹 Nitpick comments (4)
pkg/testutils/contracts.go (2)
129-134: Error handling looks good, but consider refactoring initialization logic.The addition of proper error handling after each operation (deployment, contract creation, and initialization) is great. This enhances the robustness of the RatesManager contract deployment process.
Consider extracting the initialization logic to a separate helper function since it's only applied to the RatesManager contract but not to other contracts. This would make the switch statement more consistent and easier to maintain.
139-139: Redundant error check for the RATES_MANAGER_CONTRACT_NAME case.There's already error checking on lines 130, 132, and 134 specifically for the RatesManager contract case, making this general error check redundant for that specific case.
This doesn't affect functionality, but it's worth noting for code clarity.
pkg/blockchain/ratesAdmin_test.go (1)
36-54: Test covers the happy path, but lacks error case testing.The test successfully verifies that rates can be added correctly and that the returned rates match the expected values.
Consider adding tests for error cases in the future, such as:
- Adding a rate with a start time earlier than the last rate
- Handling transaction failures
- Trying to add rates with invalid parameters
+ func TestAddRatesWithInvalidStartTime(t *testing.T) { + // Test adding a rate with a start time earlier than the existing rate + } + + func TestAddRatesTransactionFailure(t *testing.T) { + // Test handling of transaction failures + }pkg/blockchain/ratesAdmin.go (1)
54-84: AddRates implementation looks correct, but consider making timeouts configurable.The method correctly sets up the transaction, calls the contract method, and waits for confirmation.
Consider making the WaitForTransaction timeouts configurable or defined as constants at the package level instead of hardcoding them:
- 2*time.Second, - 250*time.Millisecond, + defaultPollInterval, + defaultRetryInterval,With constants defined at the package level:
const ( defaultPollInterval = 2 * time.Second defaultRetryInterval = 250 * time.Millisecond )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/blockchain/ratesAdmin.go(1 hunks)pkg/blockchain/ratesAdmin_test.go(1 hunks)pkg/testutils/contracts.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Test (Node)
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
🔇 Additional comments (4)
pkg/blockchain/ratesAdmin_test.go (1)
14-34: Helper function looks good, but consider adding error handling options.The
buildRatesAdminfunction is well-structured and properly sets up the testing environment with appropriate error checking at each step.pkg/blockchain/ratesAdmin.go (3)
20-25: Well-structured RatesAdmin type with clear responsibilities.The
RatesAdminstruct has all the necessary components to interact with the RatesManager contract.
27-47: Constructor is well-implemented with proper error handling.The
NewRatesAdminfunction correctly initializes all components of theRatesAdminstruct and handles potential errors during contract creation.
49-53: Good documentation, but contract constraint is only documented, not enforced.The comment correctly indicates that "The new rate must have a later start time than the last rate in the contract," which is an important constraint.
Is this constraint enforced by the contract itself? If not, consider adding validation in the Go code to prevent transactions that would fail at the contract level.
neekolas
force-pushed
the
02-28-add_admin_functions_for_rates_contract
branch
from
5c22252 to
32ee962
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/blockchain/ratesAdmin_test.go (4)
18-19: Misleading comment - inconsistent with implementation.The comment mentions setting "the nodes contract address to a random smart contract" but the code is actually setting the RatesManager contract address. This suggests the comment might have been copied from another test file without proper adaptation.
- // Set the nodes contract address to a random smart contract instead of the fixed deployment + // Set the rates manager contract address to a new deployment for testing contractsOptions.RatesManagerContractAddress = testutils.DeployRatesManagerContract(t)
36-54: Test coverage could be expanded.The current test only covers the happy path for adding rates. Consider adding tests for edge cases and error conditions:
- Adding rates with invalid values
- Authorization failures
- Attempting to add rates when the contract is in an invalid state
- Behavior when multiple rates are added sequentially
Also, consider adding a test timeout to prevent hanging in case of issues:
func TestAddRates(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() ratesAdmin := buildRatesAdmin(t) rates := ratesmanager.RatesManagerRates{ MessageFee: 100, StorageFee: 200, CongestionFee: 300, StartTime: 1000, } - err := ratesAdmin.AddRates(context.Background(), rates) + err := ratesAdmin.AddRates(ctx, rates) require.NoError(t, err)
39-44: Use descriptive constants instead of magic numbers.The test uses magic numbers for rate values and timestamp. Consider using named constants or variables to make the test more readable and the intent clearer.
+ const ( + testMessageFee = 100 + testStorageFee = 200 + testCongestionFee = 300 + testStartTime = 1000 // Unix timestamp + ) + rates := ratesmanager.RatesManagerRates{ - MessageFee: 100, - StorageFee: 200, - CongestionFee: 300, - StartTime: 1000, + MessageFee: testMessageFee, + StorageFee: testStorageFee, + CongestionFee: testCongestionFee, + StartTime: testStartTime, }
1-55: Consider adding more comprehensive test validation.While the basic functionality is tested, consider enhancing validation to ensure the rates system works correctly under various scenarios:
- Test retrieving rates by specific index
- Test adding multiple rates and verifying pagination works correctly
- Verify rate value boundaries and constraints
Also, you may want to add a test for the
Initializemethod that likely exists in the RatesAdmin implementation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/blockchain/ratesAdmin.go(1 hunks)pkg/blockchain/ratesAdmin_test.go(1 hunks)pkg/testutils/contracts.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/testutils/contracts.go
- pkg/blockchain/ratesAdmin.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Graphite / mergeability_check
- GitHub Check: Build pre-baked anvil-xmtpd
- GitHub Check: Test (Node)
tl;dr
Summary by CodeRabbit
New Features
Tests
Chores