Fee work #279
Fee work #279
Conversation
|
Warning Rate limit exceeded@cbrit has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 3 minutes and 31 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 ignored due to path filters (7)
📒 Files selected for processing (22)
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 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. 🔧 buf (1.47.2)proto/steward/v4/cellar_v2.protoConfig file YAML parsing error: yaml: unmarshal errors: WalkthroughThis pull request introduces a comprehensive update to the Steward project, focusing on enhancing fee management and job scheduling capabilities across multiple Cellar contract versions. The changes span various components, including Solidity contracts, Go and Rust implementations, configuration files, and integration tests. Key modifications include adding new methods for managing strategist platform cuts, deprecating existing fee-related functions, and implementing a new background job for updating platform cut configurations. Changes
Sequence DiagramsequenceDiagram
participant Jobs Thread
participant Blockchain
participant Cellar V2.2
participant Cellar V2.5
participant Scheduler
Jobs Thread->>Blockchain: Get Latest Block Height
Blockchain-->>Jobs Thread: Return Block Height
alt Block Height Meets Criteria
Jobs Thread->>Cellar V2.2: Prepare Platform Cut Update
Jobs Thread->>Cellar V2.5: Prepare Platform Cut Update
Jobs Thread->>Scheduler: Schedule Cork for V2.2
Scheduler-->>Jobs Thread: Confirm Scheduling
Jobs Thread->>Scheduler: Schedule Cork for V2.5
Scheduler-->>Jobs Thread: Confirm Scheduling
end
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: 7
🧹 Nitpick comments (8)
src/jobs.rs (3)
27-33: Suggestion to improve error handling logicThe current code uses
latest_block_height.is_err()and then immediately callslatest_block_height.unwrap()within the same condition. While Rust's short-circuiting logical operators ensure safety here, it might be clearer and more robust to refactor this using pattern matching to explicitly handle both cases.Consider refactoring to:
if let Ok(height) = latest_block_height { if height < target_height { if let Err(err) = update_strategist_platform_cut::run(target_height, cellars_v2_2, cellars_v2_5).await { warn!("job failed: update strategist platform cut: {:?}", err); } } } else { warn!("failed to get latest block height. running jobs just in case."); if let Err(err) = update_strategist_platform_cut::run(target_height, cellars_v2_2, cellars_v2_5).await { warn!("job failed: update strategist platform cut: {:?}", err); } }This refactoring explicitly handles both the
OkandErrcases, improving readability and reducing the risk of future errors.
115-131: Refactor duplicated functions to improve maintainabilityThe functions
set_strategist_platform_cut_2_2_callandset_strategist_platform_cut_2_5_callhave similar implementations with only minor differences. Refactoring these into a single generic function can reduce code duplication and improve maintainability.Consider the following refactor:
fn set_strategist_platform_cut_call<T>(call_constructor: T) -> Result<Vec<u8>, Error> where T: Fn(u64) -> CellarCall, { let call = call_constructor(500_000_000_000_000_000u64); Ok(call.encode()) }Then adjust your existing functions:
fn set_strategist_platform_cut_2_2_call() -> Result<Vec<u8>, Error> { set_strategist_platform_cut_call(|cut| { CellarV2_2Calls::SetStrategistPlatformCut(V2_2_SetStrategistPlatformCutCall { cut }) }) } fn set_strategist_platform_cut_2_5_call() -> Result<Vec<u8>, Error> { set_strategist_platform_cut_call(|cut| { CellarV2_5Calls::SetStrategistPlatformCut(V2_5_SetStrategistPlatformCutCall { cut }) }) }This reduces redundancy and centralizes the logic for setting the strategist platform cut.
73-113: Refactor similar scheduling functions to a generic functionThe functions
schedule_2_5_corksandschedule_2_2_corksshare similar logic. Creating a generic scheduling function can reduce duplication and make future maintenance easier.Here's how you might implement it:
async fn schedule_corks( target_height: u64, cellars: Vec<String>, get_call_fn: fn() -> Result<Vec<u8>, Error>, ) -> Result<(), Error> { let call = get_call_fn()?; for cellar_address in cellars { let cork = Cork { encoded_contract_call: call.clone(), target_contract_address: cellar_address.clone(), }; let response = somm_send::schedule_cork(cork, target_height).await?; info!( "response from scheduling cork for {}: {:?}", cellar_address, response ); } Ok(()) }You can then replace the original functions with calls to
schedule_corks:async fn schedule_2_5_corks( target_height: u64, cellars_v2_5: Vec<String>, ) -> Result<(), Error> { schedule_corks(target_height, cellars_v2_5, set_strategist_platform_cut_2_5_call).await } async fn schedule_2_2_corks( target_height: u64, cellars_v2_2: Vec<String>, ) -> Result<(), Error> { schedule_corks(target_height, cellars_v2_2, set_strategist_platform_cut_2_2_call).await }This approach reduces code duplication and enhances readability.
integration_tests/cellar_v2_2_abi.go (1)
410-429: Add documentation for the newSetStrategistPlatformCutfunctionsThe new
SetStrategistPlatformCutfunctions lack in-code documentation explaining their purpose and usage.Consider adding comments to describe the functionality of these methods, which can aid future developers in understanding the codebase.
integration_tests/jobs_test.go (2)
57-57: Consider reducing the test timeoutThe 180-second timeout with 5-second polling intervals might be excessive for a test. Consider if a shorter duration would be sufficient, or document why such a long timeout is necessary.
24-24: Extract magic numbers into named constantsThe fee values (750000000000000000, 500000000000000000) are used multiple times. Consider extracting these into named constants for better readability and maintenance.
+const ( + initialStrategistPlatformCut = 750000000000000000 + updatedStrategistPlatformCut = 500000000000000000 +)Also applies to: 31-31, 51-51
integration_tests/ethereum/contracts/MockCellarV2.5.sol (1)
51-56: Consider making initial fee values configurableThe initial values for
strategistPlatformCutandplatformFeeare hardcoded. Consider making these configurable through the constructor for flexibility.- FeeData public feeData = FeeData({ - strategistPlatformCut: 0.75e18, - platformFee: 0.01e18, - lastAccrual: 0, - strategistPayoutAddress: address(0) - }); + FeeData public feeData; + + constructor(uint64 initialPlatformCut, uint64 initialPlatformFee) Owned(msg.sender) { + feeData = FeeData({ + strategistPlatformCut: initialPlatformCut, + platformFee: initialPlatformFee, + lastAccrual: 0, + strategistPayoutAddress: address(0) + }); + }src/utils.rs (1)
253-271: Consider adding connection timeout.The function handles errors well, but could benefit from a connection timeout to prevent hanging in case of network issues.
- let mut client = TendermintClient::connect(grpc_url).await?; + let mut client = TendermintClient::connect(grpc_url) + .timeout(Duration::from_secs(30)) + .await?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
.github/workflows/integration_tests.ymlis excluded by!**/*.ymlCargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.tomlcrates/steward-proto/src/gen/descriptor.binis excluded by!**/*.bin,!**/gen/**,!**/*.bin,!**/gen/**crates/steward-proto/src/gen/steward.v4.rsis excluded by!**/gen/**,!**/gen/**integration_tests/ethereum/contracts/MockCellarV2.2.jsonis excluded by!**/*.jsonintegration_tests/ethereum/contracts/MockCellarV2.5.jsonis excluded by!**/*.json
📒 Files selected for processing (20)
Dockerfile(1 hunks)Makefile(1 hunks)hash_proto(1 hunks)integration_tests/cellar_v2_2_abi.go(4 hunks)integration_tests/cellar_v2_5_abi.go(1 hunks)integration_tests/ethereum/contracts/MockCellarV2.2.sol(1 hunks)integration_tests/ethereum/contracts/MockCellarV2.5.sol(1 hunks)integration_tests/ethereum/hardhat.config.ts(2 hunks)integration_tests/jobs_test.go(1 hunks)integration_tests/setup_test.go(5 hunks)proto/steward/v4/cellar_v2.proto(10 hunks)src/cellars/cellar_v2.rs(2 hunks)src/cellars/cellar_v2_2.rs(2 hunks)src/cellars/cellar_v2_5.rs(2 hunks)src/commands/start.rs(2 hunks)src/config.rs(3 hunks)src/error.rs(2 hunks)src/jobs.rs(1 hunks)src/lib.rs(1 hunks)src/utils.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- hash_proto
🧰 Additional context used
🪛 GitHub Actions: Rust tests
src/config.rs
[error] 320-326: This impl can be derived. Use #[derive(Default)] instead of manual implementation.
[error] 336-344: This impl can be derived. Use #[derive(Default)] instead of manual implementation.
🔇 Additional comments (18)
src/jobs.rs (1)
58-60: Verify the hardcoded cellar addressesThe cellar addresses
RYETH_CELLAR_ADDRESSandTURBO_STETH_CELLAR_ADDRESSare currently set to"0x0000000000000000000000000000000000000000", which is the zero address. Please verify that these are the intended addresses for production use.If these are placeholder addresses, consider fetching the correct addresses from a configuration file or environment variables to avoid potential issues in a production environment.
integration_tests/cellar_v2_5_abi.go (1)
1-3: Ensure generated bindings are up to date with the contract ABISince this file is auto-generated, please confirm that the ABI used for generating these bindings matches the latest version of the
CellarV25contract to avoid any potential mismatches.You can regenerate the bindings using the latest contract ABI to ensure consistency.
integration_tests/cellar_v2_2_abi.go (1)
39-39: Confirm the updated ABI includes all necessary changesThe
CellarV22MetaData's ABI has been updated to include new functions and events. Please verify that these changes align with the corresponding Solidity contract to prevent runtime errors due to mismatches.Ensure that the Solidity contract and the Go bindings are synchronized by regenerating the bindings if the contract has changed.
src/commands/start.rs (1)
26-26: Verify thread lifecycle managementThe new jobs thread is started without capturing its
JoinHandle, similar to other threads. While this is consistent with the existing pattern, consider if explicit lifecycle management would be beneficial for graceful shutdowns.integration_tests/ethereum/hardhat.config.ts (1)
48-51: LGTM! Deployment setup follows existing patterns.The deployment and ownership transfer for CellarV2_5 contract is implemented consistently with other Cellar contracts.
Also applies to: 80-86
src/lib.rs (1)
21-21: LGTM! Clean module addition.The jobs module is properly declared following Rust module system conventions.
src/error.rs (1)
52-54: LGTM! Well-structured error variants.The new error variants are properly defined with clear error messages and follow the existing error handling patterns.
Also applies to: 97-98
src/utils.rs (1)
102-104: LGTM! Consistent error helper function.The sp_disabled_call_error function follows the established pattern for error helper functions.
src/config.rs (2)
54-54: LGTM! New jobs configuration field added.The addition of the
jobsfield toStewardConfigaligns with the PR's objective of enhancing job scheduling capabilities.
105-105: LGTM! Default implementation updated.The default implementation correctly initializes the new
jobsfield.src/cellars/cellar_v2.rs (2)
143-152: LGTM! SetPlatformFee correctly disabled.The function now returns an appropriate error message indicating that it's no longer available.
154-164: LGTM! SetStrategistPlatformCut correctly disabled.The function now returns an appropriate error message indicating that it's no longer available.
src/cellars/cellar_v2_2.rs (1)
227-237: LGTM! SetStrategistPlatformCut correctly disabled.The function now returns an appropriate error message indicating that it's no longer available.
src/cellars/cellar_v2_5.rs (1)
313-323: LGTM! SetStrategistPlatformCut correctly disabled for strategist function.The function now returns an appropriate error message indicating that it's no longer available as a strategist function, while the governance version remains functional.
integration_tests/setup_test.go (2)
76-77: Consider using dynamic address assignment.The cellar addresses are hardcoded instead of being dynamically retrieved from contract deployment logs. This could make the tests brittle if the contract addresses change.
Run this script to check if these addresses match the deployed contracts:
916-924: Verify the job configuration.The steward configuration includes a new job for updating strategist platform cuts, but the height parameter might need adjustment based on the test requirements.
Run this script to check the job configuration usage:
✅ Verification successful
The height=30 configuration is appropriate for testing.
The test configuration uses a low block height (30) which is suitable for the test environment, while the production code defaults to a much higher value (18000000). This difference is intentional and correct, as it allows for quick test execution while maintaining the same functional behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the job configuration usage. # Search for height parameter usage rg -A 5 "height = 30" # Search for platform cut updates rg "update_strategist_platform_cut"Length of output: 1190
Script:
#!/bin/bash # Check the implementation details of the update_strategist_platform_cut module ast-grep --pattern 'mod update_strategist_platform_cut { $$$ }' # Look for any height-related constants rg "TARGET_HEIGHT|target_height" -A 3 src/Length of output: 7113
Dockerfile (1)
4-4: LGTM! Good security practice.Adding the
--lockedflag tocargo installensures reproducible builds by using exact versions from Cargo.lock, which is a good security practice.Makefile (1)
57-60: LGTM! Well-structured test target.The new test target follows the established pattern and includes proper cleanup handling.
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
CellarV2_5contract with enhanced fee management capabilitiesImprovements
Deprecations
SetPlatformFeeandSetStrategistPlatformCutfunctions across multiple contract versionsBug Fixes