refactor(precompiles): consolidate precompiled contracts into a single package #806
Conversation
WalkthroughThe pull request introduces significant restructuring and enhancements to the cross-chain functionalities within the codebase. Key changes include the renaming of packages from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CrosschainKeeper
participant StakingKeeper
User->>CrosschainKeeper: ExecuteClaim()
CrosschainKeeper->>StakingKeeper: ProcessClaim()
StakingKeeper-->>CrosschainKeeper: ClaimProcessed()
CrosschainKeeper-->>User: ClaimSuccess()
Possibly related PRs
Poem
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: 0
🧹 Outside diff range and nitpick comments (82)
precompiles/crosschain/router.go (2)
Line range hint
11-20: Consider adding capacity hint to map initialization.The implementation is solid, but for better performance in cases where the expected number of routes is known, consider initializing the map with a capacity hint.
func NewRouter() *Router { return &Router{ - routes: make(map[string]crosschainkeeper.Keeper), + routes: make(map[string]crosschainkeeper.Keeper, 10), // adjust capacity based on expected routes } }
Line range hint
22-48: Consider adding mutex for concurrent access protection.While the sealing mechanism is idempotent, concurrent access to
routesmap andsealedflag could lead to race conditions in a concurrent environment.Consider adding a mutex to protect concurrent access:
type Router struct { + sync.RWMutex routes map[string]crosschainkeeper.Keeper sealed bool } func (rtr *Router) Seal() { + rtr.Lock() + defer rtr.Unlock() if rtr.sealed { panic("router already sealed") } rtr.sealed = true }precompiles/crosschain/execute_claim_test.go (1)
Line range hint
26-37: Consider making the test more maintainable.While the test correctly verifies the input packing, using a hardcoded hex string for verification could make the test brittle. Consider adding a complementary unpacking test to verify the round-trip conversion, making the test more robust against potential ABI encoding changes.
func TestExecuteClaimMethod_PackInput(t *testing.T) { executeClaimABI := crosschain.NewExecuteClaimABI() - input, err := executeClaimABI.PackInput(contract.ExecuteClaimArgs{ + args := contract.ExecuteClaimArgs{ Chain: ethtypes.ModuleName, EventNonce: big.NewInt(1), - }) + } + input, err := executeClaimABI.PackInput(args) require.NoError(t, err) - expected := "4ac3bdc30000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000036574680000000000000000000000000000000000000000000000000000000000" - assert.Equal(t, expected, hex.EncodeToString(input)) + + // Verify round-trip conversion + var unpacked contract.ExecuteClaimArgs + err = executeClaimABI.UnpackInput(&unpacked, input) + require.NoError(t, err) + assert.Equal(t, args.Chain, unpacked.Chain) + assert.Equal(t, args.EventNonce.String(), unpacked.EventNonce.String()) }precompiles/crosschain/has_oracle_test.go (1)
Line range hint
22-65: LGTM! Comprehensive test coverage maintained.The test suite effectively covers all scenarios including valid oracle, non-existent oracle, and invalid chain cases. Consider adding a test case for an empty chain name to strengthen edge case coverage.
Add this test case to strengthen coverage:
testCases := []struct { name string malleate func() (contract.HasOracleArgs, error) result bool }{ + { + name: "empty chain", + malleate: func() (contract.HasOracleArgs, error) { + return contract.HasOracleArgs{ + Chain: "", + ExternalAddress: helpers.GenHexAddress(), + }, fmt.Errorf("invalid module name: empty chain name: evm transaction execution failed") + }, + result: false, + }, // existing test cases... }precompiles/crosschain/keeper.go (2)
14-14: Consider using a more descriptive import alias.The alias
types2is not very descriptive and could lead to confusion. Consider using a more meaningful alias that reflects the package's purpose, such asprecompiletypesorpctypes.- types2 "github.com/functionx/fx-core/v8/precompiles/types" + precompiletypes "github.com/functionx/fx-core/v8/precompiles/types"
Line range hint
24-52: Consider emitting events for important state changes.This method performs significant state changes (burning tokens, minting coins, transfers) but doesn't emit any events. Consider emitting events to make it easier to track and audit these operations.
func (c *Keeper) EvmTokenToBaseCoin(ctx sdk.Context, evm *vm.EVM, crosschainKeeper types2.CrosschainKeeper, holder, tokenAddr common.Address, amount *big.Int) (sdk.Coin, error) { erc20Token, err := crosschainKeeper.GetBaseDenomByErc20(ctx, tokenAddr) if err != nil { return sdk.Coin{}, err } baseCoin := sdk.NewCoin(erc20Token.Denom, sdkmath.NewIntFromBigInt(amount)) + defer func() { + if err == nil { + ctx.EventManager().EmitEvent( + sdk.NewEvent( + "evm_token_to_base_coin", + sdk.NewAttribute("holder", holder.String()), + sdk.NewAttribute("token_address", tokenAddr.String()), + sdk.NewAttribute("amount", amount.String()), + sdk.NewAttribute("denom", baseCoin.Denom), + ), + ) + } + }()Consider breaking down the complex logic into helper methods.
The method contains complex branching logic with multiple operations. Consider extracting the native coin and native ERC20 handling into separate helper methods for better readability and maintainability.
Example refactor:
func (c *Keeper) handleNativeCoin(ctx sdk.Context, evm *vm.EVM, holder common.Address, tokenAddr common.Address, erc20Token types2.ERC20Token, baseCoin sdk.Coin, amount *big.Int) error { erc20Call := contract.NewERC20Call(evm, erc20ModuleAddress, tokenAddr, 0) if err := erc20Call.Burn(holder, amount); err != nil { return err } if erc20Token.Denom == fxtypes.DefaultDenom { return c.bankKeeper.SendCoins(ctx, tokenAddr.Bytes(), holder.Bytes(), sdk.NewCoins(baseCoin)) } return nil } func (c *Keeper) handleNativeERC20(ctx sdk.Context, evm *vm.EVM, holder common.Address, tokenAddr common.Address, baseCoin sdk.Coin, amount *big.Int) error { erc20Call := contract.NewERC20Call(evm, erc20ModuleAddress, tokenAddr, 0) if err := erc20Call.TransferFrom(holder, erc20ModuleAddress, amount); err != nil { return err } return c.bankKeeper.MintCoins(ctx, erc20types.ModuleName, sdk.NewCoins(baseCoin)) }precompiles/staking/allowance_shares.go (3)
Line range hint
12-22: Add documentation for better maintainability.The struct design and constructor implementation are solid, following good practices. Consider adding documentation to describe the purpose and responsibilities of
AllowanceSharesMethod.+// AllowanceSharesMethod implements the precompiled contract method for querying +// allowance shares in the staking module. type AllowanceSharesMethod struct { *Keeper AllowanceSharesABI }
Line range hint
35-47: Add input validation and error handling.The Run method needs additional safety checks:
- Validate unpacked args before use
- Handle potential errors from GetAllowance
- Validate the allowance value before packing
func (m *AllowanceSharesMethod) Run(evm *vm.EVM, contract *vm.Contract) ([]byte, error) { args, err := m.UnpackInput(contract.Input) if err != nil { return nil, err } + if args == nil || len(args.GetValidator()) == 0 { + return nil, errors.New("invalid arguments") + } stateDB := evm.StateDB.(types.ExtStateDB) allowance := m.Keeper.stakingKeeper.GetAllowance(stateDB.Context(), args.GetValidator(), args.Owner.Bytes(), args.Spender.Bytes()) + if allowance == nil { + return nil, errors.New("failed to get allowance") + } return m.PackOutput(allowance) }
Line range hint
49-89: Improve maintainability of ABI handling.Consider these improvements:
- Define method name as a constant to avoid magic strings
- Consider injecting stakingABI instead of using a global variable
+const ( + MethodAllowanceShares = "allowanceShares" +) + type AllowanceSharesABI struct { abi.Method } -func NewAllowanceSharesABI() AllowanceSharesABI { +func NewAllowanceSharesABI(stakingABI abi.ABI) AllowanceSharesABI { return AllowanceSharesABI{ - Method: stakingABI.Methods["allowanceShares"], + Method: stakingABI.Methods[MethodAllowanceShares], } }precompiles/crosschain/has_oracle.go (2)
Line range hint
12-21: Add documentation for HasOracleMethod struct and constructor.Consider adding documentation to explain the purpose and responsibilities of this type.
+// HasOracleMethod implements precompiled contract functionality for checking oracle existence type HasOracleMethod struct { *Keeper HasOracleABI } +// NewHasOracleMethod creates a new instance of HasOracleMethod with the provided keeper func NewHasOracleMethod(keeper *Keeper) *HasOracleMethod {
Line range hint
35-48: Enhance error handling and type safety.Consider these improvements:
- Add type assertion safety check for stateDB
- Use a more descriptive error message including available chains
- stateDB := evm.StateDB.(types.ExtStateDB) + stateDB, ok := evm.StateDB.(types.ExtStateDB) + if !ok { + return nil, fmt.Errorf("invalid state database type: expected ExtStateDB") + } router, has := m.Keeper.router.GetRoute(args.Chain) if !has { - return nil, fmt.Errorf("chain not support: %s", args.Chain) + return nil, fmt.Errorf("unsupported chain %q: please use one of the supported chains", args.Chain) }precompiles/crosschain/contract.go (1)
Line range hint
67-93: Consider improving error messages for better debugging.While the error handling is correct, the error messages could be more descriptive to aid in debugging:
- return contract.PackRetErrV2(errors.New("invalid input")) + return contract.PackRetErrV2(errors.New("crosschain: input must be at least 4 bytes")) - return contract.PackRetErrV2(errors.New("write protection")) + return contract.PackRetErrV2(errors.New("crosschain: cannot modify state in read-only mode")) - return contract.PackRetErrV2(errors.New("unknown method")) + return contract.PackRetErrV2(fmt.Errorf("crosschain: unknown method ID %x", vmContract.Input[:4]))precompiles/staking/allowance_shares_test.go (1)
Line range hint
22-89: Consider enhancing test documentation.While the test cases are well-structured, adding comments to describe the purpose of each test case would improve readability and maintainability. Consider documenting:
- The purpose of each test case
- Expected behavior for each scenario
- Any specific edge cases being tested
Example improvement:
testCases := []struct { name string malleate func(val sdk.ValAddress, owner, spender *helpers.Signer) (contract.AllowanceSharesArgs, error) result bool }{ { + // Test successful allowance shares retrieval with valid inputs name: "ok", malleate: func(val sdk.ValAddress, owner, spender *helpers.Signer) (contract.AllowanceSharesArgs, error) {precompiles/staking/delegation.go (3)
Line range hint
13-23: Add nil check for keeper parameterWhile the constructor implementation is correct, it would be safer to validate the keeper parameter to prevent potential nil pointer dereferences during runtime.
func NewDelegationMethod(keeper *Keeper) *DelegationMethod { + if keeper == nil { + panic("keeper cannot be nil") + } return &DelegationMethod{ Keeper: keeper, DelegationABI: NewDelegationABI(), } }
Line range hint
25-35: Document the gas calculation basisThe fixed gas value of 30,000 should be documented to explain how it was determined and what operations it accounts for. This helps with future maintenance and gas optimizations.
func (m *DelegationMethod) RequiredGas() uint64 { + // Gas cost breakdown: + // - Base cost: 20,000 (state read operations) + // - Computation: 10,000 (delegation calculations) return 30_000 }
Line range hint
37-64: Document the delegation amount calculation formulaThe delegation amount calculation involves multiple operations with shares and tokens. This complex calculation should be documented to explain the formula and its purpose.
+ // Calculate delegation amount using the formula: + // amount = (delegation.Shares * validator.Tokens) / validator.DelegatorShares + // This formula ensures the delegation amount is proportional to the + // validator's current voting power and total delegations delegationAmt := delegation.GetShares().MulInt(validator.GetTokens()).Quo(validator.GetDelegatorShares())precompiles/staking/delegation_test.go (1)
Line range hint
23-109: Consider adding edge cases to the test suite.The test suite provides good coverage of basic scenarios. Consider adding test cases for:
- Maximum delegation amounts
- Zero amount delegations
- Multiple delegations to the same validator
Would you like me to help generate these additional test cases?
precompiles/staking/delegation_rewards_test.go (1)
Line range hint
24-95: Consider adding more edge cases to strengthen test coverage.While the existing test cases cover the basic scenarios well, consider adding the following cases to improve coverage:
- Zero delegation amount
- Multiple delegations to the same validator
- Rewards with different token denominations
Here's a suggested test case structure:
{ name: "zero delegation amount", malleate: func(val sdk.ValAddress, del common.Address) (contract.DelegationRewardsArgs, error) { return contract.DelegationRewardsArgs{ Validator: val.String(), Delegator: del, }, nil }, setupDelegation: func() *big.Int { return big.NewInt(0) }, result: true, },precompiles/staking/validator_list_test.go (1)
Line range hint
79-89: Consider extracting validator list creation logic.While the changes correctly update the types to use the new staking package, the test could benefit from some refactoring to improve readability and maintainability.
Consider extracting the validator list creation logic into a helper function:
+func createValidatorList(ctx sdk.Context, stakingKeeper stakingkeeper.Keeper, slashingKeeper slashingkeeper.Keeper, validators []stakingtypes.Validator) ([]staking.Validator, error) { + valList := make([]staking.Validator, 0, len(validators)) + for _, validator := range validators { + consAddr, err := validator.GetConsAddr() + if err != nil { + return nil, err + } + info, err := slashingKeeper.GetValidatorSigningInfo(ctx, consAddr) + if err != nil { + return nil, err + } + valList = append(valList, staking.Validator{ + ValAddr: validator.OperatorAddress, + MissedBlocks: info.MissedBlocksCounter, + }) + } + return valList, nil +} // In the test: -valList := make([]staking.Validator, 0, len(valsByPower)) -for _, validator := range valsByPower { - consAddr, err := validator.GetConsAddr() - suite.Require().NoError(err) - info, err := suite.App.SlashingKeeper.GetValidatorSigningInfo(suite.Ctx, consAddr) - suite.Require().NoError(err) - valList = append(valList, staking.Validator{ - ValAddr: validator.OperatorAddress, - MissedBlocks: info.MissedBlocksCounter, - }) -} +valList, err := createValidatorList(suite.Ctx, suite.App.StakingKeeper, suite.App.SlashingKeeper, valsByPower) +suite.Require().NoError(err)This refactoring would:
- Improve code reusability
- Make the test more focused on assertions
- Better handle error cases
- Make it easier to maintain and modify the validator creation logic
precompiles/crosschain/contract_test.go (3)
Line range hint
28-36: Consider documenting the difference between test entry points.While both test entry points follow standard patterns, it's not immediately clear why there are two separate entry points (
TestCrosschainPrecompileTestSuiteandTestCrosschainPrecompileTestSuite_Contract) or when to use each one.Consider adding comments to explain the purpose and use case for each test entry point:
+// TestCrosschainPrecompileTestSuite runs the test suite with a predefined crosschain address func TestCrosschainPrecompileTestSuite(t *testing.T) { testingSuite := new(CrosschainPrecompileTestSuite) testingSuite.crosschainAddr = common.HexToAddress(contract.CrosschainAddress) suite.Run(t, testingSuite) } +// TestCrosschainPrecompileTestSuite_Contract runs the test suite with a dynamically deployed contract func TestCrosschainPrecompileTestSuite_Contract(t *testing.T) { suite.Run(t, new(CrosschainPrecompileTestSuite)) }
Line range hint
63-81: Consider enhancing oracle setup validation and flexibility.While the
SetOraclemethod is well-structured, consider these improvements:
- Add validation for critical fields like
DelegateValidator- Make the delegation amount configurable rather than hardcoded
Consider this enhancement:
-func (suite *CrosschainPrecompileTestSuite) SetOracle(online bool) crosschaintypes.Oracle { +func (suite *CrosschainPrecompileTestSuite) SetOracle(online bool, opts ...OracleOption) crosschaintypes.Oracle { + // Default values oracle := crosschaintypes.Oracle{ OracleAddress: helpers.GenAccAddress().String(), BridgerAddress: helpers.GenAccAddress().String(), ExternalAddress: helpers.GenExternalAddr(suite.chainName), - DelegateAmount: sdkmath.NewInt(1e18).MulRaw(1000), + DelegateAmount: sdkmath.NewInt(1e18), // Default to 1 token StartHeight: 1, Online: online, DelegateValidator: sdk.ValAddress(helpers.GenAccAddress()).String(), SlashTimes: 0, } + + // Apply options + for _, opt := range opts { + opt(&oracle) + } + + // Validate + if _, err := sdk.ValAddressFromBech32(oracle.DelegateValidator); err != nil { + suite.T().Fatalf("invalid delegate validator address: %v", err) + }
Incorrect comparison in contract deployment condition
The condition is comparing
crosschainAddrwithStakingAddress(0x...1003) when it should be comparing withCrosschainAddress(0x...1004). This is evident from:
- The test suite initializes
crosschainAddr = common.HexToAddress(contract.CrosschainAddress)in the test setup- The condition should check if the address matches its expected precompile address
- if suite.crosschainAddr.String() != contract.StakingAddress { + if suite.crosschainAddr.String() != contract.CrosschainAddress {Regarding the error handling suggestion in the review: the current error handling using
suite.Require().NoError(err)is actually the standard way to handle errors in test suites and is sufficient for this context.🔗 Analysis chain
Line range hint
38-57: Verify contract deployment condition and error handling.Two potential issues in the setup:
- The condition
suite.crosschainAddr.String() != contract.StakingAddressseems unusual. Should this be comparing againstcontract.CrosschainAddressinstead?- The contract deployment error handling could be more robust.
Let's verify the contract addresses:
Consider improving error handling for contract deployment:
- crosschainContract, err := suite.App.EvmKeeper.DeployContract(suite.Ctx, suite.signer.Address(), contract.MustABIJson(testscontract.CrosschainTestMetaData.ABI), contract.MustDecodeHex(testscontract.CrosschainTestMetaData.Bin)) - suite.Require().NoError(err) - suite.crosschainAddr = crosschainContract + crosschainContract, err := suite.App.EvmKeeper.DeployContract( + suite.Ctx, + suite.signer.Address(), + contract.MustABIJson(testscontract.CrosschainTestMetaData.ABI), + contract.MustDecodeHex(testscontract.CrosschainTestMetaData.Bin), + ) + if err != nil { + suite.T().Fatalf("failed to deploy crosschain contract: %v", err) + } + suite.crosschainAddr = crosschainContract🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for StakingAddress and CrosschainAddress usage patterns rg -A 2 "StakingAddress.*=|CrosschainAddress.*="Length of output: 334
Script:
#!/bin/bash # Let's check the test file to understand the context better rg -A 10 "type CrosschainPrecompileTestSuite struct" precompiles/crosschain/Length of output: 762
Script:
#!/bin/bash # Let's also check if there are any other test files using similar setup rg -A 5 "crosschainAddr.*=.*contract\." precompiles/Length of output: 1506
Script:
#!/bin/bash # Check the actual contract deployment usage in other test files ast-grep --pattern 'DeployContract($$$)'Length of output: 42
precompiles/staking/contract.go (1)
29-34: LGTM: Consider adding documentation for the updated keeper parameters.The type changes are consistent with the new modular structure. Consider adding godoc comments to document the expected interfaces and responsibilities of each keeper parameter.
func NewPrecompiledContract( + // bankKeeper handles all bank module interactions bankKeeper types.BankKeeper, stakingKeeper *fxstakingkeeper.Keeper, distrKeeper distrkeeper.Keeper, stakingDenom string, + // govKeeper handles governance-related operations govKeeper types.GovKeeper, + // slashingKeeper manages validator slashing slashingKeeper types.SlashingKeeper, ) *Contract {precompiles/staking/contract_test.go (4)
Line range hint
30-39: Consider more descriptive test function names.While the test initialization is correct,
TestStakingPrecompileTestSuite_Contractcould be more descriptive about its specific purpose (e.g.,TestStakingPrecompileTestSuite_ContractDeployment).
Line range hint
41-63: Enhance setup method robustness and clarity.A few suggestions for improvement:
- The magic number
10insuite.Commit(10)should be defined as a constant with a descriptive name.- Consider adding error handling for the
MintTokenoperation.+ const defaultCommitBlocks = 10 func (suite *StakingPrecompileTestSuite) SetupTest() { suite.MintValNumber = 2 suite.BaseSuite.SetupTest() - suite.Commit(10) + suite.Commit(defaultCommitBlocks)
Line range hint
65-73: Add documentation for helper methods.Consider adding godoc comments to explain the purpose and behavior of
GetDelAddrandIsCallPrecompilemethods, especially the conditions that determine their return values.
Line range hint
82-116: Enhance test method error handling and documentation.The precompile test methods are well-structured but could benefit from:
- More descriptive error messages in assertions
- Documentation of expected success conditions and test scenarios
suite.Require().False(res.Failed(), res.VmError) +// Example of more descriptive error message: +suite.Require().False(res.Failed(), "Delegation failed: %v", res.VmError)precompiles/crosschain/crosschain.go (3)
20-23: Enhance deprecation notice with migration guidance.While the deprecation notice is present, it would be more helpful to include information about the intended replacement or migration path.
Consider expanding the deprecation notice:
-// Deprecated: After the upgrade to v8 +// Deprecated: After the upgrade to v8. Use [NewReplacementType] instead. +// See [migration guide link] for migration instructions.
Line range hint
45-111: Consider breaking down the Run method for better maintainability.The Run method handles multiple responsibilities including:
- Input unpacking
- State management
- Cross-chain operations
- Event emission
Consider extracting these into separate helper methods for better maintainability and testability.
Example structure:
func (m *LegacyCrosschainMethod) Run(evm *vm.EVM, contract *vm.Contract) ([]byte, error) { args, err := m.UnpackInput(contract.Input) if err != nil { return nil, err } if err := m.validateInputs(contract, args); err != nil { return nil, err } if err := m.executeCrossChainOperation(evm, contract, args); err != nil { return nil, err } return m.PackOutput(true) }
130-144: Enhance error handling with wrapped errors.Consider wrapping errors with additional context to aid in debugging.
Example improvement:
func (m LegacyCrosschainABI) UnpackInput(data []byte) (*fxcontract.CrosschainArgs, error) { args := new(fxcontract.CrosschainArgs) if err := evmtypes.ParseMethodArgs(m.Method, args, data[4:]); err != nil { - return nil, err + return nil, fmt.Errorf("failed to unpack crosschain input: %w", err) } return args, nil }precompiles/staking/withdraw_test.go (1)
Line range hint
97-134: Consider adding more edge cases to strengthen test coverageThe current implementation thoroughly validates the withdrawal process, events, and state changes. However, consider adding these test cases to improve coverage:
- Withdrawal with zero rewards
- Multiple withdrawals from the same validator
- Withdrawals after validator slashing
precompiles/staking/redelegate_test.go (2)
Line range hint
21-93: Consider adding more edge cases to test suite.The current test implementation is solid with good coverage of basic scenarios. Consider enhancing it with additional edge cases such as:
- Zero amount redelegation
- Maximum amount redelegation
- Multiple redelegations within unbonding period
Line range hint
95-140: Consider enhancing error handling in event verification.While the event and log verification is thorough, consider adding more detailed error messages when verification fails. For example:
- When expected events are missing
- When attribute values don't match expectations
Example improvement:
existEvent := false +eventFound := false for _, event := range ctx.EventManager().Events() { if event.Type != stakingtypes.EventTypeRedelegate { continue } + eventFound = true for _, attr := range event.Attributes { if attr.Key == stakingtypes.AttributeKeySrcValidator { - suite.Require().Equal(attr.Value, valSrc) + suite.Require().Equal(attr.Value, valSrc, + fmt.Sprintf("source validator mismatch: got %s, want %s", attr.Value, valSrc)) } // Similar improvements for other attributes } existEvent = true } -suite.Require().True(existEvent) +suite.Require().True(eventFound, "redelegate event not found") +suite.Require().True(existEvent, "redelegate event attributes incomplete")precompiles/staking/delegate_test.go (2)
Line range hint
61-67: Consider enhancing ABI test coverage.While the basic structure validation is good, consider adding tests for:
- Input/output parameter types
- Event parameter types and indexing
- Method name and signature validation
Line range hint
155-170: Enhance error reporting in log validation.Consider improving error messages when log validation fails:
- Add descriptive messages for each assertion
- Include actual vs expected values in the error output
Example improvement:
-suite.Require().True(existLog) +suite.Require().True(existLog, "delegate event log not found") -suite.Require().Equal(event.Amount.String(), amount.String()) +suite.Require().Equal(event.Amount.String(), amount.String(), + fmt.Sprintf("delegate amount mismatch: got %s, want %s", + event.Amount.String(), amount.String()))precompiles/crosschain/bridge_call_test.go (1)
Line range hint
147-189: Consider documenting the expected hex values.While the test is thorough, the hardcoded expected values (
expectDataandexpectTopic) could benefit from comments explaining their significance and how they were derived. This would make the test more maintainable and easier to update if the event structure changes.Consider adding comments like:
+ // expectData represents packed event data with: + // - nonce: 100 + // - dstChain: "eth" + // - tokens: [0x04, 0x05] + // - amounts: [123, 456] + // ... expectData := "000000000000..."precompiles/staking/keeper.go (2)
Line range hint
35-166: Consider refactoringhandlerTransferSharesfor better maintainability.The method has high cyclomatic complexity and handles multiple responsibilities. Consider breaking it down into smaller, focused functions:
- Validation logic (validator & delegation checks)
- Reward withdrawal handling
- Delegation updates
- Starting info management
Here's a suggested approach to refactor:
func (k Keeper) handlerTransferShares( ctx sdk.Context, evm *vm.EVM, valAddr sdk.ValAddress, from, to common.Address, sharesInt *big.Int, ) (*big.Int, *big.Int, error) { // 1. Validate transfer if err := k.validateTransfer(ctx, valAddr, from, sharesInt); err != nil { return nil, nil, err } // 2. Handle rewards reward, err := k.handleRewards(ctx, evm, valAddr, from, to) if err != nil { return nil, nil, err } // 3. Update delegations token, err := k.updateDelegations(ctx, valAddr, from, to, sharesInt) if err != nil { return nil, nil, err } return token.BigInt(), reward.BigInt(), nil }
Line range hint
196-206: Improve error handling indecrementReferenceCount.
- Replace panic with proper error handling
- Simplify the if-else block
Suggested improvement:
func (k Keeper) decrementReferenceCount(ctx sdk.Context, valAddr sdk.ValAddress, period uint64) error { historical, err := k.distrKeeper.GetValidatorHistoricalRewards(ctx, valAddr, period) if err != nil { return err } if historical.ReferenceCount == 0 { - panic("cannot set negative reference count") + return errors.New("cannot set negative reference count") } historical.ReferenceCount-- - if historical.ReferenceCount == 0 { - return k.distrKeeper.DeleteValidatorHistoricalReward(ctx, valAddr, period) - } else { + if historical.ReferenceCount > 0 { return k.distrKeeper.SetValidatorHistoricalRewards(ctx, valAddr, period, historical) } + return k.distrKeeper.DeleteValidatorHistoricalReward(ctx, valAddr, period) }precompiles/staking/slashing_info.go (2)
Line range hint
47-47: Use safe type assertion to prevent potential panicsPerforming a direct type assertion with
(types.ExtStateDB)can cause a panic ifevm.StateDBdoes not implementtypes.ExtStateDB. Consider using a safe type assertion and handling the error to ensure robustness.Apply this diff to enhance safety:
-stateDB := evm.StateDB.(types.ExtStateDB) +stateDB, ok := evm.StateDB.(types.ExtStateDB) +if !ok { + return nil, fmt.Errorf("StateDB does not implement types.ExtStateDB") +}
Line range hint
95-99: Add type assertion checks to prevent panics inUnpackOutputType assertions on the unpacked output can panic if the data does not match the expected types. To ensure robustness, check the types before casting to avoid potential runtime errors.
Apply this diff to safely handle type assertions:
func (m SlashingInfoABI) UnpackOutput(data []byte) (bool, *big.Int, error) { unpack, err := m.Method.Outputs.Unpack(data) if err != nil { return false, nil, err } + jailed, ok := unpack[0].(bool) + if !ok { + return false, nil, fmt.Errorf("unexpected type for jailed: %T", unpack[0]) + } + missedBlocks, ok := unpack[1].(*big.Int) + if !ok { + return false, nil, fmt.Errorf("unexpected type for missedBlocks: %T", unpack[1]) + } - return unpack[0].(bool), unpack[1].(*big.Int), nil + return jailed, missedBlocks, nil }precompiles/crosschain/bridge_coin_amount.go (5)
Line range hint
26-27: Potential nil pointer dereference when accessingm.routerIn the
Runmethod,m.routeris accessed to get thecrosschainKeeper. However,routeris not defined withinBridgeCoinAmountMethod, which could lead to a nil pointer dereference. SinceKeeperis embedded, you should accessrouterthroughm.Keeper.Apply this diff to correct the access:
func (m *BridgeCoinAmountMethod) Run(evm *vm.EVM, contract *vm.Contract) ([]byte, error) { args, err := m.UnpackInput(contract.Input) if err != nil { return nil, err } stateDB := evm.StateDB.(evmtypes.ExtStateDB) ctx := stateDB.Context() - crosschainKeeper, found := m.router.GetRoute(ethtypes.ModuleName) + crosschainKeeper, found := m.Keeper.router.GetRoute(ethtypes.ModuleName) if !found { return nil, errors.New("invalid router") } totalSupply, err := crosschainKeeper.BridgeCoinSupply(ctx, args.Token.String(), fxtypes.Byte32ToString(args.Target)) if err != nil { return nil, err } return m.PackOutput(totalSupply.Amount.BigInt()) }
Line range hint
30-31: Avoid panic by safely assertingStateDBtypeThe type assertion
evm.StateDB.(evmtypes.ExtStateDB)could cause a panic ifevm.StateDBdoes not implementExtStateDB. It's safer to use the comma-ok idiom to check the assertion and handle the error appropriately.Apply this diff to handle possible assertion failure:
func (m *BridgeCoinAmountMethod) Run(evm *vm.EVM, contract *vm.Contract) ([]byte, error) { args, err := m.UnpackInput(contract.Input) if err != nil { return nil, err } - stateDB := evm.StateDB.(evmtypes.ExtStateDB) + stateDB, ok := evm.StateDB.(evmtypes.ExtStateDB) + if !ok { + return nil, errors.New("StateDB does not implement ExtStateDB") + } ctx := stateDB.Context() crosschainKeeper, found := m.Keeper.router.GetRoute(ethtypes.ModuleName) if !found { return nil, errors.New("invalid router") }
Line range hint
35-37: Improve error message for better debuggingThe error message
"invalid router"may not provide enough context. Consider making it more descriptive to help with debugging issues related to the router.Update the error message to:
crosschainKeeper, found := m.Keeper.router.GetRoute(ethtypes.ModuleName) if !found { - return nil, errors.New("invalid router") + return nil, errors.New("crosschain keeper not found in router for module 'eth'") }
Line range hint
42-45: Prevent potential panic when slicingdata[4:]In the
UnpackInputmethod, slicingdata[4:]without checking the length ofdatacan cause a panic ifdatais shorter than 4 bytes. Add a length check before slicing to ensure safety.Apply this diff to include the length check:
func (m BridgeCoinAmountABI) UnpackInput(data []byte) (*fxcontract.BridgeCoinAmountArgs, error) { args := new(fxcontract.BridgeCoinAmountArgs) + if len(data) < 4 { + return nil, errors.New("input data too short") + } if err := evmtypes.ParseMethodArgs(m.Method, args, data[4:]); err != nil { return nil, err } return args, nil }
Line range hint
64-68: Enhance error handling inUnpackOutputmethodWhen unpacking the output, the type assertion
amount[0].(*big.Int)could fail if the data is not as expected. Add a check to handle this scenario gracefully.Consider updating the method as follows:
func (m BridgeCoinAmountABI) UnpackOutput(data []byte) (*big.Int, error) { amount, err := m.Method.Outputs.Unpack(data) if err != nil { return nil, err } - return amount[0].(*big.Int), nil + bigIntAmount, ok := amount[0].(*big.Int) + if !ok { + return nil, errors.New("output data is not of type *big.Int") + } + return bigIntAmount, nil }precompiles/staking/delegation_rewards.go (1)
Line range hint
34-34: Handle potential panic from type assertionThe type assertion
stateDB := evm.StateDB.(types.ExtStateDB)could cause a panic ifevm.StateDBdoes not implementtypes.ExtStateDB. It's safer to use the comma-ok idiom to handle this and return an error if the assertion fails.Apply this diff to handle the potential panic:
-stateDB := evm.StateDB.(types.ExtStateDB) +stateDB, ok := evm.StateDB.(types.ExtStateDB) +if !ok { + return nil, errors.New("StateDB does not implement ExtStateDB") +}precompiles/crosschain/execute_claim.go (7)
Line range hint
26-28: Fix Grammar in Error MessageThe error message
"chain not support"has a grammatical error. It should be"chain not supported"for clarity.Apply this diff to correct the error message:
- return nil, errors.New("chain not support") + return nil, errors.New("chain not supported")
Line range hint
34-34: Check for Nil Contract InputBefore unpacking the contract input, ensure that
contract.Inputis not nil to prevent potential panics.Add a nil check for
contract.Input:+ if contract.Input == nil { + return nil, errors.New("contract input is nil") + } args, err := m.UnpackInput(contract.Input)
Line range hint
18-20: Clarify Error Message for Nil RouterThe error message
"bridge call router is empty"might be misleading. Consider changing it to"bridge call router is nil"or"bridge call router is uninitialized"for better clarity.Apply this diff to improve the error message:
- return nil, errors.New("bridge call router is empty") + return nil, errors.New("bridge call router is nil")
Line range hint
43-45: Avoid Reassigning 'err' Within ClosureReassigning the
errvariable inside the closure can lead to confusion and potential bugs. It's advisable to use a different variable name within the closure to maintain clarity.Apply this diff to rename the inner error variable:
- if err = crosschainKeeper.ExecuteClaim(ctx, args.EventNonce.Uint64()); err != nil { + if execErr := crosschainKeeper.ExecuteClaim(ctx, args.EventNonce.Uint64()); execErr != nil { + return execErr + }
Line range hint
53-55: Safely Type Assert Unpacked OutputThe type assertion
success[0].(bool)may panic if the assertion fails. It's safer to check the assertion using the comma-ok idiom to handle unexpected types gracefully.Apply this diff to safely perform the type assertion:
- return success[0].(bool), nil + output, ok := success[0].(bool) + if !ok { + return false, errors.New("failed to unpack output as bool") + } + return output, nil
Line range hint
37-39: Consider Synchronization for 'router' AccessIf
m.routercan be accessed concurrently, it's important to ensure thread safety by adding appropriate synchronization mechanisms to prevent race conditions.Would you like assistance in implementing a thread-safe design for
m.router?
Line range hint
49-51: Handle Errors from Event EmissionThe error returned by
fxcontract.EmitEventis currently not checked. To ensure robustness, consider handling any potential errors from this function.Apply this diff to handle the error:
fxcontract.EmitEvent(evm, crosschainAddress, data, topic) + if err != nil { + return err + }precompiles/staking/approve_shares.go (6)
Line range hint
14-16: Explicit field access enhances readabilityConsider accessing the embedded fields explicitly using
m.Keeperandm.ApproveSharesABI. This improves code readability and makes the codebase easier to maintain.
Line range hint
28-30: Handle type assertion failure when castingevm.StateDBWhen casting
evm.StateDBtotypes.ExtStateDB, there's a risk of a runtime panic if the assertion fails. To enhance robustness, include a type check and handle the error accordingly.Apply this diff to safely perform the type assertion:
- stateDB := evm.StateDB.(types.ExtStateDB) + stateDB, ok := evm.StateDB.(types.ExtStateDB) + if !ok { + return nil, errors.New("evm.StateDB does not implement types.ExtStateDB") + }
Line range hint
34-36: Correct field access forstakingKeeperIn the
Runmethod, you are accessingm.stakingKeeper, butstakingKeeperis likely a field ofKeeper. To prevent potential issues, access it through the embeddedKeeperpointer.Apply this diff to correct the field access:
- m.stakingKeeper.SetAllowance(ctx, args.GetValidator(), owner.Bytes(), args.Spender.Bytes(), args.Shares) + m.Keeper.stakingKeeper.SetAllowance(ctx, args.GetValidator(), owner.Bytes(), args.Spender.Bytes(), args.Shares)If
stakingKeeperis publicly accessible, ensure that it is correctly referenced. Alternatively, ifstakingKeeperis meant to be accessed via a method, adjust the code accordingly.
Line range hint
38-44: Simplify error handling when creating eventsYou can streamline the error handling by using an inline
ifstatement when callingNewApproveSharesEvent. This reduces nesting and enhances code readability.Apply this diff to simplify the code:
- data, topic, err := m.NewApproveSharesEvent(owner, args.Spender, args.Validator, args.Shares) - if err != nil { - return err - } + if data, topic, err := m.NewApproveSharesEvent(owner, args.Spender, args.Validator, args.Shares); err != nil { + return err + }
Line range hint
70-73: Add nil check after parsing the event logAfter calling
filterer.ParseApproveShares(*log), consider checking if the returned event isnilto prevent potentialnilpointer dereferences.Apply this diff to enhance safety:
func (m ApproveSharesABI) UnpackEvent(log *ethtypes.Log) (*fxcontract.IStakingApproveShares, error) { if log == nil { return nil, errors.New("empty log") } filterer, err := fxcontract.NewIStakingFilterer(common.Address{}, nil) if err != nil { return nil, err } - return filterer.ParseApproveShares(*log) + event, err := filterer.ParseApproveShares(*log) + if err != nil { + return nil, err + } + if event == nil { + return nil, errors.New("parsed event is nil") + } + return event, nil }
Line range hint
52-54: Consistent use of pointers for method receiversIn the
ApproveSharesABImethods, you are using value receivers. Consider using pointer receivers to maintain consistency and prevent unnecessary copies, especially if the methods modify the receiver or if the struct contains large fields.Apply this diff to change the method receivers:
- func (m ApproveSharesABI) PackInput(args fxcontract.ApproveSharesArgs) ([]byte, error) { + func (m *ApproveSharesABI) PackInput(args fxcontract.ApproveSharesArgs) ([]byte, error) { - func (m ApproveSharesABI) UnpackInput(data []byte) (*fxcontract.ApproveSharesArgs, error) { + func (m *ApproveSharesABI) UnpackInput(data []byte) (*fxcontract.ApproveSharesArgs, error) { - func (m ApproveSharesABI) PackOutput(result bool) ([]byte, error) { + func (m *ApproveSharesABI) PackOutput(result bool) ([]byte, error) { - func (m ApproveSharesABI) UnpackOutput(data []byte) (bool, error) { + func (m *ApproveSharesABI) UnpackOutput(data []byte) (bool, error) { - func (m ApproveSharesABI) UnpackEvent(log *ethtypes.Log) (*fxcontract.IStakingApproveShares, error) { + func (m *ApproveSharesABI) UnpackEvent(log *ethtypes.Log) (*fxcontract.IStakingApproveShares, error) {precompiles/staking/withdraw.go (3)
Line range hint
38-61: Undefined variablestakingAddressinRunmethodIn the
Runmethod, the variablestakingAddressis used in the call tofxcontract.EmitEvent, but it is not defined within this scope or imported from elsewhere. This will result in a compilation error due to the undefined variable.To fix this issue, ensure that
stakingAddressis properly defined or imported before it is used. If it is a constant representing the staking contract address, you may define it at the package level. For example:var stakingAddress = common.HexToAddress("0x...") // Replace with the correct address
Line range hint
89-92: Add a check for input data length to prevent potential panicIn the
UnpackInputmethod, the code slices the input data usingdata[4:]without checking if the length ofdatais at least 4 bytes. Ifdatais less than 4 bytes, this will cause a runtime panic due to out-of-bounds slicing.To prevent this, add a length check before slicing
data:func (m WithdrawABI) UnpackInput(data []byte) (*fxcontract.WithdrawArgs, error) { if len(data) < 4 { return nil, errors.New("input data too short") } args := new(fxcontract.WithdrawArgs) err := types.ParseMethodArgs(m.Method, args, data[4:]) return args, err }
Line range hint
98-101: Check the length of unpacked output to prevent index out of range panicIn the
UnpackOutputmethod, after unpacking the data, you accessamount[0]without checking ifamounthas at least one element. If the unpacked data is empty, this will cause a runtime panic.Add a length check before accessing
amount[0]:func (m WithdrawABI) UnpackOutput(data []byte) (*big.Int, error) { amount, err := m.Method.Outputs.Unpack(data) if err != nil { return nil, err } if len(amount) == 0 { return nil, errors.New("no output data unpacked") } return amount[0].(*big.Int), nil }precompiles/staking/undelegate.go (3)
Line range hint
31-33: Handle potential type assertion failure forevm.StateDB.Currently, the code performs a direct type assertion on
evm.StateDBtotypes.ExtStateDBwithout checking. Ifevm.StateDBdoes not implementtypes.ExtStateDB, this will result in a runtime panic.Apply this diff to safely handle the type assertion:
func (m *UndelegateV2Method) Run(evm *vm.EVM, contract *vm.Contract) ([]byte, error) { args, err := m.UnpackInput(contract.Input) if err != nil { return nil, err } - stateDB := evm.StateDB.(types.ExtStateDB) + stateDB, ok := evm.StateDB.(types.ExtStateDB) + if !ok { + return nil, errors.New("StateDB does not implement ExtStateDB") + }
Line range hint
67-70: Prevent potential slice out-of-bounds error when unpacking input.The function
UnpackInputaccessesdata[4:]without checking ifdatahas at least 4 bytes. Ifdatais less than 4 bytes, this will cause a slice out-of-bounds panic. It's advisable to check the length ofdatabefore slicing.Apply this diff to fix the issue:
func (m UndelegateABI) UnpackInput(data []byte) (*fxcontract.UndelegateV2Args, error) { + if len(data) < 4 { + return nil, errors.New("input data too short") + } args := new(fxcontract.UndelegateV2Args) err := types.ParseMethodArgs(m.Method, args, data[4:]) return args, err }
Line range hint
75-79: Check for empty output before accessingamount[0].In
UnpackOutput, the code accessesamount[0]without ensuring thatamounthas at least one element. Ifamountis empty, this will cause an index out-of-range panic.Apply this diff to prevent a potential panic:
func (m UndelegateABI) UnpackOutput(data []byte) (bool, error) { amount, err := m.Method.Outputs.Unpack(data) if err != nil { return false, err } + if len(amount) == 0 { + return false, errors.New("unpacked output is empty") + } return amount[0].(bool), nil }precompiles/staking/redelegate.go (3)
Line range hint
23-23: Avoid Unchecked Type Assertion to Prevent Potential PanicThe type assertion
evm.StateDB.(types.ExtStateDB)is unchecked and could cause a panic ifevm.StateDBdoes not implementtypes.ExtStateDB. It's safer to use the comma-ok idiom to check the assertion and handle the error appropriately.Apply this diff to safely perform the type assertion:
- stateDB := evm.StateDB.(types.ExtStateDB) + stateDB, ok := evm.StateDB.(types.ExtStateDB) + if !ok { + return nil, errors.New("StateDB does not implement ExtStateDB") + }
Line range hint
65-68: Ensure Input Data Length Before Slicing to Prevent Index Out of RangeIn the
UnpackInputfunction, slicingdata[4:]without checking ifdatahas at least 4 bytes may cause an index out of range error if the input data is too short.Consider adding a length check before slicing:
func (m RedelegateABI) UnpackInput(data []byte) (*fxcontract.RedelegateV2Args, error) { args := new(fxcontract.RedelegateV2Args) + if len(data) < 4 { + return nil, errors.New("input data too short") + } err := types.ParseMethodArgs(m.Method, args, data[4:]) return args, err }
Line range hint
73-77: Passingnilas Backend May Cause Nil Pointer DereferenceIn the
UnpackEventfunction, you are passingnilas the backend tofxcontract.NewIStakingFilterer. If this function expects a non-nil backend, this could lead to a nil pointer dereference when methods of the backend are called.Consider providing a valid backend or handling the case where the backend is
nil.precompiles/crosschain/bridge_call.go (2)
Line range hint
81-82: Typo in parameter name 'nonceNonce' in 'PackOutput'The parameter name
nonceNoncein thePackOutputmethod appears to be a typo. It should likely benonceto reflect its purpose and for consistency with the rest of the code.Apply this diff to correct the parameter name:
-func (m BridgeCallABI) PackOutput(nonceNonce *big.Int) ([]byte, error) { - return m.Method.Outputs.Pack(nonceNonce) +func (m BridgeCallABI) PackOutput(nonce *big.Int) ([]byte, error) { + return m.Method.Outputs.Pack(nonce) }
Line range hint
43-89: Consider refactoring theRunmethod for improved readabilityThe
Runmethod is lengthy and contains nested logic, which can affect readability and maintainability. Consider refactoring portions of the code into smaller helper functions to enhance clarity and manage complexity.precompiles/types/expected_keepers.go (4)
Line range hint
88-90: Consistency in Context UsageThe
EvmKeeperinterface usessdk.Contextinstead ofcontext.Context. For consistency:
- Verify if
sdk.Contextis intentionally used here while other interfaces usecontext.Context.- If possible, standardize the context usage across all interfaces for consistency and clarity.
Consider refactoring to use
context.Contextif appropriate, or document the reasoning behind usingsdk.Context.
Line range hint
54-81: Error Handling inStakingKeeperInterface MethodsSeveral methods in the
StakingKeeperinterface return error types. Ensure that:
- All possible errors are properly handled in the implementations.
- The methods that modify state (
SetDelegation,RemoveDelegation, etc.) have appropriate transactional behavior.- Edge cases, such as delegating zero amounts or delegations to non-existent validators, are correctly managed.
Ensure that the implementations prevent potential issues like double spending, inconsistent delegation states, or race conditions in concurrent environments.
Line range hint
83-86: Clarify the Purpose ofGetParamsinEvmKeeperThe method
GetParams(ctx sdk.Context) evmtypes.Paramsretrieves the EVM parameters. Verify that:
- The parameters returned are up-to-date and reflect the current state of the EVM module.
- Consumers of this method handle the parameters correctly, especially regarding security and gas configurations.
Consider adding documentation to
GetParamsto specify what parameters are returned and how they should be used.
15-17: Unused Import StatementThe import statement for
crosschaintypeson line 16 is added. Verify that:
- This import is necessary for the code within this file.
- All imported packages are utilized; otherwise, remove unused imports to keep the codebase clean.
If
crosschaintypesis not used within this file, consider removing the import.precompiles/staking/transfer_shares.go (3)
Line range hint
38-40: Consider increasing the required gas based on the method's complexity.The
RequiredGasmethod returns a fixed value of 50,000. However, given the complexity of theRunmethod, which involves unpacking input, executing a native action on the state database, and emitting events, the gas requirement might need to be higher to account for the computational cost.
Line range hint
42-70: Ensure error handling is consistent and informative.The
Runmethod handles errors in multiple places:
- When unpacking the input arguments
- When executing the native action on the state database
- When creating the transfer share event
In each case, the error is returned to the caller. Consider adding more context to the returned errors to assist with debugging. For example:
if err != nil { return nil, fmt.Errorf("failed to unpack input arguments: %w", err) }Additionally, verify that the errors returned by the
handlerTransferSharesandNewTransferShareEventmethods are sufficiently informative for effective error handling and debugging.
Line range hint
91-153: TheTransferFromSharesMethodimplementation looks good, with a minor suggestion.The
TransferFromSharesMethodstruct and its associated methods are well-structured and handle the necessary operations for transferring shares from one account to another. TheRunmethod correctly unpacks the input arguments, decrements the allowance, executes the transfer, and emits the appropriate events.One suggestion for improvement:
In the
Runmethod, consider moving the event emission and result packing outside theExecuteNativeActionclosure. This would make the code more readable and easier to maintain.app/keepers/keepers.go (5)
66-67: Consider aliasing imports for consistencyThe import
"github.com/functionx/fx-core/v8/precompiles/crosschain"is not aliased, while"github.com/functionx/fx-core/v8/precompiles/staking"is aliased asstakingprecompile. For consistency and clarity, consider aliasing thecrosschainimport.Apply this diff to alias the import:
-"github.com/functionx/fx-core/v8/precompiles/crosschain" +crosschainprecompile "github.com/functionx/fx-core/v8/precompiles/crosschain"
353-353: Update variable name to reflect import aliasIf you alias the
crosschainimport ascrosschainprecompile, consider renamingcrosschainPrecompileRouterto align with the alias for clarity and consistency.Apply this diff:
-crosschainPrecompileRouter := crosschain.NewRouter() +crosschainPrecompileRouter := crosschainprecompile.NewRouter()
367-369: Ensure consistency with import aliases in function callsIn the function call to
crosschain.NewPrecompiledContract, use the import alias if you've aliasedcrosschainto maintain consistency.Apply this diff:
-return crosschain.NewPrecompiledContract( +return crosschainprecompile.NewPrecompiledContract(
Line range hint
402-470: Reduce code duplication by abstracting keeper initializationThe initialization of cross-chain keepers for different modules repeats the same pattern. Consider creating a helper function to abstract this logic, which will improve maintainability and reduce potential errors from code duplication.
Here is a possible refactor:
func initCrosschainKeeper(moduleName string, storeKey storetypes.StoreKey) crosschainkeeper.Keeper { return crosschainkeeper.NewKeeper( appCodec, moduleName, appKeepers.keys[storeKey], appKeepers.StakingKeeper, stakingkeeper.NewMsgServerImpl(appKeepers.StakingKeeper.Keeper), distrkeeper.NewMsgServerImpl(appKeepers.DistrKeeper), appKeepers.BankKeeper, appKeepers.IBCTransferKeeper, appKeepers.Erc20Keeper, appKeepers.AccountKeeper, appKeepers.EvmKeeper, contract.NewBridgeFeeQuoteKeeper(appKeepers.EvmKeeper, contract.BridgeFeeAddress), contract.NewERC20TokenKeeper(appKeepers.EvmKeeper), authAddr, ) } // Then initialize the keepers like: appKeepers.BscKeeper = initCrosschainKeeper(bsctypes.ModuleName, bsctypes.StoreKey) appKeepers.PolygonKeeper = initCrosschainKeeper(polygontypes.ModuleName, polygontypes.StoreKey) // ...and so on for other chains
Line range hint
552-559: Handle potential panic inGetSubspacefunctionThe
GetSubspacefunction panics if a subspace is not found. To improve robustness and error handling, consider returning an error instead of panicking.Apply this diff:
-func (appKeepers *AppKeepers) GetSubspace(moduleName string) paramstypes.Subspace { - subspace, ok := appKeepers.ParamsKeeper.GetSubspace(moduleName) - if !ok { - panic("couldn't load subspace for module: " + moduleName) - } - return subspace +func (appKeepers *AppKeepers) GetSubspace(moduleName string) (paramstypes.Subspace, error) { + subspace, ok := appKeepers.ParamsKeeper.GetSubspace(moduleName) + if !ok { + return paramstypes.Subspace{}, fmt.Errorf("couldn't load subspace for module: %s", moduleName) + } + return subspace, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (45)
app/keepers/keepers.go(3 hunks)precompiles/crosschain/bridge_call.go(1 hunks)precompiles/crosschain/bridge_call_test.go(5 hunks)precompiles/crosschain/bridge_coin_amount.go(1 hunks)precompiles/crosschain/bridge_coin_amount_test.go(1 hunks)precompiles/crosschain/contract.go(3 hunks)precompiles/crosschain/contract_test.go(1 hunks)precompiles/crosschain/crosschain.go(3 hunks)precompiles/crosschain/crosschain_test.go(1 hunks)precompiles/crosschain/execute_claim.go(1 hunks)precompiles/crosschain/execute_claim_test.go(3 hunks)precompiles/crosschain/has_oracle.go(1 hunks)precompiles/crosschain/has_oracle_test.go(2 hunks)precompiles/crosschain/is_oracle_online.go(1 hunks)precompiles/crosschain/is_oracle_online_test.go(2 hunks)precompiles/crosschain/keeper.go(2 hunks)precompiles/crosschain/router.go(2 hunks)precompiles/staking/allowance_shares.go(1 hunks)precompiles/staking/allowance_shares_test.go(2 hunks)precompiles/staking/approve_shares.go(1 hunks)precompiles/staking/approve_shares_test.go(3 hunks)precompiles/staking/contract.go(3 hunks)precompiles/staking/contract_test.go(1 hunks)precompiles/staking/delegate.go(1 hunks)precompiles/staking/delegate_test.go(4 hunks)precompiles/staking/delegation.go(1 hunks)precompiles/staking/delegation_rewards.go(1 hunks)precompiles/staking/delegation_rewards_test.go(2 hunks)precompiles/staking/delegation_test.go(2 hunks)precompiles/staking/keeper.go(3 hunks)precompiles/staking/redelegate.go(1 hunks)precompiles/staking/redelegate_test.go(3 hunks)precompiles/staking/slashing_info.go(1 hunks)precompiles/staking/slashing_info_test.go(2 hunks)precompiles/staking/transfer_shares.go(1 hunks)precompiles/staking/transfer_shares_test.go(5 hunks)precompiles/staking/undelegate.go(1 hunks)precompiles/staking/undelegate_test.go(3 hunks)precompiles/staking/validator_list.go(1 hunks)precompiles/staking/validator_list_test.go(3 hunks)precompiles/staking/withdraw.go(1 hunks)precompiles/staking/withdraw_test.go(3 hunks)precompiles/types/expected_keepers.go(2 hunks)tests/integration/staking_precompile_suite.go(2 hunks)x/crosschain/precompile/expected_keepers.go(0 hunks)
💤 Files with no reviewable changes (1)
- x/crosschain/precompile/expected_keepers.go
✅ Files skipped from review due to trivial changes (3)
- precompiles/crosschain/bridge_coin_amount_test.go
- precompiles/crosschain/is_oracle_online.go
- precompiles/staking/validator_list.go
🔇 Additional comments (101)
precompiles/crosschain/crosschain_test.go (2)
1-1: Package restructuring aligns with consolidation objectives.
The package renaming and import path changes effectively support the PR's goal of consolidating precompiled contracts into a single package. The new structure provides better organization and clarity.
Also applies to: 8-8
Line range hint 12-17: Test implementation remains robust after package restructuring.
The test continues to verify critical aspects of the crosschain ABI:
- Validates the expected number of inputs (6) and outputs (1) for the Method
- Confirms the correct number of Event inputs (8)
The test maintains its effectiveness while adapting to the new package structure.
precompiles/crosschain/router.go (3)
1-9: LGTM! Package rename aligns with consolidation objectives.
The package rename and import changes effectively support the consolidation of precompiled contracts while maintaining clean separation of concerns.
51-54: LGTM! Clean interface abstraction with types.CrosschainKeeper.
The return type change to types.CrosschainKeeper improves the abstraction layer and aligns with the package reorganization.
51-51: Verify the impact of return type change.
The change in return type from CrosschainKeeper to types.CrosschainKeeper might affect existing callers.
✅ Verification successful
Let me gather more information about the type definitions to understand the impact better.
Based on the gathered information, I can now generate the final response:
Return type change is safe and properly aligned with the codebase
The change from CrosschainKeeper to types.CrosschainKeeper is actually a correction that properly references the interface defined in precompiles/types/expected_keepers.go. All existing callers of GetRoute() are already using the return value correctly according to this interface definition, as shown in the usage patterns across:
precompiles/crosschain/is_oracle_online.goprecompiles/crosschain/has_oracle.goprecompiles/crosschain/execute_claim.goprecompiles/crosschain/bridge_coin_amount.goprecompiles/crosschain/bridge_call.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes due to return type change
# Test: Search for GetRoute method calls that might need updates
rg -A 2 "GetRoute\(" --type go
Length of output: 2753
Script:
#!/bin/bash
# Check the definition of CrosschainKeeper interface
ast-grep --pattern 'type CrosschainKeeper interface {
$$$
}'
# Also check for any other potential interface definitions
rg "type.*CrosschainKeeper" --type go
Length of output: 2898
precompiles/crosschain/execute_claim_test.go (2)
Line range hint 1-14: LGTM! Package restructuring aligns with PR objectives.
The package rename and import path changes correctly reflect the consolidation of precompiled contracts into a single package.
Line range hint 16-24: LGTM! Test maintains functionality while adapting to new package structure.
The test correctly verifies both the method and event string formats using the updated package reference.
precompiles/crosschain/has_oracle_test.go (3)
Line range hint 1-14: LGTM! Package and import changes align with the refactoring objectives.
The package rename and import updates correctly reflect the consolidation of precompiled contracts into a single package.
16-20: LGTM! ABI instantiation correctly updated.
The test maintains its validation of the ABI structure while using the new package reference.
Line range hint 1-65: Verify consistent test file updates across the codebase.
Since this is part of a larger refactoring effort, let's verify that other test files have been updated consistently.
✅ Verification successful
Package naming and test structure are consistent across crosschain tests
The verification shows that:
- All test files in the
precompiles/crosschaindirectory consistently usepackage crosschain_test - The test suite structure is properly defined in
contract_test.goand reused across the test files - No remaining references to the old
precompile_testpackage were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old package name in test files
# and verify consistent package naming in the new structure.
echo "Checking for any remaining 'package precompile_test' declarations..."
rg -l "package precompile_test"
echo "Verifying consistent package naming in crosschain test files..."
rg -l "package" precompiles/crosschain/*_test.go
Length of output: 959
Script:
#!/bin/bash
# Let's check the package declarations in these test files to ensure consistency
echo "Checking package declarations in crosschain test files..."
for file in precompiles/crosschain/*_test.go; do
echo "=== $file ==="
rg "^package" "$file" -A 1
done
# Also check test suite struct definitions for consistency
echo -e "\nChecking test suite definitions..."
ast-grep --pattern 'type $_ struct {
$$$
}' precompiles/crosschain/*_test.go
Length of output: 3557
precompiles/crosschain/keeper.go (1)
19-22: LGTM! Clean and well-structured Keeper definition.
The Keeper struct is minimal, focused, and follows Go conventions for field ordering.
precompiles/staking/allowance_shares.go (2)
Line range hint 1-10: Package rename aligns with consolidation objectives.
The package rename from precompile to staking properly reflects the domain-specific functionality of this module.
Line range hint 24-33: Verify the hardcoded gas cost.
The gas cost of 5000 for this operation needs justification. Please verify if this value:
- Covers the worst-case scenario
- Is consistent with similar operations in other precompiles
- Has been benchmarked
✅ Verification successful
Let me analyze the gas costs across precompiles to make a proper comparison. I'll search for the implementation of AllowanceShares to understand its complexity.
Let me try a different approach to find the implementation.
Gas cost of 5000 for allowanceShares is reasonable and consistent
The gas cost of 5000 is appropriate for this operation because:
- It's a read-only operation that only retrieves allowance data using
GetAllowance - It's more expensive than simple queries (1000 gas) due to multiple state reads (validator, owner, spender)
- It's cheaper than write operations like
approveShares(10000 gas) which is its complementary write operation - The implementation shows minimal computation overhead, mainly focused on state access
Looking at similar read operations in the codebase:
- Simple queries (validator list, slashing info): 1000 gas
- Delegation queries: 30000 gas
- This allowance query: 5000 gas
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other gas costs in precompiles for comparison
rg -A 1 "RequiredGas\(\) uint64" | rg "return \d+"
Length of output: 996
Script:
#!/bin/bash
# Search for AllowanceShares implementation
ast-grep --pattern 'func (m *AllowanceSharesMethod) Run($_) ([]byte, error) {
$$$
}'
Length of output: 88
Script:
#!/bin/bash
# Search for AllowanceShares implementation using ripgrep
rg -A 20 "func \(m \*AllowanceSharesMethod\) Run"
Length of output: 1455
precompiles/crosschain/has_oracle.go (3)
Line range hint 1-10: LGTM! Package rename aligns with refactoring objectives.
The package rename from precompile to crosschain and the imports are well-structured for the intended functionality.
Line range hint 23-33: Verify the hardcoded gas requirement.
The gas requirement is hardcoded to 1000. Consider:
- Adding a comment explaining why this specific value was chosen
- Making it configurable if the gas cost might need adjustment in the future
✅ Verification successful
The search results show a clear pattern of gas costs across different precompile methods. I can now generate a final response:
Hardcoded gas value of 1000 is consistent with similar read-only operations
The gas requirement of 1000 is consistent with other read-only operations in the precompiles like validator_list and is_oracle_online. Higher gas values (30000-60000) are used for state-modifying operations like delegations, withdrawals, and transfers. Consider adding a comment to document this pattern:
// RequiredGas returns 1000 which is the standard gas cost for read-only oracle queries
func (m *HasOracleMethod) RequiredGas() uint64 {
return 1_000
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other similar gas requirements in precompiles
rg -A 1 "RequiredGas\(\) uint64" | rg "return \d+"
Length of output: 996
Line range hint 50-86: Verify the crosschainABI global variable initialization.
The code relies on a global crosschainABI variable. Let's ensure it's properly initialized.
Otherwise, the ABI implementation looks solid with proper error handling and type safety.
✅ Verification successful
The crosschainABI global variable is properly initialized
The variable is correctly initialized in precompiles/crosschain/contract.go using contract.MustABIJson(contract.ICrosschainMetaData.ABI). This is a safe initialization pattern that happens at package level and will panic if the ABI is invalid, preventing any runtime issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for crosschainABI initialization
rg "var crosschainABI\b" -A 5
Length of output: 31
Script:
#!/bin/bash
# Let's try different search patterns to find crosschainABI initialization
# Search for crosschainABI as a variable or constant
rg "crosschainABI\s*[:=]" -A 5
# Also search for any abi.JSON initialization that might contain crosschain
rg "abi\.JSON|abi\.MustUnmarshalJSON" -A 5
# Search for any file that might contain crosschain ABI definitions
fd "crosschain.*abi"
Length of output: 1184
precompiles/staking/slashing_info_test.go (2)
Line range hint 1-13: LGTM: Package restructuring looks good!
The package rename and import path updates align well with the PR objective of consolidating precompiled contracts into a single package.
Line range hint 15-20: Verify consistent usage of NewSlashingInfoABI across the codebase.
The change from precompile.NewSlashingInfoABI() to staking.NewSlashingInfoABI() looks good, but let's ensure all references are updated.
✅ Verification successful
Let me gather more information to verify if there are any other references to NewSlashingInfoABI in the codebase.
All references to NewSlashingInfoABI are correctly using the staking package
The search results confirm that:
- There are no remaining references to
precompile.NewSlashingInfoABI - The only usage of
NewSlashingInfoABIis within thestakingpackage - The function is properly defined in
slashing_info.goand used in both the implementation and test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all references to NewSlashingInfoABI have been updated
# to use the new staking package.
# Search for any remaining references to precompile.NewSlashingInfoABI
echo "Checking for old references to precompile.NewSlashingInfoABI:"
rg "precompile\.NewSlashingInfoABI"
# Search for new references to staking.NewSlashingInfoABI
echo -e "\nVerifying new references to staking.NewSlashingInfoABI:"
rg "staking\.NewSlashingInfoABI"
Length of output: 423
Script:
#!/bin/bash
# Search for any function definition or usage of NewSlashingInfoABI
echo "Searching for any NewSlashingInfoABI function definitions or usages:"
rg "NewSlashingInfoABI" -A 3
# Search for any slashing info related ABI usage
echo -e "\nSearching for slashing info ABI related code:"
rg "slashingInfo.*ABI" -i
Length of output: 16747
precompiles/crosschain/contract.go (2)
1-1: LGTM! Package restructuring aligns with PR objectives.
The package rename and new import structure improve code organization by consolidating precompiled contracts into a dedicated package while maintaining proper type separation.
Also applies to: 11-11
22-22: Verify type changes across the codebase.
The type changes from GovKeeper to types.GovKeeper look good, but we should verify that all callers have been updated accordingly.
Also applies to: 26-27
✅ Verification successful
Based on the results, I can generate the final response since I have enough information to verify the type changes.
Type changes for GovKeeper have been properly propagated
All instantiations of NewPrecompiledContract in app/keepers/keepers.go correctly pass appKeepers.GovKeeper which matches the new types.GovKeeper parameter type. The remaining GovKeeper reference in app/app.go is unrelated to this change as it's part of a struct initialization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all instantiations of Contract use the new types
# Search for NewPrecompiledContract calls
rg -A 3 "NewPrecompiledContract\("
# Search for any remaining references to the old GovKeeper type
ast-grep --pattern 'GovKeeper'
Length of output: 1220
precompiles/crosschain/is_oracle_online_test.go (3)
Line range hint 1-14: LGTM! Package and import changes align with the refactoring objectives.
The package rename from precompile_test to crosschain_test and the corresponding import updates are consistent with the PR's goal of consolidating precompiled contracts.
Line range hint 22-89: LGTM! Comprehensive test coverage maintained.
The test cases thoroughly cover all important scenarios:
- Oracle online state
- Oracle offline state
- Non-existent oracle
- Invalid chain handling
The test structure and error handling remain robust.
16-20: LGTM! Verify ABI consistency across the codebase.
The update to use crosschain.NewIsOracleOnlineABI() is consistent with the package restructuring.
Let's verify that the ABI structure is consistent across the codebase:
✅ Verification successful
Based on the gathered information, I can now provide a final response:
ABI structure verified and consistent across the codebase
The IsOracleOnline ABI implementation is consistent:
- The ABI structure matches with 2 inputs (
_chainand_externalAddress) as defined incontract/crosschain.go - The test correctly verifies both inputs and single output
- The implementation in
precompiles/crosschain/is_oracle_online.goproperly uses this structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of IsOracleOnline ABI implementation
# Expected: All implementations should have 2 inputs and 1 output
# Search for IsOracleOnline ABI definitions
ast-grep --pattern 'NewIsOracleOnlineABI() $_ {
$$$
}'
Length of output: 58
Script:
#!/bin/bash
# Let's try a different approach to find the ABI implementation
# First, let's find the file containing the ABI definition
rg -l "NewIsOracleOnlineABI"
# Then let's look at the actual implementation
rg "NewIsOracleOnlineABI" -A 10
Length of output: 2515
Script:
#!/bin/bash
# Let's examine the ABI definition in the crosschainABI variable
rg "crosschainABI.*Methods\[\"isOracleOnline\"\]" -B 10
# Also check the contract package for the args structure
rg "type IsOracleOnlineArgs struct" -A 5
Length of output: 1183
Script:
#!/bin/bash
# Let's find and examine the crosschainABI variable definition
rg "var crosschainABI" -A 20
# Also check if there are any other files using this ABI to ensure consistency
rg "isOracleOnline.*Inputs" -A 2
Length of output: 316
precompiles/staking/allowance_shares_test.go (2)
Line range hint 1-13: LGTM! Package restructuring looks good.
The package rename and import path changes align well with the PR objective of consolidating precompiled contracts into a single package.
Line range hint 15-20: LGTM! ABI instantiation updated correctly.
The test maintains its validation of ABI structure while using the new package path.
precompiles/staking/delegation.go (2)
Line range hint 1-11: LGTM: Package structure and imports are well-organized
The package name change to staking aligns with the PR objective of consolidating precompiles, and the imports appropriately cover both Cosmos SDK and Ethereum dependencies needed for the functionality.
Line range hint 66-99: Verify stakingABI initialization
The code references stakingABI.Methods, but the initialization of stakingABI is not visible in this file.
✅ Verification successful
stakingABI is properly initialized
The stakingABI variable is correctly initialized in precompiles/staking/contract.go as a package-level variable:
var (
stakingAddress = common.HexToAddress(contract.StakingAddress)
stakingABI = contract.MustABIJson(contract.IStakingMetaData.ABI)
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for stakingABI initialization
rg -A 5 "var stakingABI" --type go
Length of output: 36
Script:
#!/bin/bash
# Let's try different approaches to find stakingABI
# 1. Search for stakingABI as a variable or constant
rg "stakingABI" --type go
# 2. Search for any ABI initialization in staking related files
fd "staking.*\.go$" --exec grep -l "ABI"
# 3. Look specifically in the precompiles/staking directory
ls -la precompiles/staking/
Length of output: 3729
Script:
#!/bin/bash
# Let's examine the contract.go file where stakingABI is initialized
cat precompiles/staking/contract.go
Length of output: 3003
precompiles/staking/delegation_test.go (2)
Line range hint 1-14: LGTM: Package restructuring is well-aligned with PR objectives.
The package rename and import updates correctly reflect the consolidation of precompiled contracts into a more organized structure.
Line range hint 16-21: LGTM: Verify ABI compatibility across package move.
The test maintains proper validation of the ABI structure. Let's verify that the ABI remains consistent across all references.
✅ Verification successful
Let me gather more information about the ABI usage and implementation to ensure compatibility.
Based on the results, I can now generate the final response:
✓ ABI compatibility verified after package move
The verification shows:
NewDelegationABI()is properly implemented in the newstakingpackage- No references to the old precompile package remain
- All delegation ABI usages are consistently using the new package location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent ABI usage across the codebase
# Expect: All ABI references should use the new staking package
# Search for any remaining references to the old precompile package
rg -l "precompile\.NewDelegationABI"
# Search for all current references to ensure consistent usage
rg -l "staking\.NewDelegationABI"
Length of output: 113
Script:
#!/bin/bash
# Check the implementation of NewDelegationABI in both old and new locations
ast-grep --pattern 'func NewDelegationABI() $$$'
# Look for any other delegation ABI related code
rg -l "DelegationABI"
# Check for any imports of the old precompile package
rg -l "\"github\.com/.*?/precompile\""
Length of output: 481
precompiles/staking/delegation_rewards_test.go (2)
1-1: LGTM: Package restructuring looks good!
The package name change to staking_test and the updated import path align well with the PR objective of consolidating precompiled contracts.
Also applies to: 13-13
Line range hint 17-22: LGTM: ABI test is well-structured!
The test appropriately validates the ABI structure with proper assertions for inputs and outputs.
precompiles/staking/validator_list_test.go (2)
Line range hint 1-13: LGTM: Package restructuring aligns with PR objectives.
The package rename and import updates correctly reflect the consolidation of precompiled contracts into a dedicated staking package.
Line range hint 15-20: LGTM: ABI instantiation updated correctly.
The test maintains its original validation logic while properly using the new staking package.
precompiles/crosschain/contract_test.go (3)
Line range hint 1-17: Package rename aligns with architectural changes.
The package rename from precompile_test to crosschain_test correctly reflects the consolidation of precompiled contracts and better represents the cross-chain functionality being tested.
Line range hint 19-26: Test suite structure follows best practices.
The CrosschainPrecompileTestSuite struct is well-designed with appropriate composition and field definitions.
Line range hint 59-61: LGTM!
The SetupSubTest method follows standard testing patterns by ensuring a fresh setup for each subtest.
precompiles/staking/contract.go (2)
1-1: LGTM: Package restructuring aligns with PR objectives.
The package rename and new imports reflect the consolidation of precompiles while maintaining good modularization practices.
Also applies to: 13-13
24-25: Verify consistent keeper type updates across the codebase.
The govKeeper type has been updated to use the new types package. Let's verify that all keeper types follow this pattern consistently.
✅ Verification successful
All keeper types are consistently using the new types package
Based on the search results, all keeper field declarations in the precompiles are consistently using the types package interfaces:
-
In
precompiles/staking/contract.go:govKeeper types.GovKeeperbankKeeper types.BankKeeperslashingKeeper types.SlashingKeeper
-
In
precompiles/crosschain/contract.go:govKeeper types.GovKeeperbankKeeper types.BankKeeper
-
In
precompiles/crosschain/keeper.go:bankKeeper types2.BankKeeper
The only exceptions are in test files and function parameters where concrete keeper implementations are needed, which is the expected pattern. The type change for govKeeper aligns with the consistent usage of the types package for all keeper interfaces across the precompiles module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining direct keeper type references that should use the types package
# Expected: All keeper types should use the new types package
# Search for keeper type declarations
rg -g '*.go' -A 2 'type.*Keeper\s+(?!types\.).*keeper\.' precompiles/
Length of output: 326
Script:
#!/bin/bash
# Let's try a different approach to find keeper type declarations and usages
# First, let's find all keeper-related type declarations
echo "=== Keeper Type Declarations ==="
rg -g '*.go' 'type.*Keeper.*struct' precompiles/
# Then look for keeper field declarations in structs
echo -e "\n=== Keeper Field Declarations ==="
rg -g '*.go' -B1 -A1 '\s+\w+[Kk]eeper\s+' precompiles/
Length of output: 5713
precompiles/staking/approve_shares_test.go (3)
Line range hint 1-15: LGTM! Package restructuring looks good.
The package rename and import updates align well with the PR objective of consolidating precompiled contracts into a single package.
Line range hint 17-24: LGTM! Test structure maintained with updated package reference.
The ABI initialization update maintains the same test coverage while aligning with the new package structure.
91-92: LGTM! Event handling remains consistent.
The ABI initialization update maintains proper event log verification while aligning with the new package structure.
Let's verify the event handling implementation:
✅ Verification successful
The verification results show consistent event handling patterns across the codebase. The approve_shares_test.go follows the same pattern as other test files in the staking package:
- Checking event topic using
Event.ID.String() - Verifying the contract address
- Unpacking the event using the ABI's
UnpackEventmethod
Event handling implementation is verified and consistent
The ABI initialization and event handling in approve_shares_test.go perfectly aligns with the established patterns across the staking package's test files, maintaining consistency in how events are verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the event handling is consistent across the codebase
# Test: Search for other event handling implementations to ensure consistency
# Look for similar event unpacking patterns
ast-grep --pattern 'approveSharesABI.UnpackEvent($$$)'
# Look for event ID usage patterns
rg 'Event.ID.String()' -A 2
Length of output: 2472
precompiles/staking/contract_test.go (3)
Line range hint 1-19: Package rename aligns with refactoring objectives.
The package rename from precompile_test to staking_test correctly reflects the consolidation of precompiled contracts into focused packages.
Line range hint 21-28: Test suite structure follows best practices.
The StakingPrecompileTestSuite is well-structured with appropriate embedding of base suites and necessary fields for testing.
Line range hint 75-80: Query client setup is well-implemented.
The distribution query client setup follows standard patterns and correctly utilizes baseapp test helpers.
tests/integration/staking_precompile_suite.go (2)
10-10: LGTM! Import path updated to reflect new package structure.
The import path change aligns with the PR objective of consolidating precompiles into a single package.
100-100: LGTM! Updated NewWithdrawABI call to use new package.
The change correctly updates the package reference while maintaining the same functionality.
Let's verify that all NewWithdrawABI references have been consistently updated across the codebase:
✅ Verification successful
All references to NewWithdrawABI are consistently using the new stakingprecompile package
The verification confirms that there is only one instance of NewWithdrawABI() in the codebase, and it's correctly using the new stakingprecompile package. No inconsistencies or old package references were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old precompile.NewWithdrawABI
rg "precompile\.NewWithdrawABI"
# Search for references to the new stakingprecompile.NewWithdrawABI
rg "stakingprecompile\.NewWithdrawABI"
Length of output: 268
precompiles/crosschain/crosschain.go (3)
Line range hint 1-15: LGTM! Package rename aligns with consolidation objective.
The package rename from precompile to crosschain and the import organization look good.
26-31: LGTM! Constructor properly implements new naming convention.
The constructor is well-structured and maintains consistency with the type renaming.
20-23: Verify handling of deprecated types in dependent code.
Since these types are marked as deprecated, we should ensure that dependent code is prepared for the transition.
Also applies to: 113-115
✅ Verification successful
The previous results show that LegacyCrosschainMethod and LegacyCrosschainABI are only used within their own file, but we need to:
- Check if there are any new/replacement types
- See where this package is imported to understand the impact
This will help us determine if these types are truly deprecated and if there are proper replacements in place.
Based on the search results, I can now provide a final response:
Legacy types are properly handled with no impact on dependent code
The LegacyCrosschainMethod and LegacyCrosschainABI types are only used within their own file (precompiles/crosschain/crosschain.go) and are not directly imported or used by other packages. The codebase shows a proper abstraction through interfaces (CrosschainKeeper, ICrosschain) that are used by dependent modules, making the implementation details of these legacy types internal to the precompile package.
Key findings:
- The types are only referenced within their own file
- Other packages interact through well-defined interfaces
- No direct dependencies on these legacy types in other modules
- The crosschain functionality is properly abstracted through interfaces in
precompiles/types/expected_keepers.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of deprecated types
echo "Searching for usage of deprecated types..."
rg "LegacyCrosschainMethod|LegacyCrosschainABI" --type go
# Search for potential new replacement types
echo "Searching for potential replacement types..."
rg "NewCrosschain(?!Method|ABI)" --type go
Length of output: 2154
Script:
#!/bin/bash
# Let's try a different approach to find potential new types and their usage
echo "Searching for any Crosschain related types and methods..."
rg "type.*Crosschain|func.*Crosschain" --type go -A 2
echo -e "\nSearching for imports of the crosschain package..."
rg "import.*crosschain" --type go -A 5
Length of output: 59818
precompiles/staking/withdraw_test.go (2)
Line range hint 1-16: LGTM: Package restructuring looks good
The package rename and import updates align well with the PR objective of consolidating precompiled contracts.
Line range hint 18-25: LGTM: ABI structure test is well-defined
The test properly verifies the structure of the withdraw ABI, ensuring it maintains the expected interface with correct number of inputs, outputs, and events.
precompiles/staking/undelegate_test.go (3)
Line range hint 1-18: LGTM: Package and import changes align with consolidation objectives.
The package renaming and import updates correctly reflect the restructuring of precompiled contracts into a consolidated package structure.
96-96: LGTM: Method usage updated consistently with package restructuring.
The change from precompile.NewUndelegateV2ABI() to staking.NewUndelegateV2ABI() maintains the same functionality while aligning with the new package structure.
1-1: Verify complete migration from precompile package.
Let's ensure all references to the old package structure have been updated consistently.
✅ Verification successful
Package structure is correctly migrated
The package structure has been properly migrated:
- All implementation files use
package staking - All test files consistently use
package staking_test - No references to the old precompile package structure remain
- Imports in the test files are using the correct dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old precompile package structure
# that might have been missed during the refactoring.
echo "Checking for any remaining references to old package structure..."
# Search for any remaining imports of the old precompile package
rg -l "\"github.com/functionx/fx-core/v8/x/staking/precompile\""
# Search for any remaining usage of precompile package in function calls
rg "precompile\.NewUndelegateV2ABI"
Length of output: 243
Script:
#!/bin/bash
# Let's try a broader search to understand the package structure changes
# Check for any files with "precompile" in their path or content
echo "Files with precompile in path:"
fd precompile
echo -e "\nChecking imports and package declarations:"
# Look for any precompile package declarations
rg -g '*.go' '^package precompile'
# Look for any imports containing precompile
rg -g '*.go' 'import.*precompile'
# Check the specific test file's package structure
echo -e "\nChecking the specific test file structure:"
cat precompiles/staking/undelegate_test.go | head -n 5
Length of output: 735
Script:
#!/bin/bash
# Let's check the imports and structure of related files to verify the migration
# Check the imports in the test file
echo "Checking imports in undelegate_test.go:"
rg -A 10 "^import \(" precompiles/staking/undelegate_test.go
# Check other files in the precompiles/staking directory
echo -e "\nListing files in precompiles/staking:"
ls -la precompiles/staking/
# Check package declarations in the staking precompile files
echo -e "\nChecking package declarations in staking precompile files:"
rg -g 'precompiles/staking/*.go' '^package'
Length of output: 3957
precompiles/staking/redelegate_test.go (2)
Line range hint 1-19: LGTM: Package and import changes align with refactoring objectives.
The package rename and import adjustments correctly reflect the consolidation of precompiles into focused packages.
102-102: LGTM: ABI initialization updated correctly.
The change to use staking.NewRedelegateV2ABI() aligns with the package restructuring.
precompiles/staking/delegate_test.go (2)
1-1: LGTM: Package restructuring looks good!
The package name change and import path updates align well with the PR objective of consolidating precompiled contracts.
Also applies to: 18-18
Line range hint 1-170: Verify package references are consistently updated.
The migration from precompile to staking package looks consistent throughout the file. Let's verify no references were missed:
✅ Verification successful
No inconsistencies found in package references
All occurrences of "precompile" in the file are either:
- Part of the new package import path (
precompiles/staking) - Part of the test suite name (
StakingPrecompileTestSuite) - Part of method names that appear to be test helper methods (
PrecompileStakingDelegateV2,PrecompileStakingWithdraw)
These are all valid and expected usages, not remnants of an old package structure that need to be updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old precompile package
rg -i 'precompile(?!s)' precompiles/staking/delegate_test.go
Length of output: 291
Script:
#!/bin/bash
# Search for any remaining references to the old precompile package using simpler pattern
rg -i "precompile" precompiles/staking/delegate_test.go
Length of output: 683
precompiles/crosschain/bridge_call_test.go (5)
Line range hint 1-15: LGTM: Package and import changes align with refactoring objectives.
The package renaming and import path updates correctly reflect the consolidation of precompiled contracts into the crosschain package.
17-22: LGTM: Test maintains correct validation of ABI structure.
The test properly validates the bridge call ABI structure with the updated package reference.
Line range hint 24-79: LGTM: Input parameter handling test is comprehensive.
The test thoroughly validates input parameter packing/unpacking with proper error handling and verification of all fields.
Line range hint 81-145: LGTM: Output and event handling tests are well-structured.
Both tests provide thorough validation:
- Output test properly verifies event nonce handling
- Event test comprehensively checks event structure, including indexed fields and signatures
Line range hint 1-189: Verify complete migration from precompile package.
The changes look good, but let's verify no references to the old package remain.
✅ Verification successful
Let me verify if there are any remaining references to the old package by searching across the codebase.
Migration from precompile package is complete
The only reference to "precompile" in the test file is the correct new import path precompiles/crosschain. The codebase search shows other precompile references are for different modules (staking) and are using their correct package paths. No references to the old package remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old precompile package in this file
# Expect: No matches as all references should be updated
rg -i "precompile" "precompiles/crosschain/bridge_call_test.go"
Length of output: 119
Script:
#!/bin/bash
# Search for any imports or usage of old precompile package
rg -i "\"github\.com/functionx/fx-core/v\d+/precompile\""
# Search for any direct usage of precompile package
rg "precompile\." --type go
Length of output: 260
precompiles/staking/keeper.go (2)
1-1: LGTM! Package restructuring aligns with modularization goals.
The package rename and new imports reflect proper separation of concerns, moving from a generic precompile package to a more specific staking package while maintaining clean dependencies through a common types package.
Also applies to: 17-17
26-31: Verify interface compatibility after type changes.
The keeper interface updates properly leverage the new types package, improving modularity. However, we should verify that all required methods are still accessible through the new interfaces.
✅ Verification successful
Let me analyze the interfaces to verify their compatibility.
Based on the analysis of the shell script results, I can verify the interface compatibility. Let me check the actual usage of these interfaces in the staking precompile.
Based on the comprehensive analysis of the interfaces and their usage, I can now generate the final response.
Interface compatibility verified successfully
All keeper methods used in the staking precompile implementation are properly defined in the new interfaces:
- BankKeeper: GetBalance and other bank operations
- DistrKeeper: All distribution operations including GetDelegatorWithdrawAddr, GetValidatorHistoricalRewards, etc.
- StakingKeeper: All staking operations including GetDelegation, SetDelegation, GetValidator, etc.
- SlashingKeeper: GetValidatorSigningInfo method
The interface updates maintain complete compatibility while improving modularity through the types package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all required keeper methods are defined in the new interfaces
# Check the interface definitions in the types package
echo "Checking BankKeeper interface..."
ast-grep --pattern 'type BankKeeper interface {
$$$
}'
echo "Checking DistrKeeper interface..."
ast-grep --pattern 'type DistrKeeper interface {
$$$
}'
echo "Checking StakingKeeper interface..."
ast-grep --pattern 'type StakingKeeper interface {
$$$
}'
echo "Checking SlashingKeeper interface..."
ast-grep --pattern 'type SlashingKeeper interface {
$$$
}'
Length of output: 10763
Script:
#!/bin/bash
# Let's check how these interfaces are used in the staking precompile
echo "Checking staking precompile usage..."
rg -A 2 "BankKeeper|DistrKeeper|StakingKeeper|SlashingKeeper" precompiles/staking/
# Check if there are any method calls that might not be available in the new interfaces
echo "Checking method calls in staking precompile..."
ast-grep --pattern 'keeper.$_($_)' precompiles/staking/
Length of output: 33541
Script:
#!/bin/bash
# Check the actual method usage in the staking precompile implementation
echo "Checking method usage in staking precompile..."
rg -A 1 "k\.(bank|distr|staking|slashing)Keeper\." precompiles/staking/ --type go
# Check if there are any methods used that are not defined in the interfaces
echo "Checking interface definitions..."
cat precompiles/types/expected_keepers.go
Length of output: 10161
precompiles/staking/transfer_shares_test.go (5)
1-1: LGTM: Package rename aligns with module restructuring.
The package rename from precompile_test to staking_test is consistent with the PR's objective of consolidating precompiled contracts.
20-20: LGTM: Import statement updated correctly.
The import path has been properly updated to reflect the new package structure.
26-26: LGTM: ABI initialization updated consistently.
The ABI initialization calls have been properly updated to use the new staking package while maintaining the same test functionality.
Also applies to: 241-241, 281-281
510-510: LGTM: Event unpacking updated correctly.
The event unpacking code has been properly updated to use the new staking package.
Line range hint 1-1000: Comprehensive test coverage maintained.
The test suite provides thorough coverage of staking functionality including:
- Transfer and delegation scenarios
- Edge cases and error conditions
- Event emission verification
- Balance and state change validation
precompiles/crosschain/bridge_coin_amount.go (3)
Line range hint 17-19: Constructor initializes all fields properly
The NewBridgeCoinAmountMethod function correctly initializes all necessary fields, ensuring the BridgeCoinAmountMethod is ready for use.
Line range hint 5-5: Verify that crosschainABI is properly initialized
Ensure that crosschainABI is correctly initialized before accessing Methods["bridgeCoinAmount"] in NewBridgeCoinAmountABI. An uninitialized or incorrect ABI could lead to runtime errors.
Run the following script to check that bridgeCoinAmount method exists in the ABI definitions:
#!/bin/bash
# Description: Verify the presence of 'bridgeCoinAmount' method in crosschainABI.
# Test: Search for 'bridgeCoinAmount' in ABI files.
# Expect: Method definition should be found.
rg '"name":\s*"bridgeCoinAmount"' --glob '**/*.abi' --json | jq -r '.data.path.text'1-1: 🛠️ Refactor suggestion
Package renaming: Ensure all references are updated
The package name has been changed to crosschain. Please verify that all import statements and references to this package throughout the codebase have been updated accordingly to prevent import errors.
Run the following script to find any remaining references to the old package name:
precompiles/staking/delegation_rewards.go (1)
Line range hint 1-62: Code structure and implementation look good
The restructuring and modularization of the DelegationRewardsMethod and associated ABI handling are well-executed. The code is clean, maintains consistency with the new package structure, and adheres to best practices.
precompiles/crosschain/execute_claim.go (1)
1-1: Ensure Package Name Consistency
The package name has been changed to crosschain. Please verify that all references to this package are updated throughout the codebase to prevent import errors and maintain consistency.
You can run the following script to find any remaining references to the old package name:
✅ Verification successful
Package name change is consistent with codebase structure
The package name crosschain is already consistently used throughout the precompiles/crosschain directory, and all imports are correctly referencing it as github.com/functionx/fx-core/v8/precompiles/crosschain. The change in execute_claim.go maintains this consistency with the existing package structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for imports of the old 'precompiles' package.
# Test: Find all files importing 'precompiles' instead of 'crosschain'.
rg 'import.*precompiles/' -t go
Length of output: 33
Script:
#!/bin/bash
# Let's try a broader search to understand the package structure and imports
# First, let's find all Go files in the precompiles directory
fd -e go . precompiles/
# Then, let's look for any imports or references to both 'precompiles' and 'crosschain'
rg -g '*.go' -i "precompiles/crosschain|\"crosschain\""
# Also check for package declarations to understand the package structure
rg -g '*.go' "^package crosschain"
Length of output: 12769
precompiles/staking/approve_shares.go (2)
1-1: Package name change to staking is appropriate
Renaming the package to staking aligns the package name with its functionality, improving code clarity and organization.
Line range hint 74-81: Error handling when creating the filterer
In the UnpackEvent method, you create a new filterer without specifying the backend. Ensure that the backend is properly provided; otherwise, this might lead to unexpected errors during event parsing.
Run the following script to check for proper initialization of the filterer:
This will list all occurrences where NewIStakingFilterer is called. Ensure that the second argument (backend) is not nil to prevent runtime issues.
precompiles/staking/delegate.go (12)
Line range hint 19-28: LGTM!
The DelegateV2Method struct and its constructor function are well-defined and follow the expected pattern.
Line range hint 30-46: LGTM!
The IsReadonly, GetMethodId, and RequiredGas methods are implemented correctly.
Line range hint 73-77: LGTM!
The DelegateV2ABI struct is well-defined and follows the expected pattern.
Line range hint 79-85: LGTM!
The NewDelegateV2ABI function is implemented correctly and initializes the DelegateV2ABI struct with the appropriate ABI method and event.
Line range hint 87-94: LGTM!
The NewDelegateEvent method is implemented correctly and follows the expected pattern for creating event data and topics.
Line range hint 96-103: LGTM!
The PackInput method is implemented correctly and follows the expected pattern for packing input arguments.
Line range hint 105-109: LGTM!
The UnpackInput method is implemented correctly and follows the expected pattern for unpacking input arguments.
Line range hint 111-114: LGTM!
The PackOutput method is implemented correctly and follows the expected pattern for packing output results.
Line range hint 116-123: LGTM!
The UnpackOutput method is implemented correctly and follows the expected pattern for unpacking output results.
Line range hint 125-135: Verify error handling in the UnpackEvent method.
The UnpackEvent method handles the case of an empty log by returning an error. However, ensure that this error is properly propagated and handled at the higher levels of the codebase.
Run the following script to verify error handling:
#!/bin/bash
# Description: Verify error handling in the functions that call the `UnpackEvent` method.
# Test: Search for the `UnpackEvent` method usage. Expect: Proper error handling in the calling functions.
rg -A 10 $'\\(m DelegateV2ABI\\) UnpackEvent\\('1-1: Verify the package renaming across the codebase.
The package has been renamed from precompile to staking. Ensure that all references to this package have been updated accordingly throughout the codebase.
Run the following script to verify the package usage:
Line range hint 48-71: Verify error handling in the Run method.
The Run method handles errors appropriately by returning them to the caller. However, ensure that the errors are properly propagated and handled at the higher levels of the codebase.
Run the following script to verify error handling:
✅ Verification successful
Let me gather more information about the error handling by searching for the actual Run method and its usage.
Let me search for the actual implementation of the Run method to verify its error handling.
Let me check how the error is handled at the higher level where this Run method is called.
Based on the code analysis, I can now provide the final response:
Error handling is properly implemented throughout the call chain
The error handling in the Run method is properly propagated and handled:
- The
Runmethod itself returns errors directly to its caller - The parent contract implementation wraps the error using
contract.PackRetErrV2(err)before returning it - Multiple error checks are in place including input validation, precompile checks, and state operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error handling in the functions that call the `Run` method.
# Test: Search for the `Run` method usage. Expect: Proper error handling in the calling functions.
rg -A 10 $'\\(m \\*DelegateV2Method\\) Run\\('
Length of output: 671
Script:
#!/bin/bash
# Search for the Run method definition and its usage
rg -A 10 "func \(m \*DelegateV2Method\) Run\("
# Search for any calls to this Run method
rg -B 5 -A 5 "\.Run\("
Length of output: 62482
Script:
#!/bin/bash
# Search for the Run method implementation
ast-grep --pattern 'func (m *DelegateV2Method) Run($_, $_) {
$$$
}'
# Also search for where this Run method is called
rg -B 5 -A 5 "DelegateV2Method.*Run\("
Length of output: 701
Script:
#!/bin/bash
# Search for the contract implementation that calls Run
rg -B 10 -A 10 "method\.Run\(evm, vmContract\)" precompiles/staking/
Length of output: 1254
precompiles/staking/undelegate.go (1)
Line range hint 81-88: Verify correct initialization of IStakingFilterer in UnpackEvent.
In UnpackEvent, you're initializing IStakingFilterer with common.Address{} and nil. Ensure that passing nil as the backend client does not lead to unexpected behavior or nil pointer dereferences when calling ParseUndelegateV2.
Run the following script to verify the initialization patterns of IStakingFilterer in the codebase:
✅ Verification successful
Let me check the event parsing implementation to understand if passing nil backend is safe.
Let me try a different approach to find the event parsing implementation.
Initialization of IStakingFilterer with nil backend is safe for event parsing
The implementation shows that ParseUndelegateV2 only uses the contract's UnpackLog method which doesn't require a backend client. The filterer is only used for parsing event data from the log, and the nil backend is acceptable since no RPC calls are made during the unpacking process. This is a common pattern across all staking event handlers in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of `NewIStakingFilterer` to verify proper initialization.
# Expected: Instances showing initialization with valid backend clients.
rg -A 3 'NewIStakingFilterer\('
Length of output: 1852
Script:
#!/bin/bash
# Check the ParseUndelegateV2 implementation to verify if nil backend is handled safely
ast-grep --pattern 'func (IStakingFilterer) ParseUndelegateV2($$$) {
$$$
}'
Length of output: 79
Script:
#!/bin/bash
# Search for ParseUndelegateV2 implementation in the contract bindings
rg -A 10 "func.*ParseUndelegateV2"
# Also check how the event parsing is used in UnpackEvent
rg -A 10 "func.*UnpackEvent.*Undelegate"
Length of output: 2395
precompiles/types/expected_keepers.go (5)
29-39: Introduction of CrosschainKeeper Interface
The new CrosschainKeeper interface includes methods for cross-chain operations. Confirm that:
- All methods are properly implemented in the corresponding keeper.
- Error handling and return types are consistent with their intended usage.
- Methods like
ExecuteClaim,BridgeCoinSupply, andCrosschainBaseCoinare thoroughly tested.
Please ensure that unit tests cover the new methods added to the CrosschainKeeper interface.
Consider adding comments or documentation for each method in the interface to clarify their purpose and usage.
41-43: Addition of GovKeeper Interface
The GovKeeper interface with the method CheckDisabledPrecompiles has been added. Ensure that:
- The method is implemented in the governance keeper.
- The logic within
CheckDisabledPrecompilescorrectly handles the scenarios where precompiles may be disabled. - There are appropriate checks to prevent unauthorized access to precompiled contracts.
Check that all invocations of precompiled contracts include a call to CheckDisabledPrecompiles where necessary.
Line range hint 88-90: Ensure Correct Implementation of SlashingKeeper Method
The method GetValidatorSigningInfo retrieves the signing information for a validator. Confirm that:
- The implementation correctly accesses and returns the validator's signing information.
- Proper error handling is in place if the validator does not exist.
- This method is used securely to avoid leaking sensitive validator information.
Review the usage of GetValidatorSigningInfo to ensure compliance with security best practices.
21-24: Review of Updated BankKeeper Interface
The BankKeeper interface has new methods added and one method renamed:
- Added:
MintCoins(ctx context.Context, moduleName string, amounts sdk.Coins) errorSendCoinsFromAccountToModule(ctx context.Context, senderAddr sdk.AccAddress, recipientModule string, amt sdk.Coins) error
- Renamed:
SendCoinsmethod parameters changed to(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins) error
Ensure that all implementations of the BankKeeper interface have these methods defined, and all usages of SendCoins are updated to match the new signature.
Run the following script to find usages of the SendCoins method:
✅ Verification successful
Let me verify the implementations of the other new methods to ensure completeness.
All BankKeeper Interface Changes Verified
Based on the search results, all the new methods (MintCoins, SendCoinsFromAccountToModule, SendCoinsFromModuleToAccount) and the updated SendCoins method are properly implemented and used consistently across the codebase:
SendCoins: All usages match the new signature with(ctx context.Context, fromAddr, toAddr sdk.AccAddress, amt sdk.Coins)MintCoins: Implemented and used in various modules (crosschain, erc20, migrate)SendCoinsFromAccountToModule: Properly used in gov, erc20, and crosschain modulesSendCoinsFromModuleToAccount: Correctly implemented and used in multiple modules
No inconsistencies or missing implementations were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all usages of 'SendCoins' match the new signature.
# Test: Search for 'SendCoins' method calls.
# Expect: Parameters should match the new signature.
rg -A 1 'SendCoins\('
Length of output: 2824
Script:
#!/bin/bash
# Search for MintCoins implementations and usages
rg -A 1 'MintCoins\('
# Search for SendCoinsFromAccountToModule implementations and usages
rg -A 1 'SendCoinsFromAccountToModule\('
# Search for SendCoinsFromModuleToAccount implementations and usages
rg -A 1 'SendCoinsFromModuleToAccount\('
Length of output: 13047
1-2: Package Naming Consistency
Changing the package name from precompile to types affects the organization and import paths of this package. Ensure that all references to this package in other parts of the codebase are updated accordingly to prevent import errors.
You can run the following script to find all references to the old package name:
precompiles/staking/transfer_shares.go (5)
Line range hint 18-28: LGTM!
The TransferSharesMethod struct and its constructor function are well-defined and follow the expected structure. The inclusion of the Keeper and TransferSharesABI fields allows for the necessary interactions with the state and ABI encoding/decoding.
Line range hint 72-89: LGTM!
The TransferSharesABI struct and its associated methods (PackInput, UnpackInput) are correctly implemented. They handle the encoding and decoding of input arguments based on the defined ABI method.
Line range hint 155-175: LGTM!
The TransferFromSharesABI struct and its methods (PackInput, UnpackInput) are properly implemented, mirroring the structure and functionality of the TransferSharesABI. The ABI encoding and decoding logic is correct.
Line range hint 177-213: The transferShareABI struct and its methods are implemented correctly.
The transferShareABI struct encapsulates the ABI method and event for share transfers. The PackOutput, UnpackOutput, UnpackEvent, and NewTransferShareEvent methods handle the encoding and decoding of output values and events based on the defined ABI.
The implementation follows the expected structure and utilizes the appropriate ABI encoding/decoding functions from the github.com/ethereum/go-ethereum/accounts/abi package.
1-1: Verify the package renaming across the codebase.
The package has been renamed from precompile to staking. Ensure that all references to this package have been updated accordingly throughout the codebase to maintain consistency and avoid any build errors.
Run the following script to verify the package usage:
✅ Verification successful
Package renaming has been properly implemented across the codebase
The verification shows that:
- The package has been correctly renamed to
stakingin all relevant files underprecompiles/staking/ - All imports are using the new path
github.com/functionx/fx-core/v8/precompiles/staking - No references to the old package structure
precompiles/precompilewere found - The package is properly aliased as
stakingprecompilewhere needed in integration tests and app keepers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify all imports of the `precompile` package have been updated to `staking`.
# Test: Search for the old package name. Expect: No occurrences of `precompile`.
rg $'"github.com/functionx/fx-core/v8/precompiles/precompile"'
Length of output: 63
Script:
#!/bin/bash
# Let's verify the package references more comprehensively
# Check for any imports containing "precompile" in go files
rg -t go "import.*precompile" -A 3 -B 3
# Check for any package declarations containing "precompile"
rg -t go "^package precompile"
# Check for any references to precompile package in go files
rg -t go "precompile\."
# Check for staking package usage
rg -t go "^package staking"
# Check for imports of staking package
rg -t go "import.*staking"
Length of output: 1843
Script:
#!/bin/bash
# Let's check for any references to the old package structure and new imports
# Check for any file imports referencing the new staking package
rg -t go '"github.com/functionx/fx-core/v8/precompiles/staking"' -A 1 -B 1
# Check for any references to the old package structure
rg -t go '"github.com/functionx/fx-core/v8/precompiles/precompile/staking"' -A 1 -B 1
# Look for any potential package aliases or references
rg -t go "stakingprecompile\." -A 2 -B 2
Length of output: 4725
app/keepers/keepers.go (1)
Line range hint 291-293: Verify compatibility with Ethermint version
Ensure that the usage of evmkeeper.NewKeeper and the custom precompiled contracts are compatible with the Ethermint version specified in your dependencies. Changes in Ethermint's API might affect the initialization of the EVM keeper.
Run the following script to check the Ethermint dependency version:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Tests