Boring Vault ManagerWithMerkleVerification support #278
Conversation
|
Adding a mock contract + integration test |
WalkthroughThe changes in this pull request encompass updates to various files, primarily focusing on the introduction of a new vault management contract with Merkle verification, enhancements to existing integration tests, and updates to dependencies across multiple modules. Key modifications include the addition of new test targets in the Makefile, updates to ABI generation processes, and the introduction of a Solidity smart contract. Additionally, several files have been updated to reflect changes in import paths and types due to version upgrades of dependencies. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IntegrationTests
participant ManagerWithMerkleVerification
participant Ethereum
User->>IntegrationTests: Trigger test
IntegrationTests->>ManagerWithMerkleVerification: Set manage root
ManagerWithMerkleVerification->>Ethereum: Execute setManageRoot
Ethereum-->>ManagerWithMerkleVerification: Confirm execution
ManagerWithMerkleVerification-->>IntegrationTests: Return success
IntegrationTests-->>User: Test result
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. 🔧 bufproto/steward/v4/boring_vault/v1/manager.protoConfig file JSON parsing error: invalid character 'v' looking for beginning of value. Please check your buf configuration file for JSON syntax errors. proto/steward/v4/steward.protoConfig file JSON parsing error: invalid character 'v' looking for beginning of value. Please check your buf configuration file for JSON syntax errors. 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: 10
🧹 Outside diff range and nitpick comments (25)
proto/steward/v4/boring_vault/v1/manager.proto (2)
1-9: Consider enhancing the file-level documentationWhile the current documentation indicates the purpose, it would be beneficial to include:
- A brief description of what a Boring Vault Manager is
- The purpose of Merkle verification in this context
- Any important security considerations
/* - * Protos for function calls to Boring Vault Manager contracts + * Protocol Buffers definitions for Boring Vault Manager contract interactions. + * + * The Boring Vault Manager is responsible for managing vault access control + * using Merkle tree verification. This allows for efficient and secure + * verification of authorized strategists and their permissions. + * + * Security: Ensure that Merkle roots are properly validated and + * that only authorized entities can call these functions. */
20-26: Consider adding format specifications for address and root fieldsThe string type for Ethereum addresses and Merkle roots should include format specifications in the documentation to ensure proper validation:
message SetManageRoot { - // The address of the strategist + // The Ethereum address of the strategist (hex-encoded with '0x' prefix) string strategist = 1; - // The manage root to set + // The Merkle root to set (32-byte hex-encoded with '0x' prefix) string manage_root = 2; }Additionally, consider adding comments about validation requirements:
- Address format validation (20 bytes)
- Merkle root format validation (32 bytes)
src/cellars/boring_vault_manager_with_merkle_verification.rs (2)
16-18: Add documentation for the public function.Consider adding documentation comments to explain the purpose of this public function, its parameters, and return value.
+/// Encodes a vault management function call into bytes. +/// +/// # Arguments +/// * `function` - The function to encode (SetManageRoot, Pause, or Unpause) +/// * `cellar_id` - The identifier of the cellar +/// +/// # Returns +/// The ABI-encoded function call as a byte vector pub fn get_encoded_call(function: Function, cellar_id: String) -> Result<Vec<u8>, Error> { get_call(function, cellar_id).map(|call| call.encode()) }
28-36: Add hex string validation for manage_root.The hex decoding might fail if the input contains a '0x' prefix. Consider adding validation or handling for this case.
- let manage_root = hex::decode(params.manage_root) + let manage_root = hex::decode(params.manage_root.trim_start_matches("0x"))integration_tests/genesis.go (1)
8-14: Document the architectural change.Consider adding a comment above the imports to document this architectural change, as it represents a significant shift from Tendermint to CometBFT.
Add this comment above the imports:
package integration_tests +// This package uses CometBFT instead of Tendermint as the consensus engine, +// following the official migration path of the Cosmos SDK ecosystem. import (proto/steward/v4/steward.proto (1)
Line range hint
1-15: LGTM with documentation suggestionsThe changes maintain good proto design practices:
- Clear field naming
- Proper use of oneof for call_data variants
- Consistent with existing patterns
Consider updating the file's header comment to document the new boring vault functionality.
/* * Steward Strategy Provider API * * This proto defines the service/methods used by Strategy Providers to interact with Cellars through the Sommelier chain. + * Supports various contract interactions including Aave V2, Cellar versions, and Boring Vault with Merkle verification. */Also applies to: 59-64
integration_tests/validator.go (1)
Line range hint
1-400: Implementation maintains compatibility with upgraded dependenciesThe code successfully maintains compatibility with the upgraded dependencies. The validator implementation continues to work with CometBFT's types and functions without requiring changes, which is a positive sign.
However, consider adding a comment at the top of the file documenting the minimum required versions of the dependencies, as this would help future maintainers.
Add a comment block at the top of the file:
package integration_tests +// Minimum required dependency versions: +// - CometBFT v0.37.x +// - Gravity Bridge v5.x +// - Sommelier v8.x + import (integration_tests/proposal_test.go (2)
Line range hint
1-500: Consider refactoring common test patternsThe test functions share significant code duplication in proposal submission, voting, and verification logic. Consider extracting these into helper functions to improve maintainability and reduce duplication:
- Proposal submission and voting
- Event verification
- Height waiting logic
Example helper function structure:
+func (s *IntegrationTestSuite) submitAndVoteProposal(proposal sdk.Msg, orch Orchestrator) (uint64, error) { + // Common proposal submission and voting logic +} + +func (s *IntegrationTestSuite) waitForProposalApproval(proposalID uint64, govQueryClient govtypesv1beta1.QueryClient) error { + // Common proposal approval waiting logic +} + +func (s *IntegrationTestSuite) waitForScheduledHeight(targetHeight int64, queryClient interface{}) error { + // Common height waiting logic +}
Line range hint
1-500: Review timeout configurationsThe tests use various hardcoded timeouts and intervals:
- 3-minute timeouts for event checks
- 10-second intervals for polling
- 90-block target heights
Consider:
- Making these configurable via constants
- Reducing timeouts where possible to speed up tests
- Adding comments explaining the rationale for these values
Example configuration:
+const ( + // Maximum time to wait for events before failing the test + eventTimeout = 3 * time.Minute + // Interval between event checks + pollInterval = 10 * time.Second + // Number of blocks to wait before executing scheduled cork + scheduledBlockDelay = 90 +)integration_tests/ethereum/contracts/MockManagerWithMerkleVerification.sol (2)
27-27: Consider restricting visibility ofisPausedvariableThe
isPausedstate variable is declared aspublic, which automatically creates a getter function. If external access to this variable is not necessary, consider changing its visibility toprivateorinternalto encapsulate the state.-bool public isPaused; +bool private isPaused;If you still need to expose this state externally, you can implement a custom getter function with additional logic if required.
31-42: Remove unused error declarations or implement their usageThe contract declares several custom errors from lines 31 to 42. However, none of these errors are used in the provided code.
If these errors are intended for future use, consider adding a comment to indicate that. Otherwise, remove the unused error declarations to clean up the code.
-error ManagerWithMerkleVerification__InvalidManageProofLength(); -error ManagerWithMerkleVerification__InvalidTargetDataLength(); -error ManagerWithMerkleVerification__InvalidValuesLength(); -error ManagerWithMerkleVerification__InvalidDecodersAndSanitizersLength(); -error ManagerWithMerkleVerification__FlashLoanNotExecuted(); -error ManagerWithMerkleVerification__FlashLoanNotInProgress(); -error ManagerWithMerkleVerification__BadFlashLoanIntentHash(); -error ManagerWithMerkleVerification__FailedToVerifyManageProof(address target, bytes targetData, uint256 value); -error ManagerWithMerkleVerification__Paused(); -error ManagerWithMerkleVerification__OnlyCallableByBoringVault(); -error ManagerWithMerkleVerification__OnlyCallableByBalancerVault(); -error ManagerWithMerkleVerification__TotalSupplyMustRemainConstantDuringPlatform();integration_tests/chain.go (4)
253-256: Avoid duplicate interface registrations forsdk.Msg.The
sdk.Msgimplementations are registered both in theinitfunction and here. Ensure that this duplication is necessary or consolidate the registrations to a single location to prevent potential conflicts.
262-264: Review commented code for potential removal or reactivation.The lines involving
simapp.ModuleBasicsare commented out. Determine if they are still needed:// simapp.ModuleBasics.RegisterLegacyAminoCodec(encodingConfig.Amino) // simapp.ModuleBasics.RegisterInterfaces(encodingConfig.InterfaceRegistry)If these registrations are no longer necessary due to the migration to the new encoding configurations, consider removing them to clean up the code.
Line range hint
308-310: Handle response bytes correctly insendMsgs.The current implementation reads from
clientCtx.Input, which is set toos.Stdinin theclientContextfunction. Reading fromStdinmight not yield the expected transaction response.Current code:
resBytes := []byte{} _, err = clientCtx.Input.Read(resBytes) if err != nil { return nil, err }Consider using the result from
GenerateOrBroadcastTxWithFactory, or retrieve the transaction response through the RPC client.Apply this diff to handle the transaction response properly:
err := tx.GenerateOrBroadcastTxWithFactory(clientCtx, txf, msgs...) if err != nil { return nil, err } -resBytes := []byte{} -_, err = clientCtx.Input.Read(resBytes) -if err != nil { - return nil, err -} - -var res sdk.TxResponse -err = cdc.Unmarshal(resBytes, &res) +resTx, err := clientCtx.Client.BroadcastTxSync(clientCtx.Context, txBytes) if err != nil { return nil, err } -return &res, nil +return resTx, nilEnsure that
BroadcastTxSyncor the appropriate method is used to get the transaction response.
227-264: Initialize encoding configuration consistently across the codebase.In both the
initfunction and theclientContextmethod, encoding configurations are being initialized separately. This could lead to inconsistencies if different modules or interfaces are registered in each.Consider refactoring to initialize the encoding configuration once and reuse it.
integration_tests/setup_test.go (2)
561-561: Expose gRPC Address CarefullyBinding the gRPC server to
0.0.0.0:9090allows external access from all network interfaces, which may pose security risks. Ensure that this is intentional and acceptable for your testing environment.
692-703: Improve Log Parsing formanagerContractAddressThe current parsing logic for retrieving the
managerContractaddress assumes a specific log format and uses string splitting, which may be fragile if the log format changes. Consider using regular expressions or more robust parsing methods to extract the contract address reliably.For example, using a regular expression:
import "regexp" //... re := regexp.MustCompile(`manager contract deployed at - (\S+)`) matches := re.FindStringSubmatch(ethereumLogOutput.String()) if len(matches) > 1 { managerContract = common.HexToAddress(matches[1]) return true }integration_tests/cork_test.go (8)
Line range hint
988-1008: Add error handling for gravity module queriesErrors returned by
gravityQueryClient.ContractCallTxsandgravityQueryClient.CompletedContractCallTxsare currently ignored. This can lead to nil pointer dereferences or missed error conditions.Apply this diff to handle errors properly:
request := &gravityTypes.ContractCallTxsRequest{ Pagination: &query.PageRequest{}, } - response, _ := gravityQueryClient.ContractCallTxs(context.Background(), request) + response, err := gravityQueryClient.ContractCallTxs(context.Background(), request) + if err != nil { + s.T().Logf("Error querying ContractCallTxs: %v", err) + return false + } completedRequest := &gravityTypes.CompletedContractCallTxsRequest{} - completedResponse, _ := gravityQueryClient.CompletedContractCallTxs(context.Background(), completedRequest) + completedResponse, err := gravityQueryClient.CompletedContractCallTxs(context.Background(), completedRequest) + if err != nil { + s.T().Logf("Error querying CompletedContractCallTxs: %v", err) + return false + }
Line range hint
1034-1096: Optimize block range for Ethereum log queriesIn
waitForGravityLogicCallEvent,fromBlockis set to1, which may cause performance issues by querying from the genesis block. Consider adjusting the block range to only include relevant blocks.Apply this diff to set
fromBlockto a more recent block:- fromBlock := uint64(1) + // Use the latest block minus a reasonable offset + fromBlock := latestBlock - 1000 // Adjust offset as appropriate
Line range hint
347-350: Ensure error handling when dialing Ethereum clientIn multiple places, the error from
ethclient.Dialis not followed by adefer ethClient.Close(), which can cause resource leaks if not handled properly.Apply this pattern across all instances:
ethClient, err := ethclient.Dial(fmt.Sprintf("http://%s", s.ethResource.GetHostPort("8545/tcp"))) if err != nil { return false } + defer ethClient.Close()
1053-1054: Consider dynamic calculation offromBlockSetting
fromBlockto a fixed value might not be appropriate in all test environments. CalculatefromBlockbased on the current context to improve reliability.Example adjustment:
- fromBlock := uint64(1) + fromBlock := latestBlock - 100 // Subtract a smaller offset to cover recent blocks + if latestBlock < 100 { + fromBlock = 0 + }
782-793: Consistency in variable naming and spacingEnsure consistent use of blank lines and variable declarations for better readability.
Consider removing unnecessary blank lines and aligning variable declarations:
func (s *IntegrationTestSuite) TestBoringVaultManager() { s.Run("Set the manage root of the Manager contract", func() { s.checkCellarExists(managerContract) - cellarId := managerContract.String() val := s.chain.validators[0] kb, err := val.keyring() s.Require().NoError(err) clientCtx, err := s.chain.clientContext("tcp://localhost:26657", &kb, "val", val.address()) s.Require().NoError(err) currentHeight, err := s.GetLatestBlockHeight(clientCtx) s.Require().NoError(err) scheduledHeight := currentHeight + 10 + cellarId := managerContract.String()
825-827: Add validation formanageRootlengthBefore decoding
manageRoot, validate that the input string has the expected length to prevent potential runtime errors.Apply this diff to add a length check:
if len(manageRoot) != 64 { s.T().Log("Invalid manageRoot length") return } manageRootHex, err := hex.DecodeString(manageRoot)
840-841: Remove redundant commentThe comment about non-anonymous events is repeated multiple times. Consider removing it to reduce redundancy.
- // For non-anonymous events, the first log topic is a keccak256 hash of the - // event signature.
1056-1057: Simplify log query block rangeWhen querying logs, setting
FromBlockandToBlocktonilwill default to the latest block range, which might suffice for tests.query := ethereum.FilterQuery{ - FromBlock: big.NewInt(int64(fromBlock)), - ToBlock: big.NewInt(int64(latestBlock)), + FromBlock: nil, + ToBlock: nil, Addresses: []common.Address{ gravityContract, },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (54)
.github/workflows/integration_tests.ymlis excluded by!**/*.ymlCargo.lockis excluded by!**/*.lock,!**/*.lockabi/boring-vault/ManagerWithMerkleVerification.jsonis excluded by!**/*.jsoncrates/steward-proto/src/gen/descriptor.binis excluded by!**/*.bin,!**/gen/**,!**/*.bin,!**/gen/**crates/steward-proto/src/gen/steward.v4.rsis excluded by!**/gen/**,!**/gen/**go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumgo/steward_proto_go/steward_proto/a_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/aave_v2_enable_asset_as_collateral_adaptor.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/aave_v2_stablecoin.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/aave_v3_a_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/aave_v3_debt_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/aave_v3_debt_token_flash_loan.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/aura_erc4626.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/balancer_pool.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/balancer_pool_flash_loan.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/base.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/c_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/cellar_adaptor.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/cellar_v1.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/cellar_v2.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/collateral_f_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/common.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/convex_curve.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/curve.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/debt_f_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/debt_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/erc4626.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/f_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/fees_and_reserves.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/governance.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/legacy_cellar_adaptor.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/manager.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/morpho_aave_v2_a_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/morpho_aave_v2_debt_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/morpho_aave_v3_a_token_collateral.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/morpho_aave_v3_a_token_p2p.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/morpho_aave_v3_debt_token.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/morpho_blue_collateral.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/morpho_blue_debt.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/morpho_blue_supply.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/morpho_reward_handler.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/oneinch.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/pendle.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/staking.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/steward.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/steward_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/swap_with_uniswap.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/uniswap_v3.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/vesting_simple.pb.gois excluded by!**/*.pb.go,!**/*.pb.gogo/steward_proto_go/steward_proto/zero_x.pb.gois excluded by!**/*.pb.go,!**/*.pb.gointegration_tests/ethereum/contracts/MockManagerWithMerkleVerification.jsonis excluded by!**/*.jsonsrc/gen/abi/boring_vault/manager_with_merkle_verification.rsis excluded by!**/gen/**,!**/gen/**src/gen/proto/descriptor.binis excluded by!**/*.bin,!**/gen/**,!**/*.bin,!**/gen/**
📒 Files selected for processing (20)
Makefile(3 hunks)build.rs(3 hunks)hash_abi(1 hunks)hash_proto(1 hunks)integration_tests/chain.go(3 hunks)integration_tests/cork_test.go(10 hunks)integration_tests/ethereum/contracts/MockManagerWithMerkleVerification.sol(1 hunks)integration_tests/ethereum/hardhat.config.ts(2 hunks)integration_tests/genesis.go(1 hunks)integration_tests/manager_with_merkle_verification_abi.go(1 hunks)integration_tests/proposal_test.go(1 hunks)integration_tests/setup_test.go(10 hunks)integration_tests/validator.go(2 hunks)proto/steward/v4/boring_vault/v1/manager.proto(1 hunks)proto/steward/v4/steward.proto(2 hunks)scripts/build_go_protos.sh(1 hunks)src/cellars.rs(1 hunks)src/cellars/boring_vault_manager_with_merkle_verification.rs(1 hunks)src/cork.rs(2 hunks)src/lib.rs(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- hash_abi
- hash_proto
- integration_tests/manager_with_merkle_verification_abi.go
- src/cellars.rs
🔇 Additional comments (31)
scripts/build_go_protos.sh (1)
13-13: Verify the proto directory structure and files
The changes look good and follow consistent patterns. Let's verify the directory structure and proto files exist.
Also applies to: 17-17
✅ Verification successful
Proto directory structure and files verified successfully
The verification confirms:
- Directory
proto/steward/v4/boring_vault/v1exists - Contains
manager.protofile - Path is correctly referenced in
build.rs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the proto directory structure and files exist
# Check if boring_vault/v1 directory exists
if [ -d "proto/steward/v4/boring_vault/v1" ]; then
echo "✓ Directory exists: proto/steward/v4/boring_vault/v1"
# List proto files in the directory
echo "Proto files in boring_vault/v1:"
find proto/steward/v4/boring_vault/v1 -name "*.proto" -type f
# Verify consistency with build.rs
echo "Checking build.rs for corresponding proto path:"
rg "boring_vault.*proto" build.rs
else
echo "✗ Directory not found: proto/steward/v4/boring_vault/v1"
fi
Length of output: 670
proto/steward/v4/boring_vault/v1/manager.proto (3)
10-19: LGTM! Well-structured message definition
The oneof construct is appropriately used to represent mutually exclusive operations, and the field numbers are properly sequential. The documentation clearly maps each field to its corresponding contract function.
28-33: LGTM! Clear and concise message definitions
The empty message definitions for Pause and Unpause operations are appropriate as these operations don't require parameters.
1-33: Verify proto definitions match smart contract interface
Let's ensure the proto definitions align with the smart contract implementation:
✅ Verification successful
Based on the shell script results, I can now generate the final response since I have sufficient information to verify the proto definitions against the smart contract interface.
Proto definitions correctly match smart contract interface
The proto definitions in proto/steward/v4/boring_vault/v1/manager.proto accurately reflect the smart contract interface found in the codebase:
setManageRoot(address strategist, bytes32 _manageRoot)matches the proto messageSetManageRootpause()matches the proto messagePauseunpause()matches the proto messageUnpause
The function signatures, parameter types, and names are consistent between the proto definitions and the smart contract implementation found in integration_tests/ethereum/contracts/MockManagerWithMerkleVerification.sol.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the proto definitions match the smart contract interface
# Find and check the contract interface
echo "Searching for contract interface..."
rg -A 10 "contract.*ManagerWithMerkleVerification"
# Verify function signatures
echo "Verifying function signatures..."
rg "function set[mM]anageRoot|function pause|function unpause"
Length of output: 30014
src/cellars/boring_vault_manager_with_merkle_verification.rs (3)
1-15: LGTM! Well-organized imports and clear constant definition.
The imports are logically grouped and the constant naming is clear and descriptive.
20-56: LGTM! Well-structured implementation with proper error handling.
The function implementation is robust with:
- Comprehensive error handling with context
- Proper logging for all operations
- Type-safe conversions
- Clear match arms for each operation
20-56: Verify event emission for security operations.
For security-critical operations like SetManageRoot and Pause/Unpause, it's important to ensure events are being emitted for audit purposes.
integration_tests/genesis.go (1)
8-8: LGTM! Verify dependency compatibility.
The migration from Tendermint to CometBFT is correct and aligns with the Cosmos SDK's architectural direction. CometBFT is now the official consensus engine fork of Tendermint.
Let's verify the version compatibility with other dependencies:
✅ Verification successful
Dependency versions are compatible and correctly aligned
The CometBFT v0.37.5 and Cosmos SDK v0.47.14 versions are compatible, as this combination is well-tested and officially supported. The explicit version exclusion of CometBFT v0.38.0 also shows proper version management to maintain stability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for version compatibility between CometBFT and Cosmos SDK
# Look for go.mod to verify versions
rg -A 5 "github.com/cometbft/cometbft" go.mod
rg -A 5 "github.com/cosmos/cosmos-sdk" go.mod
Length of output: 977
proto/steward/v4/steward.proto (2)
59-64: Verify backward compatibility with field number changes
The shifting of field numbers (chain_id: 8->9, deadline: 9->10) could impact backward compatibility if there are existing clients using the old field numbers. Please ensure:
- All clients are updated to use the new field numbers
- Consider adding migration documentation if needed
#!/bin/bash
# Description: Search for direct usage of field numbers in code
# Look for potential hardcoded field numbers in Go files
rg -t go "uint64.*[89].*=.*chain_id|uint64.*[910].*=.*deadline"
# Look for proto definitions that might conflict with these field numbers
fd -e proto | xargs rg "= [89]|= 10"15-15: Verify the boring_vault proto file existence
The new import suggests integration with a v1 boring vault manager proto definition.
✅ Verification successful
Let me gather more information to verify the existence of the boring_vault proto file using a different search strategy.
Let me check the contents of the boring_vault directory and related files to verify the proto file's existence and structure.
Let me check the contents of the v1 subdirectory to verify the manager.proto file.
Import path is valid and properly structured
The import statement import "boring_vault/v1/manager.proto" is correct. The proto file exists at the expected location with proper package and message definitions for the Boring Vault Manager functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and location of the boring_vault proto file
fd -e proto "manager.proto" | grep -i "boring_vault"
Length of output: 4411
Script:
#!/bin/bash
# Search for any file or directory containing "boring_vault" in its name
fd "boring_vault"
# Search for any references to boring_vault in proto files
rg -t proto "boring_vault"
Length of output: 192
Script:
#!/bin/bash
# List contents of the boring_vault proto directory
ls -la proto/steward/v4/boring_vault/
# Check contents of any proto files in the boring_vault directory
cat proto/steward/v4/boring_vault/*.proto
# Search for references to boring vault manager in proto files using ripgrep
rg --type-add 'proto:*.proto' -t proto "boring.*vault.*manager"
Length of output: 585
Script:
#!/bin/bash
# List contents of the v1 directory
ls -la proto/steward/v4/boring_vault/v1/
# Check if manager.proto exists and show its contents
cat proto/steward/v4/boring_vault/v1/manager.proto
Length of output: 1140
Makefile (2)
24-24: LGTM! Test target follows established patterns
The new test target e2e_boring_vault_manager_test and its integration into e2e_cork_test follows the established patterns:
- Consistent cleanup handling using
scripts/clean_slate.sh - Same test execution pattern with proper flags
- Error handling via the
failtarget - Integration with the main test suite
Also applies to: 57-60
3-3: Verify compatibility with Sommelier version upgrade
The Sommelier version has been upgraded from v7.0.1 to v8.0.3, which is a significant version bump that could include breaking changes.
Let's verify the changelog and compatibility:
✅ Verification successful
Let me try to gather more information about potential breaking changes by examining the codebase for version-specific dependencies and configurations.
Let me try one final verification to check for any version-specific code or breaking changes in the codebase.
Version upgrade appears safe to proceed
The version upgrade from v7.0.1 to v8.0.3 has been analyzed and appears safe based on the following findings:
- The codebase has been properly updated with all necessary v8 imports and dependencies
- Integration tests have been updated to use the new v8 packages
- No migration scripts or breaking changes documentation were found
- The version is consistently referenced across the codebase
- The builder module still maintains v7 compatibility where needed (in examples)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in Sommelier v8.0.3
# Check the release notes and changelog
gh api repos/PeggyJV/sommelier/releases/tags/v8.0.3 --jq '.body' || echo "Release notes not found"
# Look for any migration guides or breaking changes documentation
fd -t f "CHANGELOG.md|BREAKING-CHANGES.md|MIGRATION.md" | xargs cat
Length of output: 163
Script:
#!/bin/bash
# Look for version-specific imports, configurations, or dependencies
rg -g '!{node_modules,dist,build}/*' -l "github.com/PeggyJV/sommelier" --type-add 'go:*.{go,mod}' --type go
# Check go.mod for sommelier dependency version
fd "go.mod" -x cat {}
# Look for any version-specific configurations or constants
rg -g '!{node_modules,dist,build}/*' "v7.0" -A 2 -B 2
Length of output: 19236
Script:
#!/bin/bash
# Look for any version-specific imports or code patterns
rg -g '!{node_modules,dist,build}/*' "sommelier.*v[78]" -A 2 -B 2
# Check for any migration scripts or upgrade handlers
fd -t f "upgrade|migration" -x cat {}
# Look for any version-specific configuration or constants
rg -g '!{node_modules,dist,build}/*' "v8.0" -A 2 -B 2
Length of output: 6920
integration_tests/ethereum/hardhat.config.ts (1)
100-106: LGTM! Ownership transfer implementation is consistent.
The ownership transfer to the Gravity contract follows the same pattern as other contracts, using consistent gas pricing and proper logging.
src/lib.rs (1)
199-203: LGTM! Module structure follows the established pattern.
The new Boring Vault Manager module is well-organized and follows the existing pattern for ABI definitions.
Let's verify the ABI file generation setup:
✅ Verification successful
ABI file and build configuration verified successfully ✓
The verification confirms:
- The ABI file exists at the expected path:
src/gen/abi/boring_vault/manager_with_merkle_verification.rs - The build configuration in
build.rscorrectly maps the contract name to the module path
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the ABI file existence and build configuration
# Check if the ABI file exists
if [ -f "src/gen/abi/boring_vault/manager_with_merkle_verification.rs" ]; then
echo "✓ ABI file exists"
else
echo "✗ ABI file is missing"
fi
# Check build.rs for proper ABI generation configuration
rg -A 5 "boring-vault/ManagerWithMerkleVerification" build.rs
Length of output: 354
build.rs (4)
135-141: LGTM! Good improvements to variable naming and path handling
The changes improve code clarity through:
- More descriptive variable names
- Robust handling of both flat and nested contract paths
Line range hint 142-164: LGTM! Improved error messages and consistent error handling
The error messages are now more precise and consistently use the correct file names, making debugging easier.
174-178: Verify proto file existence and consider const for path segments
Let's verify the proto file exists and suggest a minor improvement for maintainability.
#!/bin/bash
# Description: Check if the proto files exist
PROTO_PATH="proto/steward/v4/boring_vault/v1"
if [ -d "$PROTO_PATH" ]; then
echo "✓ Proto directory exists"
find "$PROTO_PATH" -name "*.proto" -type f -exec echo "Found proto file: {}" \;
else
echo "✗ Proto directory not found at: $PROTO_PATH"
fiConsider extracting the proto path segments as constants for better maintainability:
const TMP_PATH: &str = "/tmp/steward/";
const OUT_PATH: &str = "crates/steward-proto/src/gen/";
const HASH_ABI_FILE: &str = "hash_abi";
const HASH_PROTO_FILE: &str = "hash_proto";
+const PROTO_BASE_PATH: &str = "proto/steward/v4";
+const BORING_VAULT_PROTO_PATH: &str = "proto/steward/v4/boring_vault/v1";128-131: Verify the ABI file existence for the new contract
The new contract path uses a nested structure. Let's verify that the ABI file exists at the expected location.
✅ Verification successful
ABI file for ManagerWithMerkleVerification contract is valid and complete
The ABI file exists at the correct location (abi/boring-vault/ManagerWithMerkleVerification.json), has valid JSON structure, and contains a comprehensive interface including core functionality (manageVaultWithMerkleVerification, flashLoan), administrative functions (pause, unpause, setAuthority), and expected events/errors. The contract path mapping in build.rs correctly corresponds to the actual file structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the ABI file exists and is valid JSON
ABI_FILE="abi/boring-vault/ManagerWithMerkleVerification.json"
if [ -f "$ABI_FILE" ]; then
if jq empty "$ABI_FILE" 2>/dev/null; then
echo "✓ ABI file exists and is valid JSON"
jq 'keys' "$ABI_FILE" # Show available functions
else
echo "✗ ABI file exists but is not valid JSON"
fi
else
echo "✗ ABI file not found at: $ABI_FILE"
fi
Length of output: 468
Script:
#!/bin/bash
# Let's try a different approach to locate and verify the ABI file
# First, let's check if the directory exists
if [ -d "abi/boring-vault" ]; then
echo "✓ Directory abi/boring-vault exists"
ls -la abi/boring-vault/
else
echo "✗ Directory abi/boring-vault not found"
echo "Checking parent directory:"
ls -la abi/
fi
# Let's also search for any JSON files that might contain "ManagerWithMerkleVerification"
echo -e "\nSearching for relevant JSON files:"
fd -e json . abi/ | grep -i "merkle"
Length of output: 510
Script:
#!/bin/bash
# Now that we found the file, let's verify its JSON structure
echo "Verifying JSON structure of ManagerWithMerkleVerification.json:"
if jq empty "abi/boring-vault/ManagerWithMerkleVerification.json" 2>/dev/null; then
echo "✓ Valid JSON structure"
# Let's check the contract interface by looking at function names
echo -e "\nContract interface (function names):"
jq -r '.[].name | select(. != null)' "abi/boring-vault/ManagerWithMerkleVerification.json" | sort | uniq
else
echo "✗ Invalid JSON structure"
fi
Length of output: 1422
integration_tests/validator.go (1)
13-16: Verify version compatibility across dependencies
The changes include significant version upgrades:
- Migration from Tendermint to CometBFT
- Gravity Bridge module upgrade (v4 -> v5)
- Sommelier upgrade (v7 -> v8)
While the code appears structurally sound, these version changes warrant verification.
Also applies to: 31-33
✅ Verification successful
Let me analyze the version compatibility based on the results:
-
The go.mod shows explicit version constraints:
- CometBFT v0.37.5 (with v0.38.0 explicitly excluded)
- Gravity Bridge v5.0.0 (specific commit)
- Sommelier v8.0.3
-
For Sommelier v8, we have clear changelog information showing:
- Upgrade of gravity module to v5 (matching our changes)
- New features and bug fixes
- Important note about using v8.0.3 specifically
Let me verify the CometBFT migration impact:
Version changes are compatible and properly implemented
The verification shows:
- Clean migration from Tendermint to CometBFT (no lingering Tendermint imports)
- CometBFT APIs (p2p, privval, config) are used consistently with v0.37.5
- Sommelier v8.0.3 explicitly includes Gravity Bridge v5 upgrade
- Version constraints in go.mod are properly pinned
The changes are safe and maintain compatibility across all dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version compatibility and check for breaking changes
# Check go.mod for version constraints
echo "Checking go.mod for version constraints..."
cat go.mod | grep -E "github.com/cometbft/cometbft|github.com/peggyjv/gravity-bridge|github.com/peggyjv/sommelier"
# Check for breaking changes in CometBFT migration
echo "Checking for CometBFT migration guides..."
curl -s https://raw.githubusercontent.com/cometbft/cometbft/main/UPGRADING.md | grep -A 5 "UPGRADING"
# Check Gravity Bridge changelog
echo "Checking Gravity Bridge changelog..."
curl -s https://api.github.com/repos/peggyjv/gravity-bridge/releases/tags/v5.0.0 | jq .body
# Check Sommelier changelog
echo "Checking Sommelier changelog..."
curl -s https://api.github.com/repos/peggyjv/sommelier/releases/tags/v8.0.0 | jq .body
Length of output: 2202
Script:
#!/bin/bash
# Check for any Tendermint imports that might have been missed
echo "Checking for remaining Tendermint imports..."
rg "github.com/tendermint" --type go
# Check usage of potentially changed CometBFT APIs
echo "Checking CometBFT API usage..."
rg "p2p\.|privval\.|config\." --type go -A 2
Length of output: 5237
src/cork.rs (2)
12-15: LGTM: Import changes follow existing patterns
The addition of boring_vault_manager_with_merkle_verification to the cellars imports is well-organized and consistent with the existing import structure.
153-162: Implementation looks good, verify protobuf alignment
The new match arm for BoringVaultManagerWithMerkleVerification follows the established pattern and includes proper error handling. However, let's verify the alignment with protobuf definitions and implementation.
✅ Verification successful
Implementation aligns with protobuf definitions
The verification confirms that the implementation in src/cork.rs correctly aligns with both the protobuf definitions and contract ABI:
- The protobuf definition in
proto/steward/v4/boring_vault/v1/manager.protodefines the expected message structure with functionsSetManageRoot,Pause, andUnpause - The implementation in
src/cellars/boring_vault_manager_with_merkle_verification.rscorrectly maps these functions to the corresponding contract calls usingManagerWithMerkleVerificationCalls - The error handling for empty function data matches the pattern used across other variants
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify protobuf definitions and implementation alignment
# Check for protobuf definition
echo "Checking protobuf definition..."
rg -A 10 "BoringVaultManagerWithMerkleVerification" --type proto
# Check for corresponding implementation
echo "Checking implementation..."
rg -A 10 "get_encoded_call" --type rust src/cellars/boring_vault_manager_with_merkle_verification.rs
# Look for any test coverage
echo "Checking test coverage..."
rg -A 10 "test.*BoringVaultManager" --type rust
Length of output: 874
Script:
#!/bin/bash
# Let's try different search patterns to find relevant information
# Search for proto files in the repository
echo "Finding proto files..."
fd -e proto
# Search for BoringVaultManager in all files
echo "Searching BoringVaultManager references..."
rg "BoringVaultManager" -A 5
# Check implementation details
echo "Checking implementation details..."
rg "ManagerWithMerkleVerificationCalls" -A 10
# Look for test files
echo "Looking for test files..."
fd -e rs test
Length of output: 59894
integration_tests/proposal_test.go (1)
16-17: Verify compatibility with sommelier v8 types
The imports have been updated to use v8 of the sommelier package. Ensure that any breaking changes in the API are properly handled in the test implementations.
✅ Verification successful
Import changes are compatible and correctly used
Based on the verification results, the v8 imports are being used consistently throughout the integration tests:
- The types are properly initialized in
setup_test.gowith correct genesis state setup - The proposal type URLs remain unchanged (
/cork.v2.ScheduledCorkProposaland/axelarcork.v1.AxelarScheduledCorkProposal) - The usage patterns in the test files match the expected structure for both cork and axelarcork proposals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes between v7 and v8 of the sommelier package
# Search for type definitions and method signatures in v7 and v8
rg -A 5 "type (Axelar)?ScheduledCorkProposal struct"
rg -A 5 "func New(Axelar)?ScheduledCorkProposal"
Length of output: 105
Script:
#!/bin/bash
# Let's try a different approach to find the relevant types and their usage
# Search for imports of these types in test files
rg -l "axelarcorktypes|corktypesv2" integration_tests/
# Look for usage of ScheduledCorkProposal in the test file
rg -A 5 "ScheduledCorkProposal" integration_tests/proposal_test.go
# Check if there are any other files using these types
fd . -t f -e go -x rg -l "axelarcorktypes|corktypesv2" {}
Length of output: 1852
Script:
#!/bin/bash
# Let's check the actual usage of these types in the test files to verify compatibility
# Check setup_test.go for any initialization or usage
rg -A 5 "axelarcorktypes|corktypesv2" integration_tests/setup_test.go
# Check cork_test.go for any usage
rg -A 5 "axelarcorktypes|corktypesv2" integration_tests/cork_test.go
# Check if there are any changes in the proposal handling
rg "Content.TypeUrl.*ScheduledCorkProposal" integration_tests/
Length of output: 2823
integration_tests/ethereum/contracts/MockManagerWithMerkleVerification.sol (1)
4-4: Verify the imported contracts and interfaces
You are importing Address and Owned from "./interfaces.sol". Ensure that interfaces.sol contains the correct definitions and that there are no naming conflicts.
Run the following script to check the contents of interfaces.sol:
This will help confirm that the imported contracts are correctly defined.
✅ Verification successful
Both Address and Owned are correctly defined in interfaces.sol
The file integration_tests/ethereum/contracts/interfaces.sol contains both required definitions:
Addressis defined as a library with utility functions for address handlingOwnedis defined as an abstract contract with ownership functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that interfaces.sol contains the correct definitions.
# Check for the presence of Address and Owned definitions
rg -A 5 -e 'contract Address' -e 'contract Owned' ./interfaces.sol
Length of output: 125
Script:
#!/bin/bash
# Let's first find the correct path to interfaces.sol
fd interfaces.sol
# Then check the content of the file
fd interfaces.sol --exec cat {}
Length of output: 19976
integration_tests/chain.go (3)
257-261: Register all necessary governance proposal content types.
Ensure that all custom proposal types are registered correctly for governance functionalities.
8-9: Update import path for corktypes to the correct version.
The import path for corktypes may be incorrect or outdated. Ensure that the version matches the rest of the codebase.
Current import:
corktypes "github.com/peggyjv/sommelier/v8/x/cork/types/v2"Verify that v2 is the intended version. If not, update it accordingly.
Run the following script to check available versions:
✅ Verification successful
The import path for corktypes is correct and consistent with the codebase.
Based on the search results, the import path github.com/peggyjv/sommelier/v8/x/cork/types/v2 is being used consistently across multiple files in the codebase:
- integration_tests/cork_test.go
- integration_tests/validator.go
- integration_tests/proposal_test.go
- integration_tests/chain.go
Additionally, the go.mod file shows that the project depends on github.com/peggyjv/sommelier/v8 v8.0.3, confirming that v8 is the correct major version.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List available versions of corktypes package.
# Expected: Verify if 'v2' is the correct version.
git ls-remote --tags https://github.com/peggyjv/sommelier.git | grep "x/cork/types"
Length of output: 83
Script:
#!/bin/bash
# Let's try a different approach to verify the correct version
# 1. Search for all imports of corktypes in the codebase
rg --type go "github.com/peggyjv/sommelier/.*/x/cork/types" -A 1
# 2. Look for go.mod to check the sommelier module version
fd "go.mod" --exec cat {}
Length of output: 19032
273-280: Check broadcast mode and client context settings.
- The broadcast mode is set to
flags.BroadcastSync. If immediate block confirmation is required during tests, consider usingflags.BroadcastBlock. - Ensure that the keyring and account retriever are properly configured for transaction signing and account lookups.
Run the following script to confirm the broadcast mode usage:
integration_tests/cork_test.go (5)
319-319: Ensure all calls to waitForGravityLogicCallEvent are updated
Similar to line 116, confirm that this call to waitForGravityLogicCallEvent has been correctly updated to match the new function signature.
Refer to the previous verification script to ensure consistency.
825-827: 🛠️ Refactor suggestion
Handle potential errors when decoding the manage root
When decoding manageRoot from a hex string, ensure that any errors are properly handled to avoid unexpected behavior.
Apply this diff to handle errors appropriately:
manageRootHex, err := hex.DecodeString(manageRoot)
if err != nil {
+ s.T().Logf("Failed to decode manageRoot: %v", err)
return
}Likely invalid or redundant comment.
881-885:
Validate event data and handle potential issues
While unpacking the ManageRootUpdated event, ensure that the number of topics is sufficient before accessing them to prevent index out-of-range errors.
Apply this diff to add necessary checks:
if len(log.Topics) >= 2 {
strategistAddr := common.HexToAddress(log.Topics[1].Hex())
// Rest of the code...
} else {
+ s.T().Log("Insufficient log topics")
return false
}Likely invalid or redundant comment.
1047-1052:
Handle potential errors when fetching the latest block
Currently, if fetching the latest block number fails, ethClient.Close() is not called, leading to a resource leak.
Apply this diff to ensure resources are properly closed:
latestBlock, err := ethClient.BlockNumber(context.Background())
if err != nil {
ethClient.Close()
+ s.T().Logf("Error fetching latest block number: %v", err)
return false
}
+ defer ethClient.Close()Likely invalid or redundant comment.
116-116: Ensure all calls to waitForGravityLogicCallEvent are updated
The function waitForGravityLogicCallEvent has been updated to remove the topic parameter. Verify that all calls to this function have been updated accordingly throughout the codebase to prevent any inconsistencies or missing parameters.
Run the following script to find any calls to waitForGravityLogicCallEvent with incorrect parameters:
✅ Verification successful
Based on the search results, I can now generate the final response:
All calls to waitForGravityLogicCallEvent are correctly updated
All three calls to waitForGravityLogicCallEvent in the codebase consistently use exactly two parameters (invalidationScope and invalidationNonce), matching the function's definition. No instances of incorrect parameter usage were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find calls to waitForGravityLogicCallEvent with more than two arguments.
# Search for function calls with more than two arguments.
ast-grep --pattern $'waitForGravityLogicCallEvent($_, $_, $_)'
Length of output: 63
Script:
#!/bin/bash
# Let's first find all occurrences of waitForGravityLogicCallEvent to analyze its usage
rg "waitForGravityLogicCallEvent" -A 2 -B 2
Length of output: 1529
| const Manager = await hre.ethers.getContractFactory("ManagerWithMerkleVerification"); | ||
| const manager = (await Manager.deploy( | ||
| gravity.address, | ||
| "0x0000000000000000000000000000000000000000", | ||
| "0x0000000000000000000000000000000000000000" | ||
| )); | ||
| await manager.deployed(); | ||
| console.log(`manager contract deployed at - ${manager.address}`); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the ABI information from the output, I can now generate the final response:
Zero addresses in constructor are incorrect for vault and balancerVault parameters
The constructor parameters for ManagerWithMerkleVerification are:
_owner: Currently set to gravity.address (correct)_vault: Set to zero address but requires valid BoringVault contract address_balancerVault: Set to zero address but requires valid BalancerVault contract address
These addresses are crucial for contract functionality as indicated by contract errors:
ManagerWithMerkleVerification__OnlyCallableByBalancerVaultManagerWithMerkleVerification__OnlyCallableByBoringVault
🔗 Analysis chain
Verify the purpose of zero addresses in constructor.
The contract deployment follows the established pattern, but the use of zero addresses as constructor parameters needs verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the contract's constructor parameters and their purpose
# Search for the contract definition and constructor
ast-grep --pattern 'contract ManagerWithMerkleVerification {
$$$
constructor($$$) {
$$$
}
$$$
}'
# Search for any documentation or comments about these parameters
rg -B 2 -A 2 "constructor.*ManagerWithMerkleVerification"
Length of output: 17485
| function pause() external { | ||
| isPaused = true; | ||
| emit Paused(); | ||
| } |
There was a problem hiding this comment.
Add access control to pause function
The pause function is meant to be callable by MULTISIG_ROLE, but currently, there is no access control enforcing this. This could lead to unauthorized pausing of the contract.
If you have a role-based access control mechanism, apply the appropriate modifier. Otherwise, consider restricting access using the owner for now:
/**
* @notice Pause this contract, which prevents future calls to `manageVaultWithMerkleVerification`.
* @dev Callable by MULTISIG_ROLE.
*/
-function pause() external {
+function pause() external onlyOwner {
isPaused = true;
emit Paused();
}Replace onlyOwner with the correct modifier if using a different access control system.
Committable suggestion skipped: line range outside the PR's diff.
| function setManageRoot(address strategist, bytes32 _manageRoot) external { | ||
| bytes32 oldRoot = manageRoot[strategist]; | ||
| manageRoot[strategist] = _manageRoot; | ||
| emit ManageRootUpdated(strategist, oldRoot, _manageRoot); | ||
| } |
There was a problem hiding this comment.
Add access control to setManageRoot function
The setManageRoot function is intended to be callable by the owner, as indicated by the documentation. However, there is no access control enforcing this.
Apply this diff to add the onlyOwner modifier (assuming it's provided by the Owned contract):
/**
* @notice Sets the manageRoot.
* @dev Callable by OWNER_ROLE.
*/
-function setManageRoot(address strategist, bytes32 _manageRoot) external {
+function setManageRoot(address strategist, bytes32 _manageRoot) external onlyOwner {
bytes32 oldRoot = manageRoot[strategist];
manageRoot[strategist] = _manageRoot;
emit ManageRootUpdated(strategist, oldRoot, _manageRoot);
}This ensures that only the contract owner can call this function, enhancing the security of the contract.
📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function setManageRoot(address strategist, bytes32 _manageRoot) external { | |
| bytes32 oldRoot = manageRoot[strategist]; | |
| manageRoot[strategist] = _manageRoot; | |
| emit ManageRootUpdated(strategist, oldRoot, _manageRoot); | |
| } | |
| function setManageRoot(address strategist, bytes32 _manageRoot) external onlyOwner { | |
| bytes32 oldRoot = manageRoot[strategist]; | |
| manageRoot[strategist] = _manageRoot; | |
| emit ManageRootUpdated(strategist, oldRoot, _manageRoot); | |
| } |
| constructor(address _owner, address _vault, address _balancerVault) Owned(msg.sender) { | ||
| vault = payable(_vault); | ||
| } |
There was a problem hiding this comment.
Fix constructor to initialize the owner correctly and remove unused parameter
In the constructor, you are passing msg.sender to the Owned contract, but you have an _owner parameter. This means the intended owner (_owner) is not being set correctly. Additionally, the _balancerVault parameter is not used.
Apply this diff to fix the constructor:
-contract ManagerWithMerkleVerification is Owned {
+contract ManagerWithMerkleVerification is Owned(_owner) {
address public immutable vault;
- constructor(address _owner, address _vault, address _balancerVault) Owned(msg.sender) {
+ constructor(address _owner, address _vault) {
vault = payable(_vault);
}
}Ensure that you pass _owner to the Owned contract and remove the unused _balancerVault parameter if it is not needed elsewhere in the contract.
Committable suggestion skipped: line range outside the PR's diff.
| function unpause() external { | ||
| isPaused = false; | ||
| emit Unpaused(); | ||
| } |
There was a problem hiding this comment.
Add access control to unpause function
Similar to the pause function, the unpause function should be restricted to MULTISIG_ROLE, but no access control is enforced.
Apply the appropriate access control modifier:
/**
* @notice Unpause this contract, which allows future calls to `manageVaultWithMerkleVerification`.
* @dev Callable by MULTISIG_ROLE.
*/
-function unpause() external {
+function unpause() external onlyOwner {
isPaused = false;
emit Unpaused();
}Ensure that you replace onlyOwner with the correct modifier if using a specific role for multisig access.
Committable suggestion skipped: line range outside the PR's diff.
| sdk "github.com/cosmos/cosmos-sdk/types" | ||
| moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil" |
There was a problem hiding this comment.
Import moduletestutil from the correct module path.
The import path for moduletestutil might need to be updated to match the latest module organization.
Current import:
moduletestutil "github.com/cosmos/cosmos-sdk/types/module/testutil"Consider verifying if the path should be:
moduletestutil "github.com/cosmos/cosmos-sdk/testutil/moduletestutil"| encodingConfig := moduletestutil.MakeTestEncodingConfig( | ||
| auth.AppModuleBasic{}, | ||
| genutil.AppModuleBasic{}, | ||
| bank.AppModuleBasic{}, | ||
| capability.AppModuleBasic{}, | ||
| consensus.AppModuleBasic{}, | ||
| staking.AppModuleBasic{}, | ||
| mint.AppModuleBasic{}, | ||
| distribution.AppModuleBasic{}, | ||
| gov.NewAppModuleBasic( | ||
| []govclient.ProposalHandler{ | ||
| paramsclient.ProposalHandler, | ||
| upgradeclient.LegacyProposalHandler, | ||
| upgradeclient.LegacyCancelProposalHandler, | ||
| gravityclient.ProposalHandler, | ||
| }, | ||
| ), | ||
| //params.AppModuleBasic{}, | ||
| crisis.AppModuleBasic{}, | ||
| slashing.AppModuleBasic{}, | ||
| ibc.AppModuleBasic{}, | ||
| upgrade.AppModuleBasic{}, | ||
| evidence.AppModuleBasic{}, | ||
| ibctransfer.AppModuleBasic{}, | ||
| vesting.AppModuleBasic{}, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure proper module registration in MakeTestEncodingConfig.
When creating the encodingConfig, make sure all necessary modules are registered and configured correctly. Missing or misconfigured modules can lead to issues in message encoding and transaction processing.
Specifically:
- The
params.AppModuleBasic{}is commented out at line 244. If the parameters module is required, uncomment it. - Confirm the order and completeness of the modules passed to
MakeTestEncodingConfig.
Apply this diff to include the params module:
encodingConfig := moduletestutil.MakeTestEncodingConfig(
auth.AppModuleBasic{},
genutil.AppModuleBasic{},
bank.AppModuleBasic{},
capability.AppModuleBasic{},
consensus.AppModuleBasic{},
staking.AppModuleBasic{},
mint.AppModuleBasic{},
distribution.AppModuleBasic{},
gov.NewAppModuleBasic(
[]govclient.ProposalHandler{
paramsclient.ProposalHandler,
upgradeclient.LegacyProposalHandler,
upgradeclient.LegacyCancelProposalHandler,
gravityclient.ProposalHandler,
},
),
- //params.AppModuleBasic{},
+ params.AppModuleBasic{},
crisis.AppModuleBasic{},
slashing.AppModuleBasic{},
ibc.AppModuleBasic{},
upgrade.AppModuleBasic{},
evidence.AppModuleBasic{},
ibctransfer.AppModuleBasic{},
vesting.AppModuleBasic{},
)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| encodingConfig := moduletestutil.MakeTestEncodingConfig( | |
| auth.AppModuleBasic{}, | |
| genutil.AppModuleBasic{}, | |
| bank.AppModuleBasic{}, | |
| capability.AppModuleBasic{}, | |
| consensus.AppModuleBasic{}, | |
| staking.AppModuleBasic{}, | |
| mint.AppModuleBasic{}, | |
| distribution.AppModuleBasic{}, | |
| gov.NewAppModuleBasic( | |
| []govclient.ProposalHandler{ | |
| paramsclient.ProposalHandler, | |
| upgradeclient.LegacyProposalHandler, | |
| upgradeclient.LegacyCancelProposalHandler, | |
| gravityclient.ProposalHandler, | |
| }, | |
| ), | |
| //params.AppModuleBasic{}, | |
| crisis.AppModuleBasic{}, | |
| slashing.AppModuleBasic{}, | |
| ibc.AppModuleBasic{}, | |
| upgrade.AppModuleBasic{}, | |
| evidence.AppModuleBasic{}, | |
| ibctransfer.AppModuleBasic{}, | |
| vesting.AppModuleBasic{}, | |
| ) | |
| encodingConfig := moduletestutil.MakeTestEncodingConfig( | |
| auth.AppModuleBasic{}, | |
| genutil.AppModuleBasic{}, | |
| bank.AppModuleBasic{}, | |
| capability.AppModuleBasic{}, | |
| consensus.AppModuleBasic{}, | |
| staking.AppModuleBasic{}, | |
| mint.AppModuleBasic{}, | |
| distribution.AppModuleBasic{}, | |
| gov.NewAppModuleBasic( | |
| []govclient.ProposalHandler{ | |
| paramsclient.ProposalHandler, | |
| upgradeclient.LegacyProposalHandler, | |
| upgradeclient.LegacyCancelProposalHandler, | |
| gravityclient.ProposalHandler, | |
| }, | |
| ), | |
| params.AppModuleBasic{}, | |
| crisis.AppModuleBasic{}, | |
| slashing.AppModuleBasic{}, | |
| ibc.AppModuleBasic{}, | |
| upgrade.AppModuleBasic{}, | |
| evidence.AppModuleBasic{}, | |
| ibctransfer.AppModuleBasic{}, | |
| vesting.AppModuleBasic{}, | |
| ) |
| var govGenState govtypesv1.GenesisState | ||
| govGenState.Params = &govtypesv1.Params{} | ||
| duration := time.Second * 20 | ||
| govGenState.Params.VotingPeriod = &duration | ||
| govGenState.Params.MinDeposit = sdk.Coins{{Denom: testDenom, Amount: sdk.OneInt()}} |
There was a problem hiding this comment.
Potential Overwrite of Governance Parameters
You are setting govGenState.Params and its fields before unmarshaling the existing genesis state into govGenState. This could lead to your parameter settings being overwritten by the unmarshaled data. To ensure your parameters are preserved, unmarshal first, then set your parameters.
Consider applying this fix:
var govGenState govtypesv1.GenesisState
+ s.Require().NoError(cdc.UnmarshalJSON(appGenState[govtypes.ModuleName], &govGenState))
+ if govGenState.Params == nil {
+ govGenState.Params = &govtypesv1.Params{}
+ }
duration := time.Second * 20
govGenState.Params.VotingPeriod = &duration
govGenState.Params.MinDeposit = sdk.Coins{{Denom: testDenom, Amount: sdk.OneInt()}}
- s.Require().NoError(cdc.UnmarshalJSON(appGenState[govtypes.ModuleName], &govGenState))📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var govGenState govtypesv1.GenesisState | |
| govGenState.Params = &govtypesv1.Params{} | |
| duration := time.Second * 20 | |
| govGenState.Params.VotingPeriod = &duration | |
| govGenState.Params.MinDeposit = sdk.Coins{{Denom: testDenom, Amount: sdk.OneInt()}} | |
| var govGenState govtypesv1.GenesisState | |
| s.Require().NoError(cdc.UnmarshalJSON(appGenState[govtypes.ModuleName], &govGenState)) | |
| if govGenState.Params == nil { | |
| govGenState.Params = &govtypesv1.Params{} | |
| } | |
| duration := time.Second * 20 | |
| govGenState.Params.VotingPeriod = &duration | |
| govGenState.Params.MinDeposit = sdk.Coins{{Denom: testDenom, Amount: sdk.OneInt()}} |
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // For non-anonymous events, the first log topic is a keccak256 hash of the | ||
| // event signature. | ||
| eventSignature := []byte("ManageRootUpdated(address,bytes32,bytes32)") | ||
| manageRootUpdatedEventSignatureTopic := crypto.Keccak256Hash(eventSignature) | ||
| query := ethereum.FilterQuery{ | ||
| FromBlock: nil, | ||
| ToBlock: nil, | ||
| Addresses: []common.Address{ | ||
| managerContract, | ||
| }, | ||
| Topics: [][]common.Hash{ | ||
| { | ||
| manageRootUpdatedEventSignatureTopic, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| logs, err := ethClient.FilterLogs(context.Background(), query) | ||
| if err != nil { | ||
| ethClient.Close() | ||
| return false | ||
| } |
There was a problem hiding this comment.
Add resource cleanup and error handling in Ethereum event queries
Ensure that ethClient.Close() is called in all execution paths, especially when errors occur during log filtering.
Apply this diff to handle errors and prevent resource leaks:
logs, err := ethClient.FilterLogs(context.Background(), query)
if err != nil {
+ s.T().Logf("Error filtering logs: %v", err)
ethClient.Close()
return false
}
+ defer ethClient.Close()Committable suggestion skipped: line range outside the PR's diff.
| s.Require().NoError(err) | ||
| scheduledHeight := currentHeight + 10 | ||
|
|
||
| // Create the cork request to send to Steward | ||
| manageRoot := "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" | ||
| strategist := "0x1111111111111111111111111111111111111111" | ||
| request := &steward_proto.ScheduleRequest{ | ||
| ChainId: 1, | ||
| CellarId: cellarId, | ||
| CallData: &steward_proto.ScheduleRequest_BoringVaultManagerWithMerkleVerification{ | ||
| BoringVaultManagerWithMerkleVerification: &steward_proto.BoringVaultManagerWithMerkleVerification{ | ||
| Function: &steward_proto.BoringVaultManagerWithMerkleVerification_SetManageRoot_{ | ||
| SetManageRoot: &steward_proto.BoringVaultManagerWithMerkleVerification_SetManageRoot{ | ||
| ManageRoot: manageRoot, | ||
| Strategist: strategist, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| BlockHeight: uint64(scheduledHeight), | ||
| } | ||
|
|
||
| s.executeStewardCalls(request) | ||
| s.waitForScheduledHeight(clientCtx, scheduledHeight) | ||
|
|
||
| // Construct invalidation scope and nonce for gravity query | ||
| managerABI, err := ManagerWithMerkleVerificationMetaData.GetAbi() | ||
| s.Require().NoError(err) | ||
|
|
||
| methodName := "setManageRoot" | ||
| var manageRootBytes [32]byte | ||
| manageRootHex, err := hex.DecodeString(manageRoot) | ||
| s.Require().NoError(err) | ||
| copy(manageRootBytes[:], manageRootHex) | ||
| method, err := managerABI.Pack(methodName, common.HexToAddress(strategist), manageRootBytes) | ||
| s.Require().NoError(err) | ||
| addr := common.HexToAddress(cellarId) | ||
| invalidationScope := crypto.Keccak256Hash( | ||
| bytes.Join( | ||
| [][]byte{addr.Bytes(), method}, | ||
| []byte{}, | ||
| )).Bytes() | ||
| invalidationNonce := 1 | ||
|
|
||
| s.queryLogicCallTransaction(clientCtx, invalidationScope, invalidationNonce) | ||
|
|
||
| // For non-anonymous events, the first log topic is a keccak256 hash o f the | ||
| // event signature. | ||
| s.waitForGravityLogicCallEvent(invalidationScope, invalidationNonce) | ||
|
|
||
| s.T().Logf("checking for contract event") | ||
| s.Require().Eventuallyf(func() bool { | ||
| s.T().Log("querying contract events...") | ||
| 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("ManageRootUpdated(address,bytes32,bytes32)") | ||
| manageRootUpdatedEventSignatureTopic := crypto.Keccak256Hash(eventSignature) | ||
| query := ethereum.FilterQuery{ | ||
| FromBlock: nil, | ||
| ToBlock: nil, | ||
| Addresses: []common.Address{ | ||
| managerContract, | ||
| }, | ||
| Topics: [][]common.Hash{ | ||
| { | ||
| manageRootUpdatedEventSignatureTopic, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| logs, err := ethClient.FilterLogs(context.Background(), query) | ||
| if err != nil { | ||
| ethClient.Close() | ||
| return false | ||
| } | ||
| s.T().Logf("got %v logs", len(logs)) | ||
| if len(logs) == 1 { | ||
| s.T().Log("saw ManageRootUpdated event!") | ||
|
|
||
| log := logs[0] | ||
| if len(log.Topics) >= 2 { // First topic is event signature, second is indexed strategist | ||
| strategistAddr := common.HexToAddress(log.Topics[1].Hex()) | ||
| var event ManagerWithMerkleVerificationManageRootUpdated | ||
| err := managerABI.UnpackIntoInterface(&event, "ManageRootUpdated", log.Data) | ||
| s.Require().NoError(err, "failed to unpack ManageRootUpdated event from log data") | ||
| s.Require().Equal(strategist, strategistAddr.Hex()) | ||
| s.Require().Equal(manageRoot, hex.EncodeToString(event.NewRoot[:])) | ||
| return true | ||
| } | ||
| } | ||
|
|
||
| return false | ||
| }, 2*time.Minute, 5*time.Second, "contract event never seen") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Add error handling and resource cleanup in TestBoringVaultManager
Several error checks and resource cleanup steps are missing in the TestBoringVaultManager function. Specifically:
- Missing error handling after dialing the Ethereum client.
- Potential resource leak if
ethClient.Close()is not called in all code paths.
Apply this diff to add error handling and ensure resources are properly closed:
+ s.Require().Eventuallyf(func() bool {
+ s.T().Log("querying contract events...")
+ ethClient, err := ethclient.Dial(fmt.Sprintf("http://%s", s.ethResource.GetHostPort("8545/tcp")))
+ if err != nil {
+ s.T().Logf("Failed to dial eth client: %v", err)
+ return false
+ }
+ defer ethClient.Close()
+
+ // Rest of the code...Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
New Features
ManagerWithMerkleVerification, for enhanced vault management with Merkle verification.ScheduleRequestmessage to support the new vault manager.Bug Fixes
Documentation
Chores