test: refactor staking precompile test suite #792
Conversation
WalkthroughThe pull request introduces significant changes across multiple files, primarily focusing on enhancing error handling and restructuring argument types within the contract and staking functionalities. A new function, Changes
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: 26
🧹 Outside diff range and nitpick comments (31)
x/staking/precompile/slashing_info.go (1)
Line range hint 82-85: Consider enhancing error messages for better debugging.
While the error handling is present, consider adding more context to error messages to aid in debugging.
func (m SlashingABI) UnpackInput(data []byte) (*fxstakingtypes.SlashingInfoArgs, error) {
args := new(fxstakingtypes.SlashingInfoArgs)
- err := types.ParseMethodArgs(m.Method, args, data[4:])
+ if len(data) <= 4 {
+ return nil, fmt.Errorf("invalid input data length: got %d, want > 4", len(data))
+ }
+ if err := types.ParseMethodArgs(m.Method, args, data[4:]); err != nil {
+ return nil, fmt.Errorf("failed to parse slashing info args: %w", err)
+ }
return args, err
}x/staking/precompile/validator_list.go (1)
Line range hint 39-58: Consider adding documentation for sorting behavior.
While the implementation is solid, it would be helpful to add documentation explaining:
- The criteria used for power-based sorting
- The definition of missed blocks and how they affect sorting
- Any performance implications of different sorting methods
func (m *ValidatorListMethod) Run(evm *vm.EVM, contract *vm.Contract) ([]byte, error) {
+ // ValidatorList returns a sorted list of validators based on the specified criteria:
+ // - ValidatorSortByPower: Orders validators by their voting power in descending order
+ // - ValidatorSortByMissed: Orders validators by their missed block count in ascending order
args, err := m.UnpackInput(contract.Input)x/staking/precompile/slashing_info_test.go (1)
89-95: Consider fixing indentation for consistency.
The test assertion block has inconsistent indentation which affects readability.
if tc.result {
- validator, err := suite.App.StakingKeeper.GetValidator(suite.Ctx, operator)
- suite.Require().NoError(err)
- suite.Require().Equal(validator.Jailed, jailed)
- consAddr, err := validator.GetConsAddr()
- suite.Require().NoError(err)
- signingInfo, err := suite.App.SlashingKeeper.GetValidatorSigningInfo(suite.Ctx, consAddr)
- suite.Require().NoError(err)
- suite.Require().Equal(signingInfo.MissedBlocksCounter, missed.Int64())
+ validator, err := suite.App.StakingKeeper.GetValidator(suite.Ctx, operator)
+ suite.Require().NoError(err)
+ suite.Require().Equal(validator.Jailed, jailed)
+ consAddr, err := validator.GetConsAddr()
+ suite.Require().NoError(err)
+ signingInfo, err := suite.App.SlashingKeeper.GetValidatorSigningInfo(suite.Ctx, consAddr)
+ suite.Require().NoError(err)
+ suite.Require().Equal(signingInfo.MissedBlocksCounter, missed.Int64())
}testutil/helpers/staking_suite.go (2)
78-80: Add validation and documentation for allowance setting.
The method lacks input validation and documentation explaining its purpose in the test suite.
Consider these improvements:
+// SetAllowance sets the number of shares that a spender is allowed to delegate on behalf of an owner for a specific validator.
func (s *BaseSuite) SetAllowance(valAddr sdk.ValAddress, owner, spender sdk.AccAddress, shares *big.Int) {
+ s.Require().NotNil(shares, "shares cannot be nil")
+ s.Require().True(shares.Sign() >= 0, "shares cannot be negative")
s.App.StakingKeeper.SetAllowance(s.Ctx, valAddr, owner, spender, shares)
}1-80: Well-structured test suite with comprehensive staking utilities.
The test suite provides a solid foundation for staking-related tests with good error handling and validation. The methods are logically organized and follow testing best practices.
Consider adding the following improvements to enhance maintainability:
- Add package-level documentation explaining the purpose and usage of the suite
- Consider grouping related methods using comments (e.g., // Validator Operations, // Delegation Operations)
- Consider adding examples in documentation showing common test scenarios
x/staking/precompile/approve_shares.go (1)
94-97: Consider enhancing error context.
While the implementation is correct, consider wrapping the error from ParseMethodArgs with additional context to aid in debugging.
Here's a suggested improvement:
func (m ApproveSharesABI) UnpackInput(data []byte) (*fxcontract.ApproveSharesArgs, error) {
args := new(fxcontract.ApproveSharesArgs)
- err := types.ParseMethodArgs(m.Method, args, data[4:])
- return args, err
+ if err := types.ParseMethodArgs(m.Method, args, data[4:]); err != nil {
+ return nil, fmt.Errorf("failed to parse approve shares args: %w", err)
+ }
+ return args, nil
}x/staking/precompile/withdraw.go (1)
104-107: Consider enhancing error handling with context.
The implementation is correct, but consider wrapping the error with additional context to aid in debugging.
func (m WithdrawABI) UnpackInput(data []byte) (*fxcontract.WithdrawArgs, error) {
args := new(fxcontract.WithdrawArgs)
- err := types.ParseMethodArgs(m.Method, args, data[4:])
- return args, err
+ if err := types.ParseMethodArgs(m.Method, args, data[4:]); err != nil {
+ return nil, fmt.Errorf("failed to parse withdraw args: %w", err)
+ }
+ return args, nil
}x/staking/precompile/validator_list_test.go (3)
11-11: Consider using a more idiomatic import alias.
The alias precompile2 suggests a temporary or transitional state. Consider using a more descriptive alias like stakingprecompile or simply precompile if there are no conflicts.
Also applies to: 13-13, 17-17
86-88: Consider making contract selection more explicit.
The contract selection based on test name prefix (strings.HasPrefix(tc.name, "contract")) is somewhat implicit and could be confusing. Consider making this more explicit by:
- Adding it as a field in the test case struct, or
- Using a more descriptive naming convention
Example improvement:
type testCase struct {
name string
+ useTestContract bool
malleate func() (contract.ValidatorListArgs, error)
result bool
}
// In test execution:
-if strings.HasPrefix(tc.name, "contract") {
+if tc.useTestContract {
suite.WithContract(suite.stakingTestAddr)
}Line range hint 102-118: Consider optimizing missed blocks validation.
The current implementation creates a new slice and performs sorting for missed blocks validation. This could be optimized by:
- Pre-allocating the slice with known capacity (already done correctly)
- Using a more efficient comparison method
Consider this optimization:
-valList := make([]precompile2.Validator, 0, len(valsByPower))
+type validatorMissed struct {
+ valAddr string
+ missed uint64
+}
+valList := make([]validatorMissed, 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, precompile2.Validator{
- ValAddr: validator.OperatorAddress,
- MissedBlocks: info.MissedBlocksCounter,
+ valList = append(valList, validatorMissed{
+ valAddr: validator.OperatorAddress,
+ missed: info.MissedBlocksCounter,
})
}
sort.Slice(valList, func(i, j int) bool {
- return valList[i].MissedBlocks > valList[j].MissedBlocks
+ return valList[i].missed > valList[j].missed
})x/staking/precompile/allowance_shares_test.go (2)
Line range hint 31-97: Consider adding a test case for maximum allowance.
The test cases comprehensively cover basic scenarios, zero allowance, and invalid inputs. However, it might be valuable to add a test case that verifies behavior with maximum allowable values to ensure proper handling of large numbers.
Example test case to add:
{
name: "ok - maximum allowance",
malleate: func(val sdk.ValAddress, owner, spender *helpers.Signer) (contract.AllowanceSharesArgs, error) {
return contract.AllowanceSharesArgs{
Validator: val.String(),
Owner: owner.Address(),
Spender: spender.Address(),
}, nil
},
result: true,
},103-117: LGTM! Test execution logic is well-structured.
The test execution flow is clear and properly handles both regular and contract-based test cases. The assertions correctly verify the allowance shares.
Consider adding more descriptive error messages to the assertions to improve debugging experience:
-suite.Require().Equal(allowanceAmt.BigInt(), shares)
+suite.Require().Equal(allowanceAmt.BigInt(), shares, "allowance shares should match the set allowance amount")x/staking/precompile/delegation_test.go (1)
123-150: Consider enhancing error case verification.
While the test execution logic is sound, consider adding explicit error message verification for failure cases. This would ensure that not only the failure occurs but that it fails for the expected reason.
Example improvement:
delValue, _ := suite.WithError(expectErr).Delegation(suite.Ctx, args)
if tc.result {
suite.Require().False(res.Failed(), res.VmError)
suite.Require().Equal(delegation.GetShares().TruncateInt().String(), delValue.String())
+} else {
+ suite.Require().EqualError(expectErr, tc.error([]string{}))
}x/staking/precompile/approve_shares_test.go (2)
105-112: LGTM: Improved test implementation with better type safety.
The changes enhance test clarity by:
- Using direct validator address handling
- Adding pre-condition verification
- Utilizing type-safe contract arguments
Consider adding more descriptive error messages in the test cases, for example:
-}, fmt.Errorf("invalid validator address: %s", valStr)
+}, fmt.Errorf("expected valid validator address, got invalid address: %s", valStr)114-118: LGTM: Clear separation of contract-specific test cases.
The code effectively handles both regular and contract-specific test scenarios.
Consider extracting the contract-specific logic into a helper method for better reusability:
func (suite *PrecompileTestSuite) setupContractContext(testName string) common.Address {
delAddr := suite.signer.Address()
suite.WithContract(suite.stakingAddr)
if strings.HasPrefix(testName, "contract") {
suite.WithContract(suite.stakingTestAddr)
delAddr = suite.stakingTestAddr
}
return delAddr
}contract/erc20_token.go (1)
Line range hint 87-116: Well-structured refactor of error handling
The changes demonstrate a well-thought-out refactor that:
- Makes ABI dependencies explicit in error handling
- Standardizes the approach across all token operations
- Removes redundant keeper-level method in favor of a shared implementation
This improves maintainability and reduces the chance of inconsistent error handling.
x/staking/precompile/withdraw_test.go (2)
114-118: Consider enhancing error handling for delegation failure.
While the delegation check verifies that the operation hasn't failed, consider adding specific error message validation to ensure the exact reason for any potential failures.
-suite.Require().False(res.Failed(), res.VmError)
+suite.Require().False(res.Failed(), fmt.Sprintf("Delegation failed: %s", res.VmError))
+// Consider adding specific error message validation for known failure casesLine range hint 140-182: Consider refactoring event validation for better maintainability.
The event validation logic could be extracted into helper functions to improve readability and maintainability. This would also make it easier to reuse the validation logic across different test cases.
Consider restructuring the event validation like this:
func (suite *PrecompileTestSuite) validateWithdrawEvent(res *evm.MsgEthereumTxResponse, abi *precompile.WithdrawABI, expectedAddr common.Address, expectedValidator sdk.ValAddress, expectedReward sdkmath.Int) {
existLog := false
for _, log := range res.Logs {
if log.Topics[0] == abi.Event.ID.String() {
suite.validateWithdrawLog(log, abi, expectedAddr, expectedValidator, expectedReward)
existLog = true
}
}
suite.Require().True(existLog)
}
func (suite *PrecompileTestSuite) validateWithdrawLog(log *ethtypes.Log, abi *precompile.WithdrawABI, expectedAddr common.Address, expectedValidator sdk.ValAddress, expectedReward sdkmath.Int) {
suite.Require().Equal(log.Address, suite.stakingAddr.String())
event, err := abi.UnpackEvent(log.ToEthereum())
suite.Require().NoError(err)
suite.Require().Equal(event.Sender, expectedAddr)
suite.Require().Equal(event.Validator, expectedValidator.String())
suite.Require().Equal(event.Reward.String(), expectedReward.BigInt().String())
}x/staking/precompile/redelegate_test.go (2)
Line range hint 24-73: LGTM with a suggestion for additional test coverage.
The test cases are well-structured and properly test both success and failure scenarios. The change to use RedelegateV2Args instead of interface{} improves type safety.
Consider adding test cases for:
- Zero amount redelegation
- Redelegation with amount greater than delegated amount
- Multiple redelegations within unbonding period
132-174: LGTM with a suggestion for improved error messages.
The validation methods thoroughly check both EVM logs and Cosmos SDK events, ensuring consistency across both layers.
Consider enhancing the assertion messages to be more descriptive:
-suite.Require().True(existLog)
+suite.Require().True(existLog, "expected to find redelegation event log")
-suite.Require().True(existEvent)
+suite.Require().True(existEvent, "expected to find redelegation SDK event")tests/staking_suite.go (1)
Line range hint 248-259: Consider improving error handling for missing logs.
While the implementation is correct, consider adding a warning log when no matching log entry is found, as this might indicate an unexpected state.
Here's a suggested improvement:
func (suite *StakingSuite) LogReward(logs []*ethtypes.Log, valAddr string, addr common.Address) *big.Int {
method := precompile.NewWithdrawMethod(nil)
+ found := false
for _, log := range logs {
if log.Address == suite.stakingContract &&
log.Topics[0] == method.Event.ID &&
log.Topics[1] == addr.Hash() {
unpack, err := method.Event.Inputs.NonIndexed().Unpack(log.Data)
suite.Require().NoError(err)
suite.Require().Equal(unpack[0].(string), valAddr)
+ found = true
return unpack[1].(*big.Int)
}
}
+ if !found {
+ suite.T().Logf("Warning: No matching log entry found for address %s and validator %s", addr.String(), valAddr)
+ }
return big.NewInt(0)
}contract/contract.go (1)
154-163: LGTM with a minor suggestion for error messages.
The function is well-structured with proper error handling and clear responsibility. Consider making the error messages more specific by including the actual error value in the logic error case.
if !ret.Value {
- return res, sdkerrors.ErrLogic.Wrapf("failed to execute %s", method)
+ return res, sdkerrors.ErrLogic.Wrapf("failed to execute %s: returned false", method)
}testutil/helpers/staking_precompile_suite.go (1)
63-66: Rename variable rewards to validators for clarity
In the ValidatorList method, the variable rewards is used to store the list of validators. Renaming it to validators will enhance readability and accurately represent the data being returned.
x/staking/precompile/transfer_shares.go (2)
99-102: Consider defining a constant for method ID length
In the UnpackInput method, data[4:] is used to skip the method ID in the input data. To enhance readability and maintainability, consider defining a constant for the method ID length.
Apply the following diff to implement this suggestion:
+const MethodIDLength = 4
func (m TransferSharesABI) UnpackInput(data []byte) (*fxcontract.TransferSharesArgs, error) {
args := new(fxcontract.TransferSharesArgs)
- err := types.ParseMethodArgs(m.Method, args, data[4:])
+ err := types.ParseMethodArgs(m.Method, args, data[MethodIDLength:])
return args, err
}192-195: Consider defining a constant for method ID length
Similar to the previous suggestion, in the UnpackInput method, using a constant for the method ID length instead of the magic number 4 can improve code clarity.
Apply the following diff:
+const MethodIDLength = 4
func (m TransferFromSharesABI) UnpackInput(data []byte) (*fxcontract.TransferFromSharesArgs, error) {
args := new(fxcontract.TransferFromSharesArgs)
- err := types.ParseMethodArgs(m.Method, args, data[4:])
+ err := types.ParseMethodArgs(m.Method, args, data[MethodIDLength:])
return args, err
}contract/staking_precompile.go (1)
123-133: Suggestion to define a named struct for Withdraw return value
In the Withdraw method, an anonymous struct is used to unpack the reward. For consistency and clarity, consider defining a named struct for the return value, as done in other methods.
Apply this diff to define a named return struct:
+ type WithdrawRet struct {
+ Reward *big.Int
+ }
- ret := struct{ Reward *big.Int }{}
+ ret := new(WithdrawRet)
if err = k.abi.UnpackIntoInterface(&ret, "withdraw", res.Ret); err != nil {This enhances readability and maintains consistency across the codebase.
contract/staking.go (3)
129-137: Consider validating that ValidatorSrc and ValidatorDst are not the same in RedelegateV2Args.
To prevent unnecessary redelegation operations, you might want to add a validation check to ensure that the source and destination validators are not identical.
Suggested code change:
func (args *RedelegateV2Args) Validate() error {
if _, err := sdk.ValAddressFromBech32(args.ValidatorSrc); err != nil {
return fmt.Errorf("invalid validator src address: %s", args.ValidatorSrc)
}
if _, err := sdk.ValAddressFromBech32(args.ValidatorDst); err != nil {
return fmt.Errorf("invalid validator dst address: %s", args.ValidatorDst)
}
+ if args.ValidatorSrc == args.ValidatorDst {
+ return errors.New("source and destination validators must be different")
+ }
if args.Amount == nil || args.Amount.Sign() <= 0 {
return errors.New("invalid amount")
}
return nil
}177-181: Add comments for exported fields in TransferSharesRet.
To enhance code readability and maintainability, consider adding comments for the exported fields Token and Reward.
Suggested code change:
type TransferSharesRet struct {
// Token represents the amount of tokens received from the share transfer
Token *big.Int
// Reward represents the reward amount associated with the transfer
Reward *big.Int
}206-208: Add comments for exported fields in TransferFromSharesRet.
To enhance code readability and maintainability, consider adding comments for the exported fields Token and Reward.
Suggested code change:
type TransferFromSharesRet struct {
// Token represents the amount of tokens received from the share transfer
Token *big.Int
// Reward represents the reward amount associated with the transfer
Reward *big.Int
}x/staking/precompile/delegate_test.go (1)
Line range hint 69-245: Consider adding comments to new helper functions for clarity.
The addition of CheckDelegateLogs and CheckDelegateEvents improves code organization. Adding comments to these functions can enhance readability and maintainability by explaining their purpose and usage.
Include function comments:
// CheckDelegateLogs verifies the logs for delegate events.
func (suite *PrecompileTestSuite) CheckDelegateLogs(logs []*evmtypes.Log, delAddr common.Address, valAddr string, amount, shares *big.Int) {
// function body...
}
// CheckDelegateEvents checks the events emitted during delegation.
func (suite *PrecompileTestSuite) CheckDelegateEvents(ctx sdk.Context, valAddr sdk.ValAddress, delAmount sdkmath.Int) {
// function body...
}x/staking/precompile/transfer_shares_test.go (1)
653-655: Ensure consistent usage of signers
At lines 653-655, new signers are created. Ensure that the use of suite.NewSigner() instead of suite.RandSigner() aligns with the intended test behavior and does not compromise test randomness where necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (34)
- contract/contract.go (2 hunks)
- contract/erc20_token.go (2 hunks)
- contract/staking.go (6 hunks)
- contract/staking_precompile.go (1 hunks)
- tests/staking_suite.go (8 hunks)
- testutil/helpers/base_suite.go (3 hunks)
- testutil/helpers/contract_base_suite.go (1 hunks)
- testutil/helpers/staking_precompile_suite.go (1 hunks)
- testutil/helpers/staking_suite.go (1 hunks)
- x/staking/precompile/allowance_shares.go (1 hunks)
- x/staking/precompile/allowance_shares_test.go (5 hunks)
- x/staking/precompile/approve_shares.go (1 hunks)
- x/staking/precompile/approve_shares_test.go (2 hunks)
- x/staking/precompile/contract_test.go (3 hunks)
- x/staking/precompile/delegate.go (1 hunks)
- x/staking/precompile/delegate_test.go (10 hunks)
- x/staking/precompile/delegation.go (1 hunks)
- x/staking/precompile/delegation_rewards.go (1 hunks)
- x/staking/precompile/delegation_rewards_test.go (8 hunks)
- x/staking/precompile/delegation_test.go (10 hunks)
- x/staking/precompile/redelegate.go (1 hunks)
- x/staking/precompile/redelegate_test.go (6 hunks)
- x/staking/precompile/slashing_info.go (1 hunks)
- x/staking/precompile/slashing_info_test.go (4 hunks)
- x/staking/precompile/transfer_shares.go (2 hunks)
- x/staking/precompile/transfer_shares_test.go (14 hunks)
- x/staking/precompile/undelegate.go (1 hunks)
- x/staking/precompile/undelegate_test.go (7 hunks)
- x/staking/precompile/validator_list.go (1 hunks)
- x/staking/precompile/validator_list_test.go (4 hunks)
- x/staking/precompile/withdraw.go (1 hunks)
- x/staking/precompile/withdraw_test.go (5 hunks)
- x/staking/testutil/staking.go (0 hunks)
- x/staking/testutil/staking_precompile.go (0 hunks)
💤 Files with no reviewable changes (2)
- x/staking/testutil/staking.go
- x/staking/testutil/staking_precompile.go
🔇 Additional comments (83)
testutil/helpers/contract_base_suite.go (2)
23-25: LGTM! Good improvement to enable method chaining.
The return type change enables a more fluent API while maintaining the same functionality.
28-30: LGTM! Method chaining enhancement improves test readability.
The change allows for more concise test setup. For example:
suite.WithSigner(newSigner).WithContract(contractAddr)instead of:
suite.WithSigner(newSigner)
suite.WithContract(contractAddr)x/staking/precompile/allowance_shares.go (1)
Line range hint 1-99: Implementation maintains functionality while improving organization.
The changes only affect the source of types without modifying the underlying functionality. The precompile contract continues to handle allowance queries correctly with proper ABI encoding/decoding.
x/staking/precompile/slashing_info.go (2)
Line range hint 33-57: Implementation follows best practices.
The Run method implementation:
- Properly handles errors at each step
- Uses cache context for state access
- Follows a clear and logical flow
9-9: Verify the import path change impact across the codebase.
The change in import path from x/staking/types to contract package appears to be part of a larger refactor. Let's verify that this change is consistently applied across the codebase.
✅ Verification successful
Import path change is correctly implemented
The verification shows that:
- The
SlashingInfoArgstype is properly defined in the contract package (contract/staking.go) - All precompile-related files consistently use the new contract package import
- Only one file (
x/staking/keeper/shares.go) still uses the old staking types import, but it's unrelated toSlashingInfoArgs
The import path change is part of a broader refactoring where contract-related types have been moved to a dedicated contract package, and this change has been consistently applied across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old import path
# and verify that all relevant files use the new contract package
# Check for any remaining references to the old staking types import
echo "Checking for old import path usage..."
rg "github\.com/functionx/fx-core/v8/x/staking/types.SlashingInfoArgs"
# Check for adoption of new contract package
echo "Checking new import path usage..."
rg "github\.com/functionx/fx-core/v8/contract.SlashingInfoArgs"
Length of output: 300
Script:
#!/bin/bash
# Let's try a different approach to verify the import changes
# First, let's check all files that import from the old staking types package
echo "Files importing from old staking types package:"
rg -l "github\.com/functionx/fx-core/v8/x/staking/types"
# Then check files importing from the new contract package
echo -e "\nFiles importing from new contract package:"
rg -l "github\.com/functionx/fx-core/v8/contract"
# Let's also check the specific file content to understand the context better
echo -e "\nContent of the slashing info file:"
cat x/staking/precompile/slashing_info.go
# Check if SlashingInfoArgs is defined in the contract package
echo -e "\nChecking SlashingInfoArgs definition:"
rg "type SlashingInfoArgs"
Length of output: 5133
x/staking/precompile/validator_list.go (2)
Line range hint 39-58: LGTM! Clean implementation with proper error handling.
The Run method implementation is well-structured with:
- Clear separation of sorting logic
- Proper error handling for both sorting cases
- Clean state management using cache context
7-7: Verify contract package compatibility.
The import path change from x/staking/types to contract package could impact other modules that depend on these types.
✅ Verification successful
Import change is consistent and correctly implemented
The import path change from x/staking/types to contract package is part of a broader refactoring where the ValidatorList related types and functionality have been moved to the contract package. The changes are consistent across the codebase:
- All precompile implementations now use the new
contractpackage import - The
ValidatorListArgsand related types are defined in the contract package - Test files and dependent modules have been updated to use the new import path
- No references to the old import path for
ValidatorListtypes remain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old import path and verify contract package adoption
# Test 1: Check if any files still use the old import path
echo "Checking for old import path usage..."
rg -l "github.com/functionx/fx-core/v8/x/staking/types.ValidatorList"
# Test 2: Verify the new contract package is consistently used
echo "Verifying new contract package adoption..."
rg -l "github.com/functionx/fx-core/v8/contract.ValidatorList"
Length of output: 312
Script:
#!/bin/bash
# Let's try a different approach to verify the impact
# Check for any references to ValidatorList in the codebase
echo "Checking all ValidatorList references..."
rg "ValidatorList" -A 2 -B 2
# Check all imports from staking/types package
echo "Checking staking types imports..."
rg "\"github.com/functionx/fx-core/v8/x/staking/types\""
# Check all imports from contract package
echo "Checking contract package imports..."
rg "\"github.com/functionx/fx-core/v8/contract\""
# Check the specific file content
echo "Checking validator_list.go content..."
cat x/staking/precompile/validator_list.go
Length of output: 21743
x/staking/precompile/slashing_info_test.go (4)
11-11: LGTM: Import aligns with the contract package migration.
The addition of the contract package import supports the transition from types.SlashingInfoArgs to contract.SlashingInfoArgs.
26-26: LGTM: Test case struct properly updated for new contract types.
The malleate function signature change from types.SlashingInfoArgs to contract.SlashingInfoArgs maintains consistency with the codebase refactoring while preserving error handling.
31-35: LGTM: Test cases properly implement new contract types.
The test case implementations:
- Correctly use the new contract.SlashingInfoArgs type
- Maintain comprehensive error handling
- Cover both success and failure scenarios
Also applies to: 40-45, 51-55, 60-65
72-76: LGTM: Test execution flow improvements.
The changes improve the test execution flow by:
- Using more direct validator address access
- Simplifying allowance setup
- Using clearer variable naming for error handling
Also applies to: 78-78, 85-85
testutil/helpers/staking_suite.go (1)
1-9: LGTM!
Package declaration and imports are well-organized and relevant for the staking operations.
x/staking/precompile/approve_shares.go (1)
86-92: LGTM! Clean implementation of PackInput method.
The implementation correctly packs the input arguments following ABI encoding standards with proper error handling.
x/staking/precompile/delegate.go (2)
91-97: LGTM! Clean transition to new argument type.
The change from fxstakingtypes.DelegateV2Args to fxcontract.DelegateV2Args is well-implemented while maintaining the same packing logic.
99-102: LGTM! Improved implementation with cleaner error handling.
The refactored implementation:
- Uses a more idiomatic Go approach with
new()for struct initialization - Leverages
types.ParseMethodArgsfor cleaner argument parsing - Maintains the same functionality with less code
x/staking/precompile/withdraw.go (1)
96-102: LGTM! Clean implementation of PackInput method.
The transition to fxcontract.WithdrawArgs is implemented correctly while maintaining the existing packing logic.
x/staking/precompile/undelegate.go (2)
93-100: LGTM! Type change maintains correct ABI encoding.
The change from fxstakingtypes.UndelegateV2Args to fxcontract.UndelegateV2Args is implemented correctly while preserving the proper ABI encoding logic.
101-104: Verify field compatibility between old and new types.
While the change from fxstakingtypes.UndelegateV2Args to fxcontract.UndelegateV2Args looks correct, let's verify that both types have compatible fields to ensure proper data parsing.
✅ Verification successful
Type compatibility verified between old and new UndelegateV2Args
Both types are actually the same type from contract/staking.go with identical fields and ABI tags:
Validator string \abi:"_val"``Amount *big.Int \abi:"_amount"``
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that both types have compatible fields
# Expected: Both types should have Validator (string) and Amount (*big.Int) fields
# Search for the old type definition
echo "Searching for old type (fxstakingtypes.UndelegateV2Args):"
ast-grep --pattern 'type UndelegateV2Args struct {
$$$
}'
# Search for the new type definition
echo "Searching for new type (fxcontract.UndelegateV2Args):"
ast-grep --pattern 'type UndelegateV2Args struct {
$$$
}'
Length of output: 761
x/staking/precompile/redelegate.go (2)
96-102: LGTM! Clean implementation of PackInput method.
The implementation correctly handles argument packing and maintains proper error handling.
96-107: Verify consistent usage of RedelegateV2Args type.
The change from fxstakingtypes.RedelegateV2Args to fxcontract.RedelegateV2Args is significant. Let's verify that all related code has been updated consistently.
✅ Verification successful
Let me gather more information about the type definitions and their usage to ensure completeness of the verification.
Let me check the test files to ensure the type change is consistently applied.
Type change has been consistently applied across the codebase
The verification shows that:
- The
RedelegateV2Argstype is properly defined incontract/staking.go - All references to the type consistently use the
fxcontractpackage - Test files and helper functions have been updated to use the new type
- No remaining references to the old
fxstakingtypes.RedelegateV2Argswere found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old type and verify new type usage
# Test 1: Check for any remaining references to the old type
echo "Checking for old type references..."
rg "fxstakingtypes\.RedelegateV2Args"
# Test 2: Verify usage of new type
echo "Verifying new type usage..."
rg "fxcontract\.RedelegateV2Args"
Length of output: 541
Script:
#!/bin/bash
# Check for type definitions and related code
echo "Checking type definitions..."
rg -A 5 "type RedelegateV2Args struct"
echo "Checking for any other redelegate-related types or functions..."
rg -l "Redelegate.*Args"
echo "Checking imports in the precompile file..."
rg "^import \(" -A 10 x/staking/precompile/redelegate.go
Length of output: 1214
Script:
#!/bin/bash
# Check test files for type usage
echo "Checking test files for RedelegateV2Args usage..."
rg -A 3 "RedelegateV2Args" x/staking/precompile/redelegate_test.go tests/staking_suite.go
echo "Checking staking precompile test suite..."
rg -A 3 "Redelegate" testutil/helpers/staking_precompile_suite.go
Length of output: 3404
x/staking/precompile/validator_list_test.go (1)
26-26: LGTM! Test cases are well-structured.
The migration from types.ValidatorListArgs to contract.ValidatorListArgs is consistently applied across all test cases, maintaining good coverage of different sorting scenarios and error conditions.
Also applies to: 31-34, 40-43, 49-52, 58-61, 67-70
x/staking/precompile/allowance_shares_test.go (2)
11-13: LGTM! Import changes align with the refactoring.
The addition of the contract package import supports the transition to using contract.AllowanceSharesArgs.
26-27: LGTM! Function signature updated correctly.
The malleate function signature has been properly updated to use contract.AllowanceSharesArgs while maintaining the error handling pattern.
x/staking/precompile/delegation_rewards_test.go (4)
13-14: LGTM: Import changes align with the refactoring goals.
The new imports support the transition to contract-based types and improved test utilities.
29-29: LGTM: Function signature updated correctly.
The malleate function signature has been properly updated to use the new contract-based types while maintaining error handling.
Line range hint 34-94: LGTM: Test cases are well-structured and comprehensive.
The test cases provide good coverage of both success and failure scenarios, with clear error messages and proper separation of contract-specific cases.
101-109: LGTM: Test setup improvements enhance clarity.
The changes improve readability with better variable naming and cleaner contract setup logic.
x/staking/precompile/delegation_test.go (3)
12-14: LGTM! Import changes align with refactoring goals.
The new imports support the transition to contract-based delegation arguments.
27-28: LGTM! Test case structure properly updated.
The malleate function signature has been correctly updated to use the new contract.DelegationArgs type while maintaining proper error handling.
Line range hint 34-116: LGTM! Comprehensive test coverage with clear error handling.
The test cases have been properly updated to use contract.DelegationArgs while maintaining good test coverage for both success and failure scenarios. The error messages are descriptive and will help with debugging.
x/staking/precompile/approve_shares_test.go (1)
13-13: LGTM: Import path update aligns with refactoring goals.
The change from x/staking/types to contract package is consistent with the broader refactoring effort to unify staking-related types.
contract/erc20_token.go (3)
87-87: LGTM: Improved error handling with explicit ABI parameter
The change to use unpackRetIsOk with explicit ABI parameter improves the code by making dependencies clearer and maintaining consistent error handling across the codebase.
108-108: LGTM: Consistent error handling implementation
The change follows the same pattern as the Approve method, maintaining consistency in error handling across the keeper's methods.
116-116: LGTM: Consistent error handling refactor completed
The change completes the consistent refactor of error handling across all token transfer-related methods.
Let's verify that all similar methods in the codebase have been updated to use the new error handling pattern:
✅ Verification successful
Refactoring verified: All unpackRet calls have been replaced with unpackRetIsOk
The verification results show that:
- No instances of the old
unpackRetpattern remain in the codebase - The new
unpackRetIsOkis consistently used across all relevant methods in:- ERC20 token operations (approve, transfer, transferFrom)
- Staking operations (approveShares, delegateV2, redelegateV2, undelegateV2)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining uses of the old unpackRet pattern
rg "unpackRet\(" --type go
# Search for consistent usage of unpackRetIsOk
rg "unpackRetIsOk\(" --type go
Length of output: 745
x/staking/precompile/undelegate_test.go (3)
7-7: LGTM: Import changes are well-structured.
The new imports are properly organized and all are utilized in the updated test implementation.
Also applies to: 11-13, 15-15, 17-18
24-24: LGTM: Test case structure properly updated.
The test cases have been well-structured to:
- Use the new
UndelegateV2Argstype consistently - Cover both success and failure scenarios
- Maintain clear separation between contract and non-contract cases
Also applies to: 30-35, 40-46, 52-57, 62-68
Line range hint 75-117: Consider enhancing event verification.
While the test implementation is generally solid, consider verifying the exact number of expected events/logs to prevent false positives in cases where multiple events of the same type might be emitted.
You could enhance the test by:
- Adding event count verification
- Using a counter instead of a boolean flag
- existEvent := false
+ eventCount := 0
// ... in the event loop
- existEvent = true
+ eventCount++
// ... after the loop
- suite.Require().True(existEvent)
+ suite.Require().Equal(1, eventCount, "expected exactly one event of each attribute type")x/staking/precompile/withdraw_test.go (2)
14-18: LGTM: Import changes align with new contract package usage.
The new imports correctly support the transition to using contract-based argument types.
32-92: LGTM: Test cases properly structured with new contract.WithdrawArgs.
The test cases maintain good coverage of different scenarios while transitioning to the new argument type. Each malleate function correctly handles its specific test case with proper error handling.
x/staking/precompile/redelegate_test.go (2)
7-18: LGTM: Import changes are appropriate.
The new imports align with the refactored test implementation, particularly for handling the new RedelegateV2Args structure and validation methods.
Line range hint 78-130: LGTM: Test execution logic is comprehensive.
The test implementation:
- Properly sets up initial delegation state
- Validates delegation state transitions
- Checks both successful and failed redelegations
- Verifies expected state changes
testutil/helpers/base_suite.go (3)
29-29: LGTM!
The import is correctly added and necessary for accessing fxtypes.DefaultDenom in the new functionality.
188-191: LGTM!
The new GetStakingBalance method is well-implemented:
- Clear and focused purpose
- Consistent with existing balance-related methods
- Properly uses the bank keeper to retrieve balances
- Returns appropriate type for token amounts
61-61: Verify impact on existing tests.
The default amount increase from 100 to 10000 provides more tokens for testing, which is good. However, we should verify that this change doesn't affect any existing tests that might depend on specific token amounts.
Let's check for any tests that might be sensitive to initial token amounts:
✅ Verification successful
Let me analyze the test files that use balance checks or amount comparisons. I notice that most of the comparisons are:
- Relative comparisons (checking if amounts match expected calculations)
- Comparing with specific amounts defined within the test cases
- Comparing with zero or other derived values
The default amount in AddTestSigner is used for initial setup, and the tests that depend on token amounts are using their own specific amounts for assertions rather than depending on this initial amount.
Let me verify this by checking if any test directly uses this default amount:
Change to default amount is safe to proceed
The increase in default amount from 100 to 10000 in AddTestSigner is safe as:
- Test cases that need specific amounts define their own values explicitly
- No tests are directly dependent on or asserting against this default initial amount
- The found usages of 100 in test files are unrelated to this initial balance
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for test cases that might be affected by the token amount change
# Look for assertions or comparisons involving token amounts in test files
# Search for balance checks or amount comparisons in test files
rg -t go "(?i)(CheckBalance|CheckAllBalance|Equal.*Amount|Amount.*Equal)" --glob "**/*_test.go"
Length of output: 9074
Script:
#!/bin/bash
# Search for the specific amount (100 or 10000) in test files
rg -t go "(100|10000).*(?i)(amount|balance)" --glob "**/*_test.go"
Length of output: 377
tests/staking_suite.go (5)
24-24: LGTM: Import change aligns with refactoring.
The addition of the precompile package import supports the transition from stakingprecompile to the new precompile package structure.
146-147: LGTM: Consistent implementation of staking operations.
The UndelegateV2 and WithdrawReward methods correctly implement the new precompile method pattern with appropriate error handling.
Also applies to: 153-154
160-161: LGTM: Query methods properly implemented.
The Delegation and Rewards query methods correctly use the new precompile methods and properly handle the unpacking of return values.
Also applies to: 171-172
182-183: LGTM: Share management methods properly implemented.
All share-related methods (transfer, approval, allowance) correctly implement the new precompile method pattern with consistent error handling.
Also applies to: 206-207, 230-231, 237-238
115-116: LGTM: Consistent implementation of V2 methods.
The implementation correctly uses the new precompile methods. The V2 suffix suggests a version change.
Let's verify if there are any V1 methods still in use:
Also applies to: 122-123
✅ Verification successful
Let me analyze the search results to verify if there are any V1 methods still in use that need migration.
Let me try a different search strategy to find V1 method implementations.
Let me try one final search to be thorough.
Based on the search results, I can now provide a final response:
No V1 methods found, migration to V2 is complete
The codebase shows only V2 implementations for both Delegate and Redelegate methods:
DelegateV2Methodstruct andNewDelegateV2MethodconstructorRedelegateV2Methodstruct andNewRedelegateV2Methodconstructor
No V1 implementations or references were found, indicating the migration to V2 methods is complete.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining V1 method calls to ensure complete migration
rg -g '!*.md' "Delegate[^V2]|Redelegate[^V2]"
Length of output: 102128
Script:
#!/bin/bash
# Search specifically for V1 method implementations in staking precompile
ast-grep --pattern 'func $$$Delegate$$$($$$) {
$$$
}'
Length of output: 57
Script:
#!/bin/bash
# Search for V1 method implementations in precompile files
rg -g "x/staking/precompile/*.go" "^func.*Delegate.*V1.*|NewDelegateV1|NewRedelegateV1"
# Also check for V1 method imports and references
rg -g "*.go" "DelegateV1|RedelegateV1"
Length of output: 130
Script:
#!/bin/bash
# Search for any staking method implementations and references in precompile
rg -g "x/staking/precompile/*.go" "type.*Method.*struct|func New.*Method" -A 2
# Check for method registrations in the precompile
rg -g "x/staking/precompile/*.go" "RegisterMethod.*Delegate|RegisterMethod.*Redelegate" -A 2
Length of output: 5154
contract/contract.go (1)
8-8: LGTM!
The addition of sdkerrors import is appropriate for the new error handling functionality.
x/staking/precompile/contract_test.go (11)
23-28: Embedding StakingPrecompileSuite into PrecompileTestSuite
The helpers.StakingPrecompileSuite is embedded into PrecompileTestSuite, promoting its methods and fields. This enhances code reuse and modularity. Ensure that all required initializations are properly handled and that there are no conflicts with existing methods or fields.
45-46: Initialize signer and mint tokens for testing
The signer is properly initialized, and tokens are minted to the signer's address for testing purposes.
48-49: Deploy staking test contract with proper error handling
The staking test contract is deployed correctly, and the error is appropriately handled using suite.Require().NoError(err).
54-54: Initialize StakingPrecompileSuite with required dependencies
The StakingPrecompileSuite is initialized with the necessary parameters, ensuring that the precompile methods are ready for use in tests.
66-68: Retrieve current delegation details before staking
Fetching delegation details before the staking operation enables verification of the amount staked.
71-74: Delegate tokens using DelegateV2 method with the signer
The delegation is performed correctly using the DelegateV2 method, utilizing the signing account.
78-80: Retrieve updated delegation details after staking
Fetching delegation details after staking to confirm the operation and verify the updated amounts.
83-84: Verify the correct amount has been staked
The assertion ensures that the staked amount matches the expected value, confirming the correctness of the staking operation.
87-89: Fetch signer's balance before withdrawal
Retrieving the signer's balance before withdrawal to calculate the amount received after the operation.
90-91: Withdraw staked tokens using the signer
The withdrawal is executed using the Withdraw method with the correct signer.
96-97: Calculate and return the withdrawn amount
Subtracting the balance before withdrawal from the balance after to determine the net amount withdrawn.
x/staking/precompile/transfer_shares.go (2)
91-95: Update aligns with new contract types
The PackInput method correctly updates the argument type to fxcontract.TransferSharesArgs, reflecting the transition to the fxcontract package. The input arguments are properly packed.
184-188: Update aligns with new contract types
The PackInput method for TransferFromSharesABI correctly updates the argument type to fxcontract.TransferFromSharesArgs. The input arguments are appropriately packed, maintaining consistency with the new data structures.
contract/staking_precompile.go (11)
13-17: Well-defined StakingPrecompileKeeper struct
The StakingPrecompileKeeper struct is properly defined with clear fields: Caller, abi, and contractAddr, which encapsulate the necessary components for contract interactions.
19-28: Correct initialization in NewStakingPrecompileKeeper
The constructor function NewStakingPrecompileKeeper effectively initializes the keeper, handling zero address scenarios by defaulting to StakingAddress. The use of MustABIJson ensures that the ABI is correctly parsed, and any errors are caught at initialization.
36-45: Efficient implementation of AllowanceShares method
The AllowanceShares method accurately queries the contract and returns the share allowance between the specified parties. The use of an output struct to capture the response is appropriate.
47-57: Correct handling of Delegation method outputs
The Delegation method successfully retrieves both the shares and the delegate amount, returning them with proper error handling. This provides comprehensive delegation information as expected.
59-68: Proper retrieval of rewards in DelegationRewards method
The method correctly queries for delegation rewards and handles errors appropriately, ensuring that the reward information is accurately obtained.
70-77: Effective implementation of ValidatorList method
The ValidatorList method retrieves a list of validators based on the specified sorting criterion. Error handling ensures that any issues during the query are properly reported.
79-89: Accurate slashing information in SlashingInfo method
The method provides detailed slashing information, including whether a validator is jailed and the number of missed blocks. The output struct captures all necessary data, and errors are handled correctly.
91-97: Consistent use of unpackRetIsOk in ApproveShares method
The ApproveShares method applies the contract call and utilizes unpackRetIsOk for error handling and response validation, ensuring that the approval process is robust.
99-109: Proper response unpacking in TransferShares method
The method executes the transfer of shares and unpacks the response into TransferSharesRet. Error handling is thorough, capturing issues both from the contract application and the unpacking process.
111-121: Comprehensive handling in TransferFromShares method
TransferFromShares effectively manages the transfer on behalf of another address, with appropriate error checks and response unpacking into TransferFromSharesRet.
135-157: Consistent use of unpackRetIsOk in delegation methods
The methods DelegateV2, RedelegateV2, and UndelegateV2 consistently apply the contract calls and utilize unpackRetIsOk for error handling. This uniform approach simplifies maintenance and improves code clarity.
contract/staking.go (4)
78-83: GetValidator method for DelegateV2Args is correctly implemented.
The method properly retrieves the validator address from the Bech32 string.
143-145: GetValidatorSrc method is correctly implemented.
The method properly retrieves the source validator address from the Bech32 string.
149-151: GetValidatorDst method is correctly implemented.
The method properly retrieves the destination validator address from the Bech32 string.
227-231: GetValidator method for UndelegateV2Args is correctly implemented.
The method properly retrieves the validator address from the Bech32 string.
x/staking/precompile/delegate_test.go (3)
26-27: Verify the change from RandSigner() to NewSigner().
The test now uses suite.NewSigner() instead of suite.RandSigner(). Ensure that NewSigner() provides the desired behavior and that it appropriately replaces RandSigner() without affecting the test logic.
69-69: Confirm the use of contract.DelegateV2Args in function signatures.
The malleate function now returns contract.DelegateV2Args instead of types.DelegateV2Args. Verify that contract.DelegateV2Args is the correct type and aligns with the updated contract definitions.
215-215:
Add a check for empty log.Topics before accessing the first element.
Accessing log.Topics[0] without ensuring that log.Topics has at least one element can lead to an index out of range error if Topics is empty. Add a length check before accessing the element.
Modify the condition as follows:
-if log.Topics[0] == delegateV2Method.Event.ID.String() {
+if len(log.Topics) > 0 && log.Topics[0] == delegateV2Method.Event.ID.String() {Likely invalid or redundant comment.
x/staking/precompile/transfer_shares_test.go (2)
359-362: Check for potential nil pointer dereference when unpacking events
When unpacking events in line 362, ensure that log.ToEthereum() does not return nil to prevent possible nil pointer dereference.
Run the following script to verify that log.ToEthereum() is not nil:
✅ Verification successful
Based on the context gathered, I can now provide the final response:
No nil pointer risk in event log unpacking
The code is safe from nil pointer dereference because:
- The log is obtained from a range loop over
res.Logs, ensuring it's a valid log entry - The code performs proper validation before unpacking:
- Checks the log topic length (
suite.Require().Len(log.Topics, 3)) - Verifies the event ID matches (
log.Topics[0] == abi.Event.ID.String())
- Checks the log topic length (
- Any potential error from unpacking is immediately checked (
suite.Require().NoError(err))
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that log.ToEthereum() is not nil in event unpacking
# Test: Search for instances where ToEthereum() might return nil
# Expected: No instances should allow ToEthereum() to be nil before unpacking
rg 'log\.ToEthereum\(\)' -A 5 -- 'x/staking/precompile/transfer_shares_test.go'
Length of output: 864
Script:
#!/bin/bash
# Let's check the implementation of ToEthereum() method and its usage context
# First, let's find the definition of ToEthereum()
ast-grep --pattern 'func $_ ToEthereum() $_'
# Then, let's see the full test context where it's used
rg -B 10 -A 10 'log\.ToEthereum\(\)' x/staking/precompile/transfer_shares_test.go
Length of output: 1771
Script:
#!/bin/bash
# Let's check the type definition of 'log' to understand its safety guarantees
# First, check the test file imports to understand the log type
rg '^import \(' -A 20 x/staking/precompile/transfer_shares_test.go
# Then check the struct/interface definition of the test suite
ast-grep --pattern 'type $_ struct {
$$$
}' x/staking/precompile/transfer_shares_test.go
Length of output: 908
751-754: Check for potential nil pointer dereference when unpacking events
Similar to a previous instance, when unpacking events in line 754, ensure that log.ToEthereum() is not nil.
Run the following script to verify:
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
x/staking/precompile/undelegate_test.go (1)
75-86: Enhance assertion messages for better debugging.
While the test execution is thorough, consider adding more descriptive messages to assertions to make test failures more informative.
Example improvements:
-suite.Require().NoError(err)
+suite.Require().NoError(err, "failed to get undelegations")
-suite.Require().Equal(1, len(undelegations))
+suite.Require().Equal(1, len(undelegations), "expected exactly one undelegation entry")
-suite.Require().Equal(delAmt, undelegations[0].Entries[0].Balance)
+suite.Require().Equal(delAmt, undelegations[0].Entries[0].Balance,
"undelegation amount mismatch, expected: %s, got: %s",
delAmt, undelegations[0].Entries[0].Balance)Also applies to: 99-116
x/staking/precompile/delegate_test.go (4)
210-210: Remove unused parameter shares from CheckDelegateLogs function
The parameter shares in the CheckDelegateLogs function is not used within the function body. Removing this unused parameter will enhance code clarity and maintainability.
Apply this diff to remove the unused parameter:
-func (suite *PrecompileTestSuite) CheckDelegateLogs(logs []*evmtypes.Log, delAddr common.Address, valAddr string, amount, shares *big.Int) {
+func (suite *PrecompileTestSuite) CheckDelegateLogs(logs []*evmtypes.Log, delAddr common.Address, valAddr string, amount *big.Int) {192-193: Move abi initialization outside the loop
The abi variable is initialized inside the loop but remains constant. Moving it outside the loop will improve efficiency by avoiding redundant initializations.
Apply this diff to move abi outside the loop:
func (suite *PrecompileTestSuite) TestDelegateV2() {
// ... existing code ...
if tc.result {
- for _, log := range res.Logs {
- abi := precompile.NewDelegateV2ABI()
+ abi := precompile.NewDelegateV2ABI()
+ for _, log := range res.Logs {
if log.Topics[0] == abi.Event.ID.String() {
// ... existing assertions ...
}Line range hint 69-105: Ensure sufficient token balance for delegations in test cases
In some of the test cases, particularly those not prefixed with "contract", the delegator may not have sufficient token balance for delegation, which could lead to test failures.
Consider minting tokens for the delegator before performing the delegation, similar to how it's done in the "contract" test cases. Apply this diff to ensure the delegator has enough tokens:
func (suite *PrecompileTestSuite) TestDelegateV2() {
testCases := []struct {
name string
malleate func(val sdk.ValAddress, delAmount sdkmath.Int) (contract.DelegateV2Args, error)
result bool
}{
{
name: "ok",
malleate: func(val sdk.ValAddress, delAmount sdkmath.Int) (contract.DelegateV2Args, error) {
+ suite.MintToken(suite.signer.AccAddress(), sdk.NewCoin(fxtypes.DefaultDenom, delAmount))
return contract.DelegateV2Args{
Validator: val.String(),
Amount: delAmount.BigInt(),
}, nil
},
result: true,
},26-27: Avoid using random amounts in tests
Using helpers.NewRandAmount() can introduce nondeterminism in your tests, potentially causing flaky tests if the random amount is zero or unsuitable.
Consider using a fixed amount for delegation to ensure test consistency:
-delAmount := helpers.NewRandAmount()
+delAmount := sdkmath.NewInt(1000) // Replace 1000 with a suitable fixed amount📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- x/staking/precompile/delegate_test.go (9 hunks)
- x/staking/precompile/undelegate_test.go (7 hunks)
🔇 Additional comments (2)
x/staking/precompile/undelegate_test.go (2)
7-18: LGTM! Import changes align with new functionality.
The new imports support the enhanced testing capabilities for logs and events.
123-162:
Improve event and log verification robustness.
The current implementation has two potential issues:
- The existence flags (
existLogandexistEvent) don't guarantee that all required fields were verified - The event verification logic could be more maintainable
For the first issue, consider tracking each required field:
type LogFields struct {
hasValidator bool
hasAmount bool
hasCompletionTime bool
}
func (l LogFields) AllFieldsVerified() bool {
return l.hasValidator && l.hasAmount && l.hasCompletionTime
}For the second issue, I agree with the previous review comment about consolidating event attribute verification. Consider implementing a generic helper as suggested in the previous review.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (16)
x/staking/precompile/validator_list.go (1)
7-7: Consider documenting the rationale for the package restructuring.
While the changes from fxstakingtypes to fxcontract are implemented correctly, it would be helpful to document the reasoning behind this architectural decision. This could help future maintainers understand:
- Why the types were moved to a new package
- What benefits this restructuring provides
- Any potential implications for future development
Also applies to: 51-51, 73-73, 81-81
x/staking/precompile/allowance_shares_test.go (3)
16-19: Consider enhancing ABI test coverage.
While the test verifies the number of inputs and outputs, it could be more comprehensive by also validating:
- Input parameter types and names
- Output parameter types and names
- Method name and signature
Here's a suggested enhancement:
func TestStakingAllowanceSharesABI(t *testing.T) {
allowanceSharesABI := precompile.NewAllowanceSharesABI()
require.Len(t, allowanceSharesABI.Method.Inputs, 3)
require.Len(t, allowanceSharesABI.Method.Outputs, 1)
+ // Verify input parameters
+ require.Equal(t, "validator", allowanceSharesABI.Method.Inputs[0].Name)
+ require.Equal(t, "owner", allowanceSharesABI.Method.Inputs[1].Name)
+ require.Equal(t, "spender", allowanceSharesABI.Method.Inputs[2].Name)
+ // Verify output
+ require.Equal(t, "shares", allowanceSharesABI.Method.Outputs[0].Name)
+ require.Equal(t, "uint256", allowanceSharesABI.Method.Outputs[0].Type.String())
}71-77: Enhance error handling verification and test coverage.
While the current test cases cover basic scenarios, consider adding:
- Error case verification could be more explicit
- Missing edge cases for allowance values
Consider adding these test cases:
{
name: "max uint256 allowance",
malleate: func(val sdk.ValAddress, owner, spender *helpers.Signer) (contract.AllowanceSharesArgs, error) {
maxUint256 := new(big.Int).Sub(new(big.Int).Lsh(big.NewInt(1), 256), big.NewInt(1))
suite.SetAllowance(val, owner.AccAddress(), spender.AccAddress(), maxUint256)
return contract.AllowanceSharesArgs{
Validator: val.String(),
Owner: owner.Address(),
Spender: spender.Address(),
}, nil
},
result: true,
},
{
name: "revoked allowance",
malleate: func(val sdk.ValAddress, owner, spender *helpers.Signer) (contract.AllowanceSharesArgs, error) {
suite.SetAllowance(val, owner.AccAddress(), spender.AccAddress(), big.NewInt(0))
return contract.AllowanceSharesArgs{
Validator: val.String(),
Owner: owner.Address(),
Spender: spender.Address(),
}, nil
},
result: true,
}75-77: Improve error case handling.
The error verification could be more explicit. Consider separating the error check from the shares validation.
- shares := suite.WithError(expectErr).AllowanceShares(suite.Ctx, args)
- if tc.result && shares.Sign() > 0 {
- suite.Require().Equal(allowanceAmt.BigInt(), shares)
+ shares, err := suite.AllowanceShares(suite.Ctx, args)
+ if expectErr != nil {
+ suite.Require().Error(err)
+ suite.Require().Contains(err.Error(), expectErr.Error())
+ return
+ }
+ suite.Require().NoError(err)
+ if tc.result && shares.Sign() > 0 {
+ suite.Require().Equal(allowanceAmt.BigInt(), shares)x/staking/precompile/delegation_test.go (1)
Line range hint 23-98: LGTM with suggestions for additional test coverage.
The test suite is well-structured and covers basic scenarios. Consider adding the following test cases for more comprehensive coverage:
- Maximum delegation amount validation
- Negative amount handling
- Multiple delegations to the same validator
Example test case structure:
{
name: "failed - maximum delegation amount exceeded",
malleate: func(val sdk.ValAddress, del common.Address) (contract.DelegationArgs, error) {
return contract.DelegationArgs{
Validator: val.String(),
Delegator: del,
}, fmt.Errorf("delegation amount exceeds maximum allowed")
},
result: false,
}x/staking/precompile/delegation_rewards_test.go (2)
18-21: Consider enhancing ABI test coverage.
While the basic parameter count validation is good, consider adding checks for:
- Parameter types
- Parameter names
- Method name and signature
func TestStakingDelegationRewardsABI(t *testing.T) {
delegationRewardsABI := precompile.NewDelegationRewardsABI()
require.Len(t, delegationRewardsABI.Method.Inputs, 2)
require.Len(t, delegationRewardsABI.Method.Outputs, 1)
+ // Validate input parameters
+ require.Equal(t, "validator", delegationRewardsABI.Method.Inputs[0].Name)
+ require.Equal(t, "delegator", delegationRewardsABI.Method.Inputs[1].Name)
+ // Validate method name
+ require.Equal(t, "delegationRewards", delegationRewardsABI.Method.Name)
}Line range hint 24-63: Consider adding more edge cases to the test suite.
The current test cases cover basic success and failure scenarios, but consider adding:
- Zero delegation amount case
- Multiple delegations case
- Redelegation rewards case
Would you like me to help generate the additional test cases?
x/staking/precompile/validator_list_test.go (5)
15-19: Consider enhancing ABI validation test coverage.
While the current test validates the number of inputs and outputs, consider adding assertions for:
- Input and output parameter types
- Method name and signature
- Event definitions (if any)
func TestValidatorListABI(t *testing.T) {
validatorListABI := precompile2.NewValidatorListABI()
require.Len(t, validatorListABI.Method.Inputs, 1)
require.Len(t, validatorListABI.Method.Outputs, 1)
+ require.Equal(t, "validatorList", validatorListABI.Method.Name)
+ require.Equal(t, "uint8", validatorListABI.Method.Inputs[0].Type.String())
+ require.Equal(t, "address[]", validatorListABI.Method.Outputs[0].Type.String())
}22-54: Consider adding more edge cases to the test suite.
The current test cases cover basic functionality and one error case. Consider adding:
- Empty validator set scenario
- Maximum validator set size scenario
- Multiple validators with identical power/missed blocks
- Invalid spender/operator addresses
Would you like me to help generate these additional test cases?
67-68: Make error handling more explicit.
The error handling using WithError() could be more explicit. Consider validating the error message content for better test coverage.
- valAddrs := suite.WithError(expectErr).ValidatorList(suite.Ctx, args)
+ valAddrs, err := suite.App.ValidatorList(suite.Ctx, args)
+ if expectErr != nil {
+ suite.Require().Error(err)
+ suite.Require().Contains(err.Error(), expectErr.Error())
+ return
+ }
+ suite.Require().NoError(err)Line range hint 78-96: Extract sorting logic for better readability.
The sorting logic for missed blocks is complex and could benefit from being extracted into a helper function for better readability and reusability.
+func (suite *StakingPrecompileTestSuite) getSortedValidatorsByMissedBlocks(validators []stakingtypes.Validator) []precompile2.Validator {
+ valList := make([]precompile2.Validator, 0, len(validators))
+ for _, validator := range validators {
+ consAddr, err := validator.GetConsAddr()
+ suite.Require().NoError(err)
+ info, err := suite.App.SlashingKeeper.GetValidatorSigningInfo(suite.Ctx, consAddr)
+ suite.Require().NoError(err)
+ valList = append(valList, precompile2.Validator{
+ ValAddr: validator.OperatorAddress,
+ MissedBlocks: info.MissedBlocksCounter,
+ })
+ }
+ sort.Slice(valList, func(i, j int) bool {
+ return valList[i].MissedBlocks > valList[j].MissedBlocks
+ })
+ return valList
+}Then use it in the test:
if args.GetSortBy() == contract.ValidatorSortByMissed {
- valList := make([]precompile2.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, precompile2.Validator{
- ValAddr: validator.OperatorAddress,
- MissedBlocks: info.MissedBlocksCounter,
- })
- }
- sort.Slice(valList, func(i, j int) bool {
- return valList[i].MissedBlocks > valList[j].MissedBlocks
- })
+ valList := suite.getSortedValidatorsByMissedBlocks(valsByPower)
for index, addr := range valAddrs {
suite.Require().Equal(addr, valList[index].ValAddr)
}
}31-33: Define constants for validator sort types.
The sort type is specified using a magic number conversion. Consider using named constants for better clarity.
+const (
+ ValidatorSortByPower = uint8(contract.ValidatorSortByPower)
+ ValidatorSortByMissed = uint8(contract.ValidatorSortByMissed)
+)
return contract.ValidatorListArgs{
- SortBy: uint8(contract.ValidatorSortByPower),
+ SortBy: ValidatorSortByPower,
}, nilx/staking/precompile/contract_test.go (2)
40-42: Remove redundant SetupSubTest method.
The SetupSubTest method only calls SetupTest without adding any additional functionality.
Consider removing this method unless there's a specific reason for its existence that should be documented.
102-112: Add balance validation for withdrawal.
The PrecompileStakingWithdraw correctly tracks balance changes, but consider adding validation to ensure:
- The withdrawn amount matches the delegated amount
- No remaining delegation exists after withdrawal
func (suite *StakingPrecompileTestSuite) PrecompileStakingWithdraw(signer *helpers.Signer, val sdk.ValAddress) sdk.Coins {
balanceBefore := suite.App.BankKeeper.GetAllBalances(suite.Ctx, signer.AccAddress())
+ // Get delegation before withdrawal
+ _, amountBefore := suite.Delegation(suite.Ctx, contract.DelegationArgs{
+ Validator: val.String(),
+ Delegator: signer.Address(),
+ })
suite.WithSigner(signer)
res, _ := suite.Withdraw(suite.Ctx, contract.WithdrawArgs{
Validator: val.String(),
})
suite.Require().False(res.Failed(), res.VmError)
balanceAfter := suite.App.BankKeeper.GetAllBalances(suite.Ctx, signer.AccAddress())
- return balanceAfter.Sub(balanceBefore...)
+ withdrawnAmount := balanceAfter.Sub(balanceBefore...)
+
+ // Verify complete withdrawal
+ _, amountAfter := suite.Delegation(suite.Ctx, contract.DelegationArgs{
+ Validator: val.String(),
+ Delegator: signer.Address(),
+ })
+ suite.Require().True(amountAfter.IsZero(), "delegation should be zero after withdrawal")
+ suite.Require().Equal(amountBefore.String(), withdrawnAmount.AmountOf(fxtypes.DefaultDenom).String(), "withdrawn amount should match delegation")
+
+ return withdrawnAmount
}x/staking/precompile/redelegate_test.go (1)
Line range hint 21-99: Consider adding more edge cases to the test suite.
While the current test cases cover basic success and failure scenarios, consider adding the following test cases to improve coverage:
- Zero amount redelegation
- Redelegation to the same validator
- Redelegation with invalid amount (e.g., exceeding balance)
Example test case structure:
{
name: "failed - zero amount redelegation",
malleate: func(valSrc, valDst sdk.ValAddress, shares sdkmath.LegacyDec, delAmount sdkmath.Int) (contract.RedelegateV2Args, error) {
return contract.RedelegateV2Args{
ValidatorSrc: valSrc.String(),
ValidatorDst: valDst.String(),
Amount: big.NewInt(0),
}, fmt.Errorf("invalid zero amount")
},
result: false,
}x/staking/precompile/keeper.go (1)
Line range hint 108-113: LGTM! Consider refactoring duplicate event emission logic.
The event emission logic is identical to the previous one except for different parameters. Consider extracting this into a helper method to reduce duplication and improve maintainability.
Here's a suggested refactor:
+func (k Keeper) emitWithdrawEvent(evm *vm.EVM, withdrawABI *WithdrawABI, addr common.Address, valAddr string, amount *big.Int) error {
+ data, topic, err := withdrawABI.NewWithdrawEvent(addr, valAddr, amount)
+ if err != nil {
+ return err
+ }
+ fxcontract.EmitEvent(evm, stakingAddress, data, topic)
+ return nil
+}Then use it in both places:
-data, topic, err := withdrawABI.NewWithdrawEvent(from, valAddr.String(), withdrawRewardRes.Amount.AmountOf(k.stakingDenom).BigInt())
-if err != nil {
- return nil, nil, err
-}
-fxcontract.EmitEvent(evm, stakingAddress, data, topic)
+if err := k.emitWithdrawEvent(evm, withdrawABI, from, valAddr.String(), withdrawRewardRes.Amount.AmountOf(k.stakingDenom).BigInt()); err != nil {
+ return nil, nil, err
+}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
- x/staking/precompile/allowance_shares.go (2 hunks)
- x/staking/precompile/allowance_shares_test.go (3 hunks)
- x/staking/precompile/approve_shares_test.go (4 hunks)
- x/staking/precompile/contract_test.go (1 hunks)
- x/staking/precompile/delegate_test.go (4 hunks)
- x/staking/precompile/delegation.go (2 hunks)
- x/staking/precompile/delegation_rewards.go (2 hunks)
- x/staking/precompile/delegation_rewards_test.go (4 hunks)
- x/staking/precompile/delegation_test.go (5 hunks)
- x/staking/precompile/keeper.go (2 hunks)
- x/staking/precompile/redelegate.go (3 hunks)
- x/staking/precompile/redelegate_test.go (4 hunks)
- x/staking/precompile/slashing_info.go (2 hunks)
- x/staking/precompile/slashing_info_test.go (2 hunks)
- x/staking/precompile/transfer_shares_test.go (12 hunks)
- x/staking/precompile/undelegate_test.go (5 hunks)
- x/staking/precompile/validator_list.go (3 hunks)
- x/staking/precompile/validator_list_test.go (2 hunks)
- x/staking/precompile/withdraw_test.go (4 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- x/staking/precompile/allowance_shares.go
- x/staking/precompile/approve_shares_test.go
- x/staking/precompile/delegation_rewards.go
- x/staking/precompile/redelegate.go
- x/staking/precompile/slashing_info_test.go
- x/staking/precompile/withdraw_test.go
🔇 Additional comments (24)
x/staking/precompile/validator_list.go (3)
51-53: LGTM! Validator sorting constants updated correctly.
The constant references have been properly updated to use the new fxcontract package while maintaining the same sorting functionality.
73-73: LGTM! Method signatures properly updated.
The method signatures have been correctly updated to use the new fxcontract.ValidatorListArgs type while maintaining proper error handling and type safety.
Also applies to: 81-82
7-7: Verify consistent usage of the new import path across the codebase.
The change from fxstakingtypes to fxcontract appears to be part of a broader refactoring effort.
✅ Verification successful
Based on the results, I can generate the final response as we have clear evidence that:
- There are no remaining references to
fxstakingtypesin the codebase - The new
fxcontractimport is consistently used across multiple precompile and keeper files
Import path change is consistent across the codebase
The migration from fxstakingtypes to fxcontract has been completely and consistently implemented. The new import is properly used across all relevant precompile and keeper files, with no lingering references to the old import path.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that fxstakingtypes is completely replaced with fxcontract
# across the codebase to ensure consistency
echo "Checking for any remaining references to fxstakingtypes..."
rg "fxstakingtypes" -l
echo "Verifying the new fxcontract import usage..."
rg 'fxcontract.*"github.com/functionx/fx-core/v8/contract"' -l
Length of output: 985
x/staking/precompile/slashing_info.go (4)
15-15: LGTM! Constructor changes are consistent.
The struct field and constructor changes correctly reflect the type rename from SlashingABI to SlashingInfoABI.
Also applies to: 20-21
73-84: LGTM! Pack/Unpack implementations are robust.
The Pack/Unpack methods:
- Handle errors appropriately
- Follow EVM ABI encoding patterns
- Maintain type safety with proper assertions
Also applies to: 87-98
9-9: Verify consistent usage of new import.
The change from fxstakingtypes to fxcontract looks good, but let's verify all usages are updated consistently.
#!/bin/bash
# Description: Verify no remaining references to old package
# Test 1: Search for any remaining references to fxstakingtypes
rg "fxstakingtypes\.SlashingInfoArgs"
# Test 2: Verify new type is used consistently
rg "fxcontract\.SlashingInfoArgs"67-70: Verify stakingABI variable definition.
The constructor uses stakingABI.Methods which isn't defined in this file. Let's verify its existence and initialization.
✅ Verification successful
Let me try a different search pattern to find the stakingABI definition.
stakingABI is properly defined and initialized
The variable stakingABI is defined in x/staking/precompile/contract.go and initialized using contract.MustABIJson(contract.IStakingMetaData.ABI). This is a package-level variable that's accessible to all files in the same package, including slashing_info.go. The usage is consistent across multiple precompile ABIs in the staking package, showing it's a well-established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find stakingABI definition
# Test: Search for stakingABI variable definition
rg "var\s+stakingABI\s*=|stakingABI\s*:=|stakingABI\s*=" -A 5
Length of output: 408
Script:
#!/bin/bash
# Search for stakingABI definition with broader pattern
rg "stakingABI" -B 2 -A 2
# Also search in abi.go files which commonly contain ABI definitions
fd "abi.go" --exec rg "stakingABI" -B 2 -A 2 {}
Length of output: 4694
x/staking/precompile/allowance_shares_test.go (2)
10-12: LGTM: Import changes align with package migration.
The addition of the contract package import is consistent with the transition from types to contract package for argument structures.
22-25: LGTM: Improved test structure and type consistency.
The renaming of the test suite to StakingPrecompileTestSuite and the update to use contract.AllowanceSharesArgs improve clarity and maintain consistency with the new contract types.
x/staking/precompile/delegation.go (3)
76-82: LGTM: PackInput method updated correctly.
The method signature has been updated to use the new fxcontract.DelegationArgs type while maintaining the same functionality.
84-88: LGTM: UnpackInput method updated correctly.
The method has been properly updated to use fxcontract.DelegationArgs and maintains proper error handling through types.ParseMethodArgs.
11-11: Verify import consistency across the codebase.
The change from fxstakingtypes to fxcontract for DelegationArgs needs to be consistent across all related files.
✅ Verification successful
Import change is consistent and correctly implemented
The verification shows that:
DelegationArgsis properly defined in thecontractpackage- All usages of
DelegationArgsconsistently reference the newfxcontractimport - The change is part of a broader, consistent refactoring where the contract package is used across multiple components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining old imports and verify new import adoption
# Check for files still using the old import path
echo "Files still using old import path:"
rg -l "github\.com/functionx/fx-core/v8/x/staking/types.+DelegationArgs"
# Check for files already using the new import path
echo "Files using new import path:"
rg -l "github\.com/functionx/fx-core/v8/contract.+DelegationArgs"
Length of output: 288
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# Check for DelegationArgs usage across the codebase
echo "Files containing DelegationArgs:"
rg -l "DelegationArgs"
# Check for any references to the contract package
echo -e "\nFiles importing fxcontract:"
rg -l "fxcontract|\"github\.com/functionx/fx-core/v8/contract\""
# Check the actual content of the delegation.go file
echo -e "\nContent of delegation.go:"
cat x/staking/precompile/delegation.go
Length of output: 5381
Script:
#!/bin/bash
# Let's check the actual implementation and usage of DelegationArgs in both locations
# Check the implementation in contract package
echo "DelegationArgs in contract package:"
rg -A 5 "type DelegationArgs" contract/
# Check the content of files that use DelegationArgs
echo -e "\nDelegationArgs usage in test files:"
rg -A 5 "DelegationArgs" x/staking/precompile/delegation_test.go
rg -A 5 "DelegationArgs" x/staking/precompile/contract_test.go
Length of output: 2191
x/staking/precompile/delegation_test.go (2)
11-11: LGTM: Import changes align with the refactoring.
The addition of the contract package import is consistent with the transition from types.DelegationArgs to contract.DelegationArgs.
17-20: LGTM: ABI test is properly updated.
The test correctly verifies the structure of the new delegation ABI with appropriate input and output length checks.
x/staking/precompile/delegation_rewards_test.go (2)
12-13: LGTM! Import changes align with the refactoring objectives.
The new imports support the transition to using contract types and test helpers.
67-92: 🛠️ Refactor suggestion
Improve test execution clarity and reliability.
- The test setup logic could be moved to a helper function to improve readability
- The existing string comparison issue (noted in previous review) is still present
Consider refactoring the test setup into a helper function:
+func (suite *StakingPrecompileTestSuite) setupDelegation(operator sdk.ValAddress) (common.Address, *big.Int) {
+ delAmt := helpers.NewRandAmount()
+ delAddr := suite.GetDelAddr()
+
+ res := suite.DelegateV2(suite.Ctx, contract.DelegateV2Args{
+ Validator: operator.String(),
+ Amount: delAmt.BigInt(),
+ })
+ suite.Require().False(res.Failed(), res.VmError)
+ suite.Commit()
+ return delAddr, delAmt.BigInt()
+}Then use it in the test:
- operator0 := suite.GetFirstValAddr()
- delAmt := helpers.NewRandAmount()
- delAddr := suite.GetDelAddr()
-
- res := suite.DelegateV2(suite.Ctx, contract.DelegateV2Args{
- Validator: operator0.String(),
- Amount: delAmt.BigInt(),
- })
- suite.Require().False(res.Failed(), res.VmError)
- suite.Commit()
+ operator0 := suite.GetFirstValAddr()
+ delAddr, delAmt := suite.setupDelegation(operator0)The string comparison issue noted in the previous review at line 91 is still valid and should be addressed.
x/staking/precompile/contract_test.go (2)
44-59: LGTM! Well-structured test setup.
The setup method demonstrates good practices:
- Proper initialization of test environment
- Conditional contract deployment
- Error handling
- Test token minting
78-99: Consider adding edge case tests for delegation.
The PrecompileStakingDelegateV2 implementation looks good with proper validation and balance checks. However, consider adding tests for:
- Zero amount delegation
- Maximum amount delegation
- Invalid validator address
Let's check if these edge cases are covered elsewhere:
x/staking/precompile/redelegate_test.go (1)
7-18: LGTM! Import changes are appropriate.
The new imports support the enhanced testing functionality and align with the refactored contract definitions.
x/staking/precompile/keeper.go (1)
80-84: LGTM! Verify event compatibility with existing consumers.
The switch to ABI-based event emission is a good practice for standardization. However, ensure that all existing event consumers (e.g., indexers, listeners) are compatible with the new event format.
Let's verify the event structure compatibility:
✅ Verification successful
Let me analyze the search results to verify the event compatibility:
- The
NewWithdrawEventis consistently used across the codebase in:x/staking/precompile/keeper.gox/staking/precompile/withdraw.go- Test files (
tests/staking_test.go,tests/staking_suite.go)
Let's verify if the event structure remains the same by checking the ABI definition:
Let's check the ABI definition to verify the event structure:
Let's check the contract interface definition to verify the event structure:
Based on the gathered information, I can now generate the final response. The search results show that:
- The
Withdrawevent structure is defined in the ABI and remains consistent:
"name": "Withdraw",
"inputs": [
{"indexed": true, "name": "sender", "type": "address"},
{"indexed": false, "name": "validator", "type": "string"},
{"indexed": false, "name": "reward", "type": "uint256"}
]- The event is used consistently across the codebase:
- In tests (
tests/staking_test.go,tests/staking_suite.go) - In the precompile implementation (
x/staking/precompile/withdraw.go) - In the keeper implementation (
x/staking/precompile/keeper.go)
- In tests (
✓ Event structure is compatible with existing consumers
The switch from NewWithdrawMethod to NewWithdrawABI maintains the same event structure as defined in the contract interface (IStakingMetaData). All event consumers in the codebase are using the same event structure, ensuring backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing event consumers in the codebase
rg -A 5 "NewWithdrawMethod|WithdrawEvent" --type go
Length of output: 3688
Script:
#!/bin/bash
# Search for the WithdrawABI definition
rg -A 10 "NewWithdrawABI" --type go
Length of output: 3770
Script:
#!/bin/bash
# Search for the staking ABI definition
rg -A 10 "stakingABI\s*=" --type go
Length of output: 652
Script:
#!/bin/bash
# Search for the IStaking interface or contract definition
rg -A 15 "IStakingMetaData|interface IStaking" --type go
Length of output: 11651
x/staking/precompile/transfer_shares_test.go (1)
875-1020: LGTM! Well-structured helper methods.
The helper methods are well-implemented with:
- Consistent error handling
- Clear separation of concerns
- Proper parameter validation
x/staking/precompile/delegate_test.go (3)
23-25: Approved: Conditional test skipping implemented correctly
The test now correctly checks if precompile calls are enabled and skips the test if they are not, ensuring appropriate test execution in different environments.
57-57: Previous review comment is still valid for this code segment.
182-182: Previous review comment is still valid for this code segment.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests