refactor(staking/precompile): split into method and abi #785
Conversation
WalkthroughThe pull request implements significant structural changes across multiple files in the staking precompile package. The primary focus is on introducing new ABI types (e.g., 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (17)
x/staking/precompile/slashing_info.go (2)
63-71: Consider using a constant for the method name.While the implementation is correct, using a string literal "slashingInfo" could lead to runtime errors if mistyped or if the method name changes. Consider defining it as a constant.
+const ( + MethodSlashingInfo = "slashingInfo" +) func NewSlashingABI() SlashingABI { return SlashingABI{ - Method: stakingABI.Methods["slashingInfo"], + Method: stakingABI.Methods[MethodSlashingInfo], } }
Line range hint
91-98: Add type assertion error handling in UnpackOutput.The type assertions
unpack[0].(bool)andunpack[1].(*big.Int)could panic if the types don't match. Consider adding explicit type checks.func (m SlashingABI) 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("expected bool for jailed, got %T", unpack[0]) + } + missed, ok := unpack[1].(*big.Int) + if !ok { + return false, nil, fmt.Errorf("expected *big.Int for missed, got %T", unpack[1]) + } - return unpack[0].(bool), unpack[1].(*big.Int), nil + return jailed, missed, nil }x/staking/precompile/delegation.go (2)
66-74: Consider using a constant for the method nameWhile the implementation is clean, using a string literal "delegation" for the method name could be fragile. Consider defining it as a package-level constant to ensure consistency and make updates easier.
+const ( + DelegationMethod = "delegation" +) func NewDelegationABI() DelegationABI { return DelegationABI{ - Method: stakingABI.Methods["delegation"], + Method: stakingABI.Methods[DelegationMethod], } }
Line range hint
94-102: Consider using named return values for better readabilityThe
UnpackOutputmethod could benefit from named return values to make the return values more self-documenting.-func (m DelegationABI) UnpackOutput(data []byte) (*big.Int, *big.Int, error) { +func (m DelegationABI) UnpackOutput(data []byte) (shares *big.Int, amount *big.Int, err error) { unpack, err := m.Method.Outputs.Unpack(data) if err != nil { return nil, nil, err } shares := unpack[0].(*big.Int) amount := unpack[1].(*big.Int) return shares, amount, nil }x/staking/precompile/approve_shares.go (2)
67-77: Consider adding validation in NewApproveSharesABI.While the implementation is clean, consider adding validation to ensure
stakingABIcontains the required method and event to prevent potential panics.func NewApproveSharesABI() ApproveSharesABI { + method, exists := stakingABI.Methods["approveShares"] + if !exists { + panic("approveShares method not found in stakingABI") + } + event, exists := stakingABI.Events["ApproveShares"] + if !exists { + panic("ApproveShares event not found in stakingABI") + } return ApproveSharesABI{ - Method: stakingABI.Methods["approveShares"], - Event: stakingABI.Events["ApproveShares"], + Method: method, + Event: event, } }
Line range hint
113-121: Consider caching the IStakingFilterer instance.Creating a new filterer for each event unpacking could be inefficient. Consider caching the filterer instance as a field in the ABI struct since it's stateless.
type ApproveSharesABI struct { abi.Method abi.Event + filterer *fxcontract.IStakingFilterer } func NewApproveSharesABI() ApproveSharesABI { + filterer, err := fxcontract.NewIStakingFilterer(common.Address{}, nil) + if err != nil { + panic(fmt.Sprintf("failed to create staking filterer: %v", err)) + } return ApproveSharesABI{ Method: stakingABI.Methods["approveShares"], Event: stakingABI.Events["ApproveShares"], + filterer: filterer, } } 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) + return m.filterer.ParseApproveShares(*log) }x/staking/precompile/delegate.go (2)
72-82: Consider adding validation in NewDelegateV2ABI.While the implementation is clean, consider adding validation to ensure
stakingABIcontains the required "delegateV2" method and "DelegateV2" event to prevent potential runtime panics.func NewDelegateV2ABI() DelegateV2ABI { + method, exists := stakingABI.Methods["delegateV2"] + if !exists { + panic("delegateV2 method not found in stakingABI") + } + event, exists := stakingABI.Events["DelegateV2"] + if !exists { + panic("DelegateV2 event not found in stakingABI") + } return DelegateV2ABI{ - Method: stakingABI.Methods["delegateV2"], - Event: stakingABI.Events["DelegateV2"], + Method: method, + Event: event, } }
Line range hint
118-127: Consider caching the IStakingFilterer instance.Creating a new filterer for each event unpacking could be inefficient. Consider caching the filterer instance or moving it to a package-level variable since it's stateless.
+var stakingFilterer *fxcontract.IStakingFilterer + +func init() { + var err error + stakingFilterer, err = fxcontract.NewIStakingFilterer(common.Address{}, nil) + if err != nil { + panic(fmt.Sprintf("failed to create staking filterer: %v", err)) + } +} + func (m DelegateV2ABI) UnpackEvent(log *ethtypes.Log) (*fxcontract.IStakingDelegateV2, 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.ParseDelegateV2(*log) + return stakingFilterer.ParseDelegateV2(*log) }x/staking/precompile/withdraw.go (1)
Line range hint
123-133: Consider optimizing filterer creation.The current implementation creates a new filterer for each event unpacking. Consider caching the filterer instance as a field in WithdrawABI to improve performance, especially when processing multiple events.
type WithdrawABI struct { abi.Method abi.Event + filterer *fxcontract.IStakingFilterer } func NewWithdrawABI() WithdrawABI { + filterer, err := fxcontract.NewIStakingFilterer(common.Address{}, nil) + if err != nil { + panic(fmt.Sprintf("failed to create staking filterer: %v", err)) + } return WithdrawABI{ Method: stakingABI.Methods["withdraw"], Event: stakingABI.Events["Withdraw"], + filterer: filterer, } } func (m WithdrawABI) UnpackEvent(log *ethtypes.Log) (*fxcontract.IStakingWithdraw, 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.ParseWithdraw(*log) + return m.filterer.ParseWithdraw(*log) }x/staking/precompile/undelegate.go (1)
Line range hint
120-130: Enhance error message in UnpackEvent.The error message "empty log" could be more descriptive to help with debugging.
func (m UndelegateABI) UnpackEvent(log *ethtypes.Log) (*fxcontract.IStakingUndelegateV2, error) { if log == nil { - return nil, errors.New("empty log") + return nil, errors.New("cannot unpack UndelegateV2 event: nil log provided") } filterer, err := fxcontract.NewIStakingFilterer(common.Address{}, nil) if err != nil { return nil, err } return filterer.ParseUndelegateV2(*log) }x/staking/precompile/redelegate.go (1)
Line range hint
43-75: Consider enhancing error handling for event emission.While the implementation is solid, consider wrapping the event emission in a separate error check to ensure proper handling of event-related failures.
Here's a suggested improvement:
- data, topic, err := m.NewRedelegationEvent(contract.Caller(), args.ValidatorSrc, args.ValidatorDst, args.Amount, resp.CompletionTime.Unix()) - if err != nil { - return err - } - fxcontract.EmitEvent(evm, stakingAddress, data, topic) + if data, topic, err := m.NewRedelegationEvent(contract.Caller(), args.ValidatorSrc, args.ValidatorDst, args.Amount, resp.CompletionTime.Unix()); err != nil { + return fmt.Errorf("failed to create redelegation event: %w", err) + } else { + fxcontract.EmitEvent(evm, stakingAddress, data, topic) + }x/staking/precompile/validator_list_test.go (1)
116-122: LGTM! Consider extracting validator info retrieval logic.The changes look good - the new
Validatortype is more focused and the implementation correctly handles validator information retrieval and error checking.Consider extracting the validator info retrieval logic into a helper function for better readability:
+func getValidatorWithMissedBlocks(ctx sdk.Context, validator stakingtypes.Validator, sk types.StakingKeeper, slk types.SlashingKeeper) (precompile.Validator, error) { + consAddr, err := validator.GetConsAddr() + if err != nil { + return precompile.Validator{}, err + } + info, err := slk.GetValidatorSigningInfo(ctx, consAddr) + if err != nil { + return precompile.Validator{}, err + } + return precompile.Validator{ + ValAddr: validator.OperatorAddress, + MissedBlocks: info.MissedBlocksCounter, + }, nil +} valList := make([]precompile.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, precompile.Validator{ - ValAddr: validator.OperatorAddress, - MissedBlocks: info.MissedBlocksCounter, - }) + val, err := getValidatorWithMissedBlocks(suite.Ctx, validator, suite.App.StakingKeeper, suite.App.SlashingKeeper) + suite.Require().NoError(err) + valList = append(valList, val) }x/staking/precompile/transfer_shares.go (2)
Line range hint
216-225: InitializeIStakingFiltererwith Appropriate ParametersThe
UnpackEventfunction initializesIStakingFiltererwith a zero address and anilbackend, which may not be appropriate and could lead to errors when parsing events.Consider providing the correct contract address and a valid backend:
func (m transferShareABI) UnpackEvent(log *ethtypes.Log) (*fxcontract.IStakingTransferShares, error) { if log == nil { return nil, errors.New("empty log") } - filterer, err := fxcontract.NewIStakingFilterer(common.Address{}, nil) + filterer, err := fxcontract.NewIStakingFilterer(stakingAddress, evm.Backend) if err != nil { return nil, err } return filterer.ParseTransferShares(*log) }Ensure that
stakingAddressandevm.Backendare accessible in this context.
106-114: Ensure Consistent Struct Initialization Across MethodsFor consistency and readability, align the struct initialization in
NewTransferFromSharesMethodwith that inNewTransferSharesMethod.func NewTransferFromSharesMethod(keeper *Keeper) *TransferFromSharesMethod { return &TransferFromSharesMethod{ Keeper: keeper, - TransferFromSharesABI: NewTransferFromSharesABI(), + TransferFromSharesABI: NewTransferFromSharesABI(), } }Ensure that any additional fields or initializations are consistently ordered.
x/staking/precompile/keeper.go (3)
56-56: Improve error message for clarityThe error message
"from has receiving redelegation"might be unclear to users. Consider rephrasing it to provide more context, such as"cannot transfer shares: delegator has an ongoing redelegation to this validator".
60-62: Enhance error message for insufficient sharesThe error message
"insufficient shares(%s < %s)"could be made more user-friendly. Rephrase it to"insufficient shares: available %s, required %s"for better clarity.
256-262: Consider integratingvalidatorListPowerinto main logicThe
validatorListPowerfunction simply extracts operator addresses from validators. If it's only used once, consider integrating this logic directly where needed to reduce unnecessary abstraction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- x/staking/precompile/allowance_shares.go (2 hunks)
- x/staking/precompile/approve_shares.go (2 hunks)
- x/staking/precompile/contract_test.go (2 hunks)
- x/staking/precompile/delegate.go (2 hunks)
- x/staking/precompile/delegation.go (2 hunks)
- x/staking/precompile/delegation_rewards.go (2 hunks)
- x/staking/precompile/keeper.go (2 hunks)
- x/staking/precompile/redelegate.go (2 hunks)
- x/staking/precompile/redelegate_test.go (1 hunks)
- x/staking/precompile/slashing_info.go (2 hunks)
- x/staking/precompile/transfer_shares.go (6 hunks)
- x/staking/precompile/undelegate.go (2 hunks)
- x/staking/precompile/validator_list.go (3 hunks)
- x/staking/precompile/validator_list_test.go (1 hunks)
- x/staking/precompile/withdraw.go (2 hunks)
🔇 Additional comments (36)
x/staking/precompile/slashing_info.go (3)
15-15: LGTM! Good separation of concerns.The change to embed
SlashingABIinstead ofabi.Methodimproves code organization by isolating ABI-related functionality.
20-21: LGTM! Constructor properly initializes all fields.The initialization of both
KeeperandSlashingABIfields is correct and follows Go conventions.
Line range hint
63-98: Verify consistency with other precompile refactorings.The refactoring pattern looks good. Let's verify that similar changes have been applied consistently across other precompile files.
✅ Verification successful
Refactoring pattern is consistent across precompile files
The verification shows that the SlashingABI implementation follows the same pattern used consistently across all precompile files in the x/staking/precompile directory:
- All 11 precompile files define their respective
*ABIstructs- Each has a corresponding
New*ABI()constructor- All implement the same method signatures for
PackInput,UnpackInput,PackOutput, andUnpackOutput- The method implementations follow the same pattern of handling ABI encoding/decoding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if other precompile files follow the same pattern # Test: Look for similar ABI type definitions and method signatures # Check for other *ABI type definitions echo "Checking for other ABI type definitions..." rg -l "type \w+ABI struct" x/staking/precompile/ # Check for NewABI constructors echo "Checking for NewABI constructors..." rg -l "func New\w+ABI\(\)" x/staking/precompile/ # Check for Pack/Unpack method signatures on ABI types echo "Checking for Pack/Unpack methods..." rg "func \([a-z] \w+ABI\) (Pack|Unpack)(Input|Output)" x/staking/precompile/Length of output: 6800
x/staking/precompile/delegation.go (2)
17-17: LGTM! Clean separation of concernsThe refactoring to embed
DelegationABIimproves code organization by isolating ABI-related functionality while maintaining clean initialization in the constructor.Also applies to: 22-23
Line range hint
66-102: Verify complete migration to DelegationABILet's ensure all references to the old Method field have been updated to use DelegationABI.
x/staking/precompile/delegation_rewards.go (4)
17-17: LGTM! Good improvement in type safetyThe change from raw
abi.Methodto a dedicatedDelegationRewardsABItype improves type safety while maintaining the same functionality. The constructor properly initializes the new field.Also applies to: 22-23
72-74: LGTM! Clean type definitionThe embedding of
abi.Methodfollows Go best practices and provides a good foundation for ABI-specific functionality.
Line range hint
82-105: LGTM! Well-structured ABI methodsThe implementation is solid with:
- Appropriate use of value receivers
- Consistent error handling
- Clean integration with the system's type parsing
76-80: Verify stakingABI initializationThe constructor looks good, but let's verify that
stakingABIis properly initialized elsewhere in the package.✅ Verification successful
stakingABIis properly initialized and used consistentlyThe variable
stakingABIis correctly initialized inx/staking/precompile/contract.goas a package-level variable:var ( stakingAddress = common.HexToAddress(contract.StakingAddress) stakingABI = contract.MustABIJson(contract.IStakingMetaData.ABI) )It's used consistently across multiple ABI constructors in the package for different staking operations (delegate, undelegate, rewards, etc.).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for stakingABI initialization rg -l "var\s+stakingABI\s*=" rg -l "stakingABI\s*:?=\s*abi\.JSON"Length of output: 69
Script:
#!/bin/bash # Let's try a broader search for stakingABI rg "stakingABI" -A 3 -B 3 # Also search for any abi.JSON initialization patterns rg "abi\.JSON" -A 3 -B 3 # Look for files that might contain ABI definitions fd "abi.*\.go$"Length of output: 6085
x/staking/precompile/approve_shares.go (2)
20-20: LGTM! Good separation of concerns.The refactoring improves code organization by moving ABI-related functionality to a dedicated type while maintaining clean initialization in the constructor.
Also applies to: 25-26
67-70: Verify consistent ABI type usage across the codebase.Let's verify that other staking precompile methods follow the same pattern of separating ABI functionality.
✅ Verification successful
Let me generate another script to search more specifically for the ABI usage pattern in the staking precompile files:
Based on the search results, I can now provide a final response:
ABI struct definition follows consistent pattern across staking precompiles
The
ApproveSharesABIstruct follows the same pattern used consistently across all staking precompile methods:
- All ABI structs embed
abi.Method- Those that emit events also embed
abi.Event- Each ABI struct has a corresponding
New*ABI()constructor function- Other precompile methods like
WithdrawABI,DelegateV2ABI,RedelegateABI, etc. follow the exact same pattern🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for consistent ABI type usage across staking precompile files # Expected: All precompile methods should have their own ABI type # Test: Search for other *ABI struct definitions echo "Checking for ABI type definitions:" ast-grep --pattern 'type $_ABI struct { $$$ }' # Test: Search for corresponding method structs using these ABI types echo -e "\nChecking for method structs using ABI types:" ast-grep --pattern 'type $_Method struct { $$$ $_ABI $$$ }'Length of output: 510945
Script:
#!/bin/bash # Search specifically in staking precompile files for ABI usage patterns echo "Checking staking precompile files for ABI usage:" rg -l "ABI" x/staking/precompile/ echo -e "\nChecking ABI struct definitions and usage in staking precompile files:" rg -A 5 "type.*ABI struct" x/staking/precompile/Length of output: 5664
x/staking/precompile/delegate.go (1)
20-28: LGTM! Well-structured refactoring of DelegateV2Method.The restructuring improves encapsulation by grouping ABI-related fields into a dedicated type while maintaining clean initialization in the constructor.
x/staking/precompile/withdraw.go (4)
21-21: LGTM! Good encapsulation of ABI-related fields.The consolidation of ABI-related fields into a single
WithdrawABItype improves code organization and follows the single responsibility principle.
26-27: LGTM! Clean constructor initialization.The constructor properly initializes both the keeper and the new WithdrawABI field while maintaining backward compatibility.
89-121: LGTM! Well-structured ABI method implementations.The ABI-related methods are properly implemented with appropriate error handling and type safety.
77-87: Consider adding safety checks in NewWithdrawABI.While the implementation is clean, consider adding validation to ensure
stakingABIis properly initialized before accessing its methods and events.Consider adding error handling:
func NewWithdrawABI() WithdrawABI { + method, exists := stakingABI.Methods["withdraw"] + if !exists { + panic("withdraw method not found in stakingABI") + } + event, exists := stakingABI.Events["Withdraw"] + if !exists { + panic("Withdraw event not found in stakingABI") + } return WithdrawABI{ - Method: stakingABI.Methods["withdraw"], - Event: stakingABI.Events["Withdraw"], + Method: method, + Event: event, } }x/staking/precompile/undelegate.go (3)
21-21: LGTM! Good refactoring of ABI-related fields.The consolidation of ABI-related fields into a single
UndelegateABItype improves encapsulation and follows the Single Responsibility Principle.
26-27: LGTM! Constructor properly initializes the new structure.The constructor correctly initializes both the Keeper and UndelegateABI fields, maintaining a clean initialization pattern.
82-92: Consider adding validation in NewUndelegateV2ABI.While the implementation is good, consider adding validation to ensure
stakingABIis properly initialized and the required method and event exist.Example validation:
func NewUndelegateV2ABI() UndelegateABI { + method, exists := stakingABI.Methods["undelegateV2"] + if !exists { + panic("undelegateV2 method not found in stakingABI") + } + event, exists := stakingABI.Events["UndelegateV2"] + if !exists { + panic("UndelegateV2 event not found in stakingABI") + } return UndelegateABI{ - Method: stakingABI.Methods["undelegateV2"], - Event: stakingABI.Events["UndelegateV2"], + Method: method, + Event: event, } }x/staking/precompile/redelegate.go (4)
19-21: LGTM! Clean struct definition and constructor implementation.The struct and constructor follow Go best practices with proper field initialization and clear separation of concerns.
Also applies to: 24-28
31-41: LGTM! Clear and concise helper method implementations.The helper methods are well-implemented with appropriate return values and gas requirements.
77-87: LGTM! Well-structured ABI implementation.The RedelegateABI struct and its constructor are well-implemented with proper initialization from stakingABI.
Line range hint
19-132: Verify integration with existing staking operations.The refactoring looks solid, but let's verify the integration with existing staking operations.
✅ Verification successful
Based on the search results, I can now provide a final response since we have comprehensive information about the redelegation functionality across the codebase:
Integration with existing staking operations is properly maintained.
The code shows proper integration with the staking system:
- The precompile correctly interfaces with the underlying staking module through
BeginRedelegatecalls- Event handling and logging are properly implemented
- Test coverage exists across multiple scenarios including contract interactions
- The implementation is consistent with other staking precompiles in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct usage of the old RedelegateMethodV2 type rg "RedelegateMethodV2" -A 3 # Search for any existing redelegation event handlers ast-grep --pattern 'func $_(log $_, $_, $_) { $$$ ParseRedelegateV2($$$) $$$ }'Length of output: 116
Script:
#!/bin/bash # Let's try a broader search for redelegate-related code rg -i "redelegate" --type go -A 3 # Search for staking precompile registration rg "RegisterPrecompile.*staking" --type go -A 5 # Look for any test files related to redelegation fd "test.*go" --exec grep -l "redelegate" {}Length of output: 119396
x/staking/precompile/redelegate_test.go (1)
116-116: Method renaming looks good, verify consistency.The renaming from
redelegateMethodV2toredelegateV2Methodaligns with the PR's refactoring objectives. The test logic remains unchanged.Let's verify the consistency of this naming pattern across the codebase:
✅ Verification successful
Method renaming is consistent across the codebase
Based on the search results:
- No instances of the old method name
redelegateMethodV2were found, confirming complete renaming- The new naming pattern
redelegateV2Methodis consistent with other method names in the codebase- All method references follow a consistent pattern of accessing properties via
Method.notation🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of old method name and verify consistent naming pattern # Test 1: Search for any remaining instances of the old method name echo "Checking for any remaining instances of old method name..." rg "redelegateMethodV2" # Test 2: Verify the naming pattern consistency for other similar methods echo "Verifying naming pattern consistency..." rg "Method\." --type goLength of output: 17385
x/staking/precompile/contract_test.go (2)
45-48: LGTM! Consistent method naming in PrecompileTestSuite struct.The renaming of fields maintains consistency with the broader refactoring changes, improving code clarity without affecting functionality.
88-88: LGTM! Consistent initialization in SetupTest.The initialization of
redelegateV2Methodcorrectly aligns with the renamed struct field, maintaining test coverage.x/staking/precompile/validator_list.go (5)
13-13: EmbeddingValidatorListABIinValidatorListMethodIncluding
ValidatorListABIwithinValidatorListMethodappropriately encapsulates the ABI-related functionality, enhancing the modularity of the code.
18-19: Proper initialization ofValidatorListABIIn the
NewValidatorListMethodconstructor,ValidatorListABIis correctly initialized usingNewValidatorListABI(), ensuring that all necessary ABI configurations are set up.
63-69: Well-definedValidatorListABIstruct and constructorThe introduction of the
ValidatorListABIstruct and its constructorNewValidatorListABIeffectively encapsulates the ABI method details, promoting code clarity and separation of concerns.
87-87: Consistent error handling inPackOutputEnsure that the
PackOutputmethod properly handles errors that may arise fromm.Method.Outputs.Pack(valList). Currently, any error would be returned directly, but consider wrapping it with additional context if necessary.
52-52: Verify the accessibility ofvalidatorListPowerfunctionThe function
validatorListPoweris called here. Please ensure that this function is defined within the package and is accessible. If it's defined externally, make sure to import the necessary package.Run the following script to check if
validatorListPoweris defined and accessible:✅ Verification successful
Let me verify if the function is in the same package by checking the package declaration.
Function
validatorListPoweris properly defined and accessibleThe function
validatorListPoweris defined inx/staking/precompile/keeper.goand both files are in the sameprecompilepackage, making the function accessible withinvalidator_list.go. No changes are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the definition and accessibility of `validatorListPower` function. # Test: Search for the function definition. Expect: Function should be found within the codebase. rg '^func\s+validatorListPower\b' -t goLength of output: 144
Script:
#!/bin/bash # Check package declarations for both files echo "=== Package declaration in validator_list.go ===" rg -A 1 "^package" x/staking/precompile/validator_list.go echo -e "\n=== Package declaration in keeper.go ===" rg -A 1 "^package" x/staking/precompile/keeper.goLength of output: 355
x/staking/precompile/transfer_shares.go (2)
86-87: Verify Correctness of ABI Method and Event ReferencesIn
NewTransferSharesABI, ensure that the method and event names"transferShares"and"TransferShares"exist instakingABI.MethodsandstakingABI.Eventsrespectively. Incorrect references could lead to runtime errors.
179-180: Verify Correctness of ABI Method and Event ReferencesIn
NewTransferFromSharesABI, confirm that the method"transferFromShares"and the event"TransferShares"are correctly defined instakingABI.MethodsandstakingABI.Events. Mismatches here could cause issues during ABI interactions.x/staking/precompile/keeper.go (3)
230-254: EnsureValidatorListMissedBlockhandles empty validator sets gracefullyIn the
ValidatorListMissedBlockfunction, ifbondedValsis empty, the function may proceed without returning an appropriate response. Verify that the function handles empty inputs and returns an empty list or an informative error as needed.
65-69:⚠️ Potential issueVerify correct usage of
withdrawAddrfor balance calculationsThe
withdrawAddris obtained for thetoaddress, but the reward withdrawal is performed for thefromdelegator. This may lead to incorrect balance calculations when comparingbeforeDelBalanceandafterDelBalance. Ensure that thewithdrawAddrcorresponds to thefromdelegator when calculating balances related tofrom.
202-204:⚠️ Potential issueCorrect condition and error message in
incrementReferenceCountThe condition
historical.ReferenceCount > 2allows theReferenceCountto reach 2, which might exceed the maximum allowed value. If 2 is the maximum, consider changing the condition to>= 2. Additionally, clarify the error message for better understanding.Apply this diff to adjust the condition and error message:
-if historical.ReferenceCount > 2 { - return errors.New("reference count should never exceed 2") +if historical.ReferenceCount >= 2 { + return errors.New("reference count cannot exceed 1") }Likely invalid or redundant comment.
| func (m DelegateV2ABI) UnpackOutput(data []byte) (bool, error) { | ||
| amount, err := m.Method.Outputs.Unpack(data) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| return amount[0].(bool), nil | ||
| } |
There was a problem hiding this comment.
Add type assertion safety check in UnpackOutput.
The type assertion amount[0].(bool) could panic if the unpacked value is not a boolean. Consider adding a type safety check.
func (m DelegateV2ABI) 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("empty output data")
+ }
+ val, ok := amount[0].(bool)
+ if !ok {
+ return false, errors.New("invalid output type: expected bool")
+ }
- return amount[0].(bool), nil
+ return val, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m DelegateV2ABI) UnpackOutput(data []byte) (bool, error) { | |
| amount, err := m.Method.Outputs.Unpack(data) | |
| if err != nil { | |
| return false, err | |
| } | |
| return amount[0].(bool), nil | |
| } | |
| func (m DelegateV2ABI) 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("empty output data") | |
| } | |
| val, ok := amount[0].(bool) | |
| if !ok { | |
| return false, errors.New("invalid output type: expected bool") | |
| } | |
| return val, nil | |
| } |
| func (m RedelegateABI) UnpackInput(data []byte) (*fxstakingtypes.RedelegateV2Args, error) { | ||
| args := new(fxstakingtypes.RedelegateV2Args) | ||
| err := types.ParseMethodArgs(m.Method, args, data[4:]) | ||
| return args, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding input validation in UnpackInput.
The UnpackInput method could benefit from additional validation before parsing the method arguments.
Here's a suggested improvement:
func (m RedelegateABI) UnpackInput(data []byte) (*fxstakingtypes.RedelegateV2Args, error) {
+ if len(data) <= 4 {
+ return nil, errors.New("invalid input data length")
+ }
args := new(fxstakingtypes.RedelegateV2Args)
err := types.ParseMethodArgs(m.Method, args, data[4:])
return args, err
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m RedelegateABI) UnpackInput(data []byte) (*fxstakingtypes.RedelegateV2Args, error) { | |
| args := new(fxstakingtypes.RedelegateV2Args) | |
| err := types.ParseMethodArgs(m.Method, args, data[4:]) | |
| return args, err | |
| } | |
| func (m RedelegateABI) UnpackInput(data []byte) (*fxstakingtypes.RedelegateV2Args, error) { | |
| if len(data) <= 4 { | |
| return nil, errors.New("invalid input data length") | |
| } | |
| args := new(fxstakingtypes.RedelegateV2Args) | |
| err := types.ParseMethodArgs(m.Method, args, data[4:]) | |
| return args, err | |
| } |
| func (m AllowanceSharesABI) PackInput(args fxstakingtypes.AllowanceSharesArgs) ([]byte, error) { | ||
| arguments, err := m.Method.Inputs.Pack(args.Validator, args.Owner, args.Spender) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return append(m.GetMethodId(), arguments...), nil | ||
| return append(m.Method.ID, arguments...), nil | ||
| } | ||
|
|
||
| func (m *AllowanceSharesMethod) UnpackInput(data []byte) (*fxstakingtypes.AllowanceSharesArgs, error) { | ||
| func (m AllowanceSharesABI) UnpackInput(data []byte) (*fxstakingtypes.AllowanceSharesArgs, error) { | ||
| args := new(fxstakingtypes.AllowanceSharesArgs) | ||
| err := types.ParseMethodArgs(m.Method, args, data[4:]) | ||
| return args, err | ||
| } | ||
|
|
||
| func (m *AllowanceSharesMethod) PackOutput(amount *big.Int) ([]byte, error) { | ||
| func (m AllowanceSharesABI) PackOutput(amount *big.Int) ([]byte, error) { | ||
| return m.Method.Outputs.Pack(amount) | ||
| } | ||
|
|
||
| func (m *AllowanceSharesMethod) UnpackOutput(data []byte) (*big.Int, error) { | ||
| func (m AllowanceSharesABI) UnpackOutput(data []byte) (*big.Int, error) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using pointer receivers to avoid unnecessary copying
Currently, the methods of AllowanceSharesABI use value receivers. Since abi.Method may be a sizable struct, using pointer receivers can improve performance by preventing unnecessary copying of the struct when methods are called.
Apply this diff to change the method receivers to pointers:
-func (m AllowanceSharesABI) PackInput(args fxstakingtypes.AllowanceSharesArgs) ([]byte, error) {
+func (m *AllowanceSharesABI) PackInput(args fxstakingtypes.AllowanceSharesArgs) ([]byte, error) {
-func (m AllowanceSharesABI) UnpackInput(data []byte) (*fxstakingtypes.AllowanceSharesArgs, error) {
+func (m *AllowanceSharesABI) UnpackInput(data []byte) (*fxstakingtypes.AllowanceSharesArgs, error) {
-func (m AllowanceSharesABI) PackOutput(amount *big.Int) ([]byte, error) {
+func (m *AllowanceSharesABI) PackOutput(amount *big.Int) ([]byte, error) {
-func (m AllowanceSharesABI) UnpackOutput(data []byte) (*big.Int, error) {
+func (m *AllowanceSharesABI) UnpackOutput(data []byte) (*big.Int, error) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (m AllowanceSharesABI) PackInput(args fxstakingtypes.AllowanceSharesArgs) ([]byte, error) { | |
| arguments, err := m.Method.Inputs.Pack(args.Validator, args.Owner, args.Spender) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return append(m.GetMethodId(), arguments...), nil | |
| return append(m.Method.ID, arguments...), nil | |
| } | |
| func (m *AllowanceSharesMethod) UnpackInput(data []byte) (*fxstakingtypes.AllowanceSharesArgs, error) { | |
| func (m AllowanceSharesABI) UnpackInput(data []byte) (*fxstakingtypes.AllowanceSharesArgs, error) { | |
| args := new(fxstakingtypes.AllowanceSharesArgs) | |
| err := types.ParseMethodArgs(m.Method, args, data[4:]) | |
| return args, err | |
| } | |
| func (m *AllowanceSharesMethod) PackOutput(amount *big.Int) ([]byte, error) { | |
| func (m AllowanceSharesABI) PackOutput(amount *big.Int) ([]byte, error) { | |
| return m.Method.Outputs.Pack(amount) | |
| } | |
| func (m *AllowanceSharesMethod) UnpackOutput(data []byte) (*big.Int, error) { | |
| func (m AllowanceSharesABI) UnpackOutput(data []byte) (*big.Int, error) { | |
| func (m *AllowanceSharesABI) PackInput(args fxstakingtypes.AllowanceSharesArgs) ([]byte, error) { | |
| arguments, err := m.Method.Inputs.Pack(args.Validator, args.Owner, args.Spender) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return append(m.Method.ID, arguments...), nil | |
| } | |
| func (m *AllowanceSharesABI) UnpackInput(data []byte) (*fxstakingtypes.AllowanceSharesArgs, error) { | |
| args := new(fxstakingtypes.AllowanceSharesArgs) | |
| err := types.ParseMethodArgs(m.Method, args, data[4:]) | |
| return args, err | |
| } | |
| func (m *AllowanceSharesABI) PackOutput(amount *big.Int) ([]byte, error) { | |
| return m.Method.Outputs.Pack(amount) | |
| } | |
| func (m *AllowanceSharesABI) UnpackOutput(data []byte) (*big.Int, error) { |
| return m.Method.Outputs.Pack(valList) | ||
| } | ||
|
|
||
| func (m *ValidatorListMethod) UnpackOutput(data []byte) ([]string, error) { | ||
| func (m ValidatorListABI) UnpackOutput(data []byte) ([]string, error) { |
There was a problem hiding this comment.
Type assertion in UnpackOutput should be checked
In UnpackOutput, there's an implicit assumption that unpack[0] will be of type []string. To prevent panics due to type assertion failures, consider adding type checking.
Apply this diff to safely assert the type:
func (m *ValidatorListABI) UnpackOutput(data []byte) ([]string, error) {
unpack, err := m.Method.Outputs.Unpack(data)
if err != nil {
return nil, err
}
- return unpack[0].([]string), nil
+ valList, ok := unpack[0].([]string)
+ if !ok {
+ return nil, fmt.Errorf("unexpected output type, expected []string")
+ }
+ return valList, nil
}Committable suggestion was skipped due to low confidence.
| func (m ValidatorListABI) PackInput(args fxstakingtypes.ValidatorListArgs) ([]byte, error) { | ||
| arguments, err := m.Method.Inputs.Pack(args.SortBy) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return append(m.GetMethodId(), arguments...), nil | ||
| return append(m.Method.ID, arguments...), nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using pointer receivers for methods
The methods associated with ValidatorListABI use value receivers. For consistency and to avoid unnecessary copying of the struct (especially if it grows in size), consider using pointer receivers.
Apply this diff to change the method receivers:
-func (m ValidatorListABI) PackInput(args fxstakingtypes.ValidatorListArgs) ([]byte, error) {
+func (m *ValidatorListABI) PackInput(args fxstakingtypes.ValidatorListArgs) ([]byte, error) {
...
-func (m ValidatorListABI) UnpackInput(data []byte) (*fxstakingtypes.ValidatorListArgs, error) {
+func (m *ValidatorListABI) UnpackInput(data []byte) (*fxstakingtypes.ValidatorListArgs, error) {Update the receiver for all methods of ValidatorListABI accordingly.
Committable suggestion was skipped due to low confidence.
| type TransferSharesMethod struct { | ||
| *Keeper | ||
| abi.Method | ||
| abi.Event | ||
| } | ||
|
|
||
| type TransferShares struct { | ||
| *TransferShare | ||
| TransferSharesABI |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider Embedding Keeper as a Value Receiver Instead of a Pointer
Embedding *Keeper as a pointer may lead to unintended side effects if the Keeper instance is modified elsewhere. Embedding it as a value could enhance safety by preventing unexpected mutations within the TransferSharesMethod struct.
type TransferSharesMethod struct {
- *Keeper
+ Keeper
TransferSharesABI
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type TransferSharesMethod struct { | |
| *Keeper | |
| abi.Method | |
| abi.Event | |
| } | |
| type TransferShares struct { | |
| *TransferShare | |
| TransferSharesABI | |
| type TransferSharesMethod struct { | |
| Keeper | |
| TransferSharesABI |
| type Validator struct { | ||
| ValAddr string | ||
| MissedBlocks int64 | ||
| } |
There was a problem hiding this comment.
Rename Validator struct to avoid naming conflicts
The Validator struct defined here may conflict with the existing Validator type from the stakingtypes package. To prevent confusion and potential type collisions, consider renaming this struct to something more specific, such as MissedBlocksValidator or ValidatorMissedBlocks.
| //nolint:gocyclo // need 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) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor handlerTransferShares to reduce cyclomatic complexity
The handlerTransferShares function is quite complex, as indicated by the //nolint:gocyclo comment. Refactoring this function into smaller, modular helper functions would improve readability and maintainability.
| return valList[i].MissedBlocks > valList[j].MissedBlocks | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize sorting function for better performance
The sorting function compares MissedBlocks in descending order. For large validator lists, consider using efficient sorting algorithms or built-in functions optimized for performance.
| if historical.ReferenceCount == 0 { | ||
| panic("cannot set negative reference count") | ||
| } |
There was a problem hiding this comment.
Avoid using panic; return an error instead
Using panic in production code can cause the application to crash unexpectedly. Instead of panicking when the ReferenceCount is zero, consider returning an error to handle the situation gracefully.
Apply this diff to return an error:
if historical.ReferenceCount == 0 {
- panic("cannot set negative reference count")
+ return errors.New("reference count cannot be negative")
}Committable suggestion was skipped due to low confidence.
Summary by CodeRabbit
New Features
AllowanceSharesABI,ApproveSharesABI,DelegateV2ABI, and others.Bug Fixes
Refactor
Tests