feat:Support parsing multicall in governance functions #255
Conversation
WalkthroughThe project introduced enhancements to support scheduled cork proposals with multicalls, expanding governance capabilities. It involved updating contract functionality and ABI definitions, adding new test types, and refining the code structure for better governance call handling. Additionally, the Changes
Related issues
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| s.Require().Eventuallyf(func() bool { | ||
| s.T().Log("querying add adaptor cellar event...") | ||
| ethClient, err := ethclient.Dial(fmt.Sprintf("http://%s", s.ethResource.GetHostPort("8545/tcp"))) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // For non-anonymous events, the first log topic is a keccak256 hash of the | ||
| // event signature. | ||
| eventSignature := []byte("AddAdaptorToCatalogue(address)") | ||
| mockEventSignatureTopic := crypto.Keccak256Hash(eventSignature) | ||
| query := ethereum.FilterQuery{ | ||
| FromBlock: nil, | ||
| ToBlock: nil, | ||
| Addresses: []common.Address{ | ||
| v2_2Cellar, | ||
| }, | ||
| Topics: [][]common.Hash{ | ||
| { | ||
| mockEventSignatureTopic, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| logs, err := ethClient.FilterLogs(context.Background(), query) | ||
| if err != nil { | ||
| ethClient.Close() | ||
| return false | ||
| } | ||
|
|
||
| vault_abi, err := CellarV22MetaData.GetAbi() | ||
| s.Require().NoError(err) | ||
|
|
||
| if len(logs) > 0 { | ||
| s.T().Logf("found %d logs!", len(logs)) | ||
| for _, log := range logs { | ||
| if len(log.Data) > 0 { | ||
| var event CellarV22AddAdaptorToCatalogue | ||
| err := vault_abi.UnpackIntoInterface(&event, "AddAdaptorToCatalogue", log.Data) | ||
| s.Require().NoError(err, "failed to unpack AddAdaptorToCatalogue event from log data") | ||
| s.Require().Equal(common.HexToAddress(adaptorContract.Hex()), event.Adaptor) | ||
|
|
||
| return true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| }, 3*time.Minute, 10*time.Second, "add adaptor cellar event never seen") | ||
|
|
||
| s.Require().Eventuallyf(func() bool { | ||
| s.T().Log("querying add position cellar event...") | ||
| ethClient, err := ethclient.Dial(fmt.Sprintf("http://%s", s.ethResource.GetHostPort("8545/tcp"))) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // For non-anonymous events, the first log topic is a keccak256 hash of the | ||
| // event signature. | ||
| eventSignature := []byte("AddPositionToCatalogue(uint32)") | ||
| mockEventSignatureTopic := crypto.Keccak256Hash(eventSignature) | ||
| query := ethereum.FilterQuery{ | ||
| FromBlock: nil, | ||
| ToBlock: nil, | ||
| Addresses: []common.Address{ | ||
| v2_2Cellar, | ||
| }, | ||
| Topics: [][]common.Hash{ | ||
| { | ||
| mockEventSignatureTopic, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| logs, err := ethClient.FilterLogs(context.Background(), query) | ||
| if err != nil { | ||
| ethClient.Close() | ||
| return false | ||
| } | ||
|
|
||
| vault_abi, err := CellarV22MetaData.GetAbi() | ||
| s.Require().NoError(err) | ||
|
|
||
| if len(logs) > 0 { | ||
| s.T().Logf("found %d logs!", len(logs)) | ||
| for _, log := range logs { | ||
| if len(log.Data) > 0 { | ||
| var event CellarV22AddPositionToCatalogue | ||
| err := vault_abi.UnpackIntoInterface(&event, "AddPositionToCatalogue", log.Data) | ||
| s.Require().NoError(err, "failed to unpack AddPositionToCatalogue event from log data") | ||
| s.Require().Equal(uint32(1), event.Position) | ||
|
|
||
| return true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| }, 3*time.Minute, 10*time.Second, "add position cellar event never seen") | ||
| } |
There was a problem hiding this comment.
The new test function TestScheduledCorkMulticallProposal is well-structured and follows a logical sequence of steps to test the multicall functionality in governance proposals. However, there are a few areas where improvements can be made:
-
Error Handling and Logging: Ensure that all errors are handled appropriately. For instance, after dialing the Ethereum client (
ethclient.Dial), it's good practice to defer closing the client connection to prevent resource leaks. This is observed in lines 344 and 394 but should include adefer ethClient.Close()after successful connection establishment. -
Repeated Code: There's a noticeable repetition in querying Ethereum logs for different events (
AddAdaptorToCatalogueandAddPositionToCatalogue). Consider refactoring this into a helper function that takes the event signature and expected outcome as parameters. This will improve code maintainability and readability. -
Magic Numbers: The test uses hardcoded values, such as the
targetBlockHeightincrement (currentHeight + 90) and sleep durations. It's advisable to define these as constants or configurable parameters at the beginning of your test file or function. This makes the test more adaptable and easier to maintain, especially if adjustments are needed based on the blockchain's performance or test environment changes. -
Test Coverage and Assertions: While the test checks for the presence of specific events to confirm the multicall execution, it might be beneficial to extend the assertions to cover more aspects of the multicall's effects. For example, verifying the state changes (if any) in the smart contracts involved in the multicall could provide a more comprehensive test coverage.
-
Documentation: Adding comments to explain the rationale behind certain steps or values (e.g., why
targetBlockHeightis set tocurrentHeight + 90) can enhance the test's readability and maintainability. This is particularly useful for complex tests involving multiple blockchain interactions.
Consider implementing these suggestions to enhance the test's effectiveness, maintainability, and readability.
src/cellars/cellar_v2_5.rs
Outdated
| Ok(result) | ||
| } | ||
|
|
||
| /// Encodes a call to a CellarV2.5 contract | ||
| pub fn get_encoded_governance_call( | ||
| function: GovernanceFunction, | ||
| call_type: GovernanceCallType, | ||
| cellar_id: &str, | ||
| proposal_id: u64, | ||
| ) -> Result<Vec<u8>, Error> { | ||
| match call_type { | ||
| GovernanceCallType::FunctionCall(f) => { | ||
| get_encoded_governance_function(f, cellar_id, proposal_id) | ||
| } | ||
| GovernanceCallType::Multicall(m) => { | ||
| let mut multicall = MulticallCall::default(); | ||
| m.function_calls | ||
| .iter() | ||
| .map(|f| get_encoded_governance_function(f.clone(), cellar_id, proposal_id)) | ||
| .collect::<Result<Vec<Vec<u8>>, Error>>()? | ||
| .iter() | ||
| .for_each(|f| multicall.data.push(Bytes::from(f.clone()))); | ||
|
|
||
| Ok(multicall.encode()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub fn get_encoded_governance_function( | ||
| call: GovernanceFunctionCall, | ||
| cellar_id: &str, | ||
| proposal_id: u64, | ||
| ) -> Result<Vec<u8>, Error> { | ||
| let function = call | ||
| .function | ||
| .ok_or_else(|| sp_call_error("call data is empty".to_string()))?; | ||
| match function { | ||
| GovernanceFunction::AddAdaptorToCatalogue(params) => { | ||
| log_governance_cellar_call( |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [486-738]
The implementation of get_encoded_governance_function and its handling of various governance functions is comprehensive. However, there are a few areas to consider:
-
Error Handling: Ensure that all potential error cases are handled gracefully, especially when parsing addresses and converting strings to
U256. It's crucial to provide clear error messages for better debugging and user experience. -
Performance: When encoding governance functions, especially in the case of multicalls, consider the performance implications of cloning and iterating over function calls. If possible, explore more efficient ways to handle these operations to minimize the computational overhead.
-
Best Practices: The use of
string_to_u256andsp_call_parse_addressfunctions repeatedly suggests that a utility function or a higher-level abstraction could simplify these operations and reduce code duplication. -
Security: Given the governance context, ensure that all security aspects are thoroughly reviewed, particularly the permissions and conditions under which these governance calls can be made. This includes validating inputs and ensuring that only authorized entities can execute sensitive operations.
Consider refactoring to reduce code duplication and improve error handling. Additionally, ensure comprehensive security reviews are conducted for governance-related functionality.
src/cellars/cellar_v2_2.rs
Outdated
| pub fn get_encoded_governance_function( | ||
| call: GovernanceFunctionCall, | ||
| cellar_id: &str, | ||
| proposal_id: u64, | ||
| ) -> Result<Vec<u8>, Error> { | ||
| let function = call | ||
| .function | ||
| .ok_or_else(|| sp_call_error("call data is empty".to_string()))?; |
There was a problem hiding this comment.
The function get_encoded_governance_function is well-structured and covers a wide range of governance functions. It's essential to ensure that the error message "call data is empty" is clear and actionable for the developers or operators who might encounter it. Consider adding more context to the error message, such as the specific governance function that failed to provide call data.
- .ok_or_else(|| sp_call_error("call data is empty".to_string()))?;
+ .ok_or_else(|| sp_call_error(format!("call data is empty for function {:?}", function)))?;Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| pub fn get_encoded_governance_function( | |
| call: GovernanceFunctionCall, | |
| cellar_id: &str, | |
| proposal_id: u64, | |
| ) -> Result<Vec<u8>, Error> { | |
| let function = call | |
| .function | |
| .ok_or_else(|| sp_call_error("call data is empty".to_string()))?; | |
| let function = call | |
| .function | |
| .ok_or_else(|| sp_call_error(format!("call data is empty for function {:?}", function)))?; |
src/cellars/cellar_v2_2.rs
Outdated
| /// Encodes a call to a CellarV2.2 contract | ||
| pub fn get_encoded_governance_call( | ||
| function: GovernanceFunction, | ||
| call_type: GovernanceCallType, | ||
| cellar_id: &str, | ||
| proposal_id: u64, | ||
| ) -> Result<Vec<u8>, Error> { | ||
| match call_type { | ||
| GovernanceCallType::FunctionCall(f) => { | ||
| get_encoded_governance_function(f, cellar_id, proposal_id) | ||
| } | ||
| GovernanceCallType::Multicall(m) => { | ||
| let mut multicall = MulticallCall::default(); | ||
| m.function_calls | ||
| .iter() | ||
| .map(|f| get_encoded_governance_function(f.clone(), cellar_id, proposal_id)) | ||
| .collect::<Result<Vec<Vec<u8>>, Error>>()? | ||
| .iter() | ||
| .for_each(|f| multicall.data.push(Bytes::from(f.clone()))); | ||
|
|
||
| Ok(multicall.encode()) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In the get_encoded_governance_call function, the use of default() for initializing MulticallCall followed by iterative data pushing is efficient. However, consider pre-allocating the vector size for multicall.data based on the number of function_calls to improve performance by reducing the number of reallocations.
let mut multicall = MulticallCall::default();
multicall.data.reserve(m.function_calls.len());
proto/steward/v4/cellar_v2.proto
Outdated
| // Represents function `setRebalanceDeviation(uint265)` | ||
| SetRebalanceDeviation set_rebalance_deviation = 3; |
There was a problem hiding this comment.
There's a typo in the SetRebalanceDeviation message definition where uint265 should likely be uint256. This typo could lead to compilation errors or unintended behavior.
- // Represents function `setRebalanceDeviation(uint265)`
+ // Represents function `setRebalanceDeviation(uint256)`Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| // Represents function `setRebalanceDeviation(uint265)` | |
| SetRebalanceDeviation set_rebalance_deviation = 3; | |
| // Represents function `setRebalanceDeviation(uint256)` | |
| SetRebalanceDeviation set_rebalance_deviation = 3; |
| @@ -42,7 +42,7 @@ message CellarV1 { | |||
| SetLiquidityLimit set_liquidity_limit = 10; | |||
| // Represents function `setShareLockPeriod()` | |||
| SetShareLockPeriod set_share_lock_period = 11; | |||
| // Represents function `setRebalanceDeviation(uint265)` | |||
| // Represents function `setRebalanceDeviation(uint256)` | |||
There was a problem hiding this comment.
While the specific change to setRebalanceDeviation is correct, it's noted that numeric values such as index, new_limit, and new_deviation are represented as string types throughout the protobuf definitions. This approach might be intentional to handle large numbers or specific formats (e.g., hexadecimal). However, if these fields are expected to be strictly numeric and within the range of standard numeric types in protobuf (e.g., int32, int64), consider using more appropriate numeric types for clarity and type safety.
Closes #256