test: add crosschain and observer operations #3207
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
kingpinXD
changed the title
kingpinXD
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3207 +/- ##
===========================================
+ Coverage 61.76% 61.81% +0.05%
===========================================
Files 428 430 +2
Lines 30791 30825 +34
===========================================
+ Hits 19018 19056 +38
+ Misses 10914 10912 -2
+ Partials 859 857 -2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 32
🧹 Outside diff range and nitpick comments (57)
pkg/coin/coint_test.go (1)
9-28: Test coverage looks good, consider adding edge casesThe table-driven test approach is well-structured and covers the main coin types. Consider adding edge cases:
- Negative coin type values
- Maximum int32 boundary values
tests := []struct { name string c coin.CoinType want bool }{ {"ERC20", coin.CoinType_ERC20, true}, {"Gas", coin.CoinType_Gas, true}, {"Zeta", coin.CoinType_Zeta, true}, {"Cmd", coin.CoinType_Cmd, false}, {"Unknown", coin.CoinType(100), false}, + {"Negative", coin.CoinType(-1), false}, + {"MaxInt32", coin.CoinType(2147483647), false}, }pkg/coin/coin.go (1)
38-40: Add documentation and consider optimization for future scalabilityThe implementation is correct but could benefit from:
- Adding documentation to explain the method's purpose and supported coin types
- Consider using a map for O(1) lookup if more coin types will be added in the future
+// SupportsRefund returns true if the coin type supports refund operations. +// Currently supported types are ERC20, Gas, and Zeta. func (c CoinType) SupportsRefund() bool { return c == CoinType_ERC20 || c == CoinType_Gas || c == CoinType_Zeta }x/crosschain/keeper/refund.go (3)
30-30: Improve error message consistencyThe error message format has been improved, but consider using a consistent error creation pattern across the codebase.
-return fmt.Errorf("unsupported coin type for refund on ZetaChain : %s", coinType) +return errorsmod.Wrapf(types.ErrUnsupportedCoinType, "coin type %s does not support refund on ZetaChain", coinType)
Line range hint
41-53: Simplify error handling in RefundAmountOnZetaChainGasConsider consolidating the error checks and using consistent error wrapping.
func (k Keeper) RefundAmountOnZetaChainGas( ctx sdk.Context, cctx types.CrossChainTx, refundAddress ethcommon.Address, ) error { refundAmount := GetAbortedAmount(cctx) - if refundAmount.IsNil() || refundAmount.IsZero() { - return errors.New("no amount to refund") + if refundAmount.IsNil() || refundAmount.IsZero() { + return errorsmod.Wrap(types.ErrInvalidAmount, "no amount to refund") } chainID := cctx.InboundParams.SenderChainId fcSenderChain, found := k.fungibleKeeper.GetGasCoinForForeignCoin(ctx, chainID) if !found { return types.ErrForeignCoinNotFound } zrc20 := ethcommon.HexToAddress(fcSenderChain.Zrc20ContractAddress) if zrc20 == (ethcommon.Address{}) { return errorsmod.Wrapf(types.ErrForeignCoinNotFound, "zrc20 contract address not found for chain %d", chainID) } - if _, err := k.fungibleKeeper.DepositZRC20(ctx, zrc20, refundAddress, refundAmount.BigInt()); err != nil { - return errors.New("failed to refund zeta on ZetaChain" + err.Error()) + if _, err := k.fungibleKeeper.DepositZRC20(ctx, zrc20, refundAddress, refundAmount.BigInt()); err != nil { + return errorsmod.Wrapf(err, "failed to refund zeta on ZetaChain") } return nil }
Line range hint
89-124: Improve error handling consistency in RefundAmountOnZetaChainERC20The error handling patterns vary throughout the method. Consider standardizing the approach.
func (k Keeper) RefundAmountOnZetaChainERC20( ctx sdk.Context, cctx types.CrossChainTx, refundAddress ethcommon.Address, ) error { refundAmount := GetAbortedAmount(cctx) // preliminary checks if cctx.InboundParams.CoinType != coin.CoinType_ERC20 { - return errors.New("unsupported coin type for refund on ZetaChain") + return errorsmod.Wrapf(types.ErrUnsupportedCoinType, "expected ERC20, got %s", cctx.InboundParams.CoinType) } if !chains.IsEVMChain(cctx.InboundParams.SenderChainId, k.GetAuthorityKeeper().GetAdditionalChainList(ctx)) { - return errors.New("only EVM chains are supported for refund on ZetaChain") + return errorsmod.Wrapf(types.ErrUnsupportedChain, "chain %d is not an EVM chain", cctx.InboundParams.SenderChainId) } if refundAmount.IsNil() || refundAmount.IsZero() { - return errors.New("no amount to refund") + return errorsmod.Wrap(types.ErrInvalidAmount, "no amount to refund") } fc, found := k.fungibleKeeper.GetForeignCoinFromAsset( ctx, cctx.InboundParams.Asset, cctx.InboundParams.SenderChainId, ) if !found { - return fmt.Errorf("asset %s zrc not found", cctx.InboundParams.Asset) + return errorsmod.Wrapf(types.ErrForeignCoinNotFound, "asset %s zrc not found", cctx.InboundParams.Asset) } zrc20 := ethcommon.HexToAddress(fc.Zrc20ContractAddress) if zrc20 == (ethcommon.Address{}) { - return fmt.Errorf("asset %s invalid zrc address", cctx.InboundParams.Asset) + return errorsmod.Wrapf(types.ErrInvalidAddress, "invalid zrc address for asset %s", cctx.InboundParams.Asset) } if _, err := k.fungibleKeeper.DepositZRC20(ctx, zrc20, refundAddress, refundAmount.BigInt()); err != nil { - return errors.New("failed to deposit zrc20 on ZetaChain" + err.Error()) + return errorsmod.Wrapf(err, "failed to deposit zrc20 on ZetaChain") } return nil }x/fungible/simulation/operations.go (2)
19-21: Improve documentation formatting and clarityThe comment explaining operation weights is informative but could be more concise and better formatted. Consider restructuring it to avoid duplication and improve readability.
-// Operation weights are used by the simulation program to simulate the weight of different operations. -// This decides what percentage of a certain type of operation is part of a block. -// Based on the weights assigned in the cosmos sdk modules , 100 seems to the max weight used , and therefore guarantees that at least one operation of that type is present in a block. +// Operation weights determine the probability of different operations being included in a simulated block. +// The weight value (max 100) represents the relative frequency of the operation's occurrence. +// Higher weights increase the likelihood of the operation being selected during simulation.
Line range hint
35-52: Consider enhancing simulation coverageThe function currently only includes the system contract deployment operation. Consider adding more weighted operations to improve simulation coverage, especially for the new crosschain and observer operations mentioned in the PR objectives.
Additionally, consider adding godoc comments to explain the function parameters and return values:
+// WeightedOperations returns a list of weighted operations for the fungible module simulation. +// Parameters: +// - appParams: Application parameters for generating weights +// - cdc: JSON codec for parameter encoding/decoding +// - k: Fungible module keeper +// Returns: List of weighted operations for simulation func WeightedOperations( appParams simtypes.AppParams, cdc codec.JSONCodec, k keeper.Keeper, ) simulation.WeightedOperations {x/observer/simulation/operation_reset_chain_nonces.go (3)
29-38: Include original error in returned error for better debuggingWhen handling the error from
GetExternalChain, the current implementation returns a generic error message. Including the original error message in the returned error provides more context and aids in debugging.Apply this diff to include the original error:
), nil, fmt.Errorf( - "error getting external chain", + "error getting external chain: %w", + err, )
40-49: Enhance error messages for clarity and debuggingIn the error handling for
GetTSS, the same error message "TSS not found" is used twice, which is redundant. Consider providing a more detailed error message or including additional context to aid in troubleshooting.For example:
), nil, fmt.Errorf( - "TSS not found", + "TSS not found in the context", )
50-54: Ensure error messages are consistent and informativeWhen
GetPendingNoncesfails to find pending nonces, the error message inNoOpMsgis less informative than the formatted error. For consistency and better debugging, include detailed information in both.Apply this change:
return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgResetChainNonces, - "Pending nonces not found"), nil, + fmt.Sprintf("Pending nonces not found for chain %d %s", randomChain.ChainId, randomChain.ChainName)), nil, fmt.Errorf("pending nonces not found for chain %d %s", randomChain.ChainId, randomChain.ChainName)x/observer/simulation/operation_add_observer.go (2)
84-84: Remove unnecessary nil check forerrAt line 84, there is a check for
errthat is not preceded by any operation that could produce an error. This nil check is redundant and should be removed to clean up the code.Apply this change:
- if err != nil { - return simtypes.NoOpMsg(types.ModuleName, msg.Type(), err.Error()), nil, err - }
46-53: Simplify theRepeatCheckfunction usageThe
RepeatCheckfunction could be optimized for better readability by using a loop construct directly, enhancing code clarity.Consider refactoring as follows:
for attempts := 0; attempts < MaxAttempts; attempts++ { newObserver = nodeAccounts[r.Intn(len(nodeAccounts))].Operator if !observerMap[newObserver] { break } } // Check if a new observer was found after looping if observerMap[newObserver] { // Handle the case where no new observer was found }x/observer/simulation/operation_add_observer_node_account.go (2)
84-84: Remove redundant nil error checkThe error check at line 84 is unnecessary since no error is returned from any function in the preceding code block. Removing this check improves code readability.
Apply this change:
- if err != nil { - return simtypes.NoOpMsg(types.ModuleName, msg.Type(), err.Error()), nil, err - }
52-67: Enhance observer selection logic for efficiencyThe
RepeatCheckfunction could be replaced with a more efficient loop to select a new observer, improving performance and code clarity.Consider refactoring as:
for attempts := 0; attempts < MaxAttempts; attempts++ { randomValidator := validators[r.Intn(len(validators))] randomValidatorAddress, err := types.GetAccAddressFromOperatorAddress(randomValidator.OperatorAddress) if err != nil { continue } newObserver = randomValidatorAddress.String() if err = k.IsValidator(ctx, newObserver); err != nil { continue } if !observerMap[newObserver] { break } } // Check if a new observer was found after looping if observerMap[newObserver] || newObserver == "" { // Handle the case where no new observer was found }x/observer/simulation/operation_update_observer.go (3)
96-96: Remove unnecessary nil check forerrAt line 96, there is a check for
errwhen no new error has been assigned. This check is redundant and should be removed to maintain code cleanliness.Apply this change:
- if err != nil { - return simtypes.NoOpMsg(types.ModuleName, msg.Type(), err.Error()), nil, err - }
47-63: Improve observer update logic for better efficiencyThe use of
RepeatCheckcan be replaced with a direct loop, enhancing code readability and potentially improving performance.Refactor as follows:
for attempts := 0; attempts < MaxAttempts; attempts++ { randomValidator := validators[r.Intn(len(validators))] randomValidatorAddress, err := types.GetAccAddressFromOperatorAddress(randomValidator.OperatorAddress) if err != nil { continue } newObserver = randomValidatorAddress.String() if err = k.IsValidator(ctx, newObserver); err != nil { continue } if !observerMap[newObserver] { break } } // Check if a new observer was found after looping if observerMap[newObserver] || newObserver == "" { // Handle the case where no new observer was found }
73-87: Consolidate observer count checks for clarityThe checks for
GetLastObserverCountand the comparison withobserverListlength can be consolidated to improve code readability.Consider refactoring as:
if lastBlockCount, found := k.GetLastObserverCount(ctx); !found || int(lastBlockCount.Count) != len(observerList) { return simtypes.NoOpMsg( types.ModuleName, types.TypeMsgUpdateObserver, "observer count mismatch or last block count not found", ), nil, nil }x/crosschain/simulation/operation_update_tss_address.go (1)
65-68: Remove commented-out code to improve code clarityThe block of commented-out code is unnecessary and can be removed to enhance readability and maintain a clean codebase. Leaving large blocks of commented code can be confusing and is generally discouraged in production code.
Apply this diff to remove the commented code:
- //index := ethcrypto.Keccak256Hash([]byte(fmt.Sprintf("%d", r.Int63()))).Hex() - //cctx := types.CrossChainTx{Index: index, - // CctxStatus: &types.Status{Status: types.CctxStatus_OutboundMined}}x/crosschain/simulation/operation_whitelist_erc20.go (1)
116-120: Correct the error message inValidateBasicfailureThe error message incorrectly references
"MsgRemoveOutboundTracker"instead of"MsgWhitelistERC20". Update the message for accuracy.Apply this diff to correct the error message:
err = msg.ValidateBasic() if err != nil { return simtypes.NoOpMsg( types.ModuleName, msg.Type(), - "unable to validate MsgRemoveOutboundTracker msg", + "unable to validate MsgWhitelistERC20 msg", ), nil, err }x/crosschain/simulation/operation_add_outbound_tracker.go (1)
75-83: Simplify and ensure correctness of random nonce selectionThe logic for selecting a random nonce is complex and may lead to issues with edge cases. Simplify the nonce selection for better readability and reliability.
You can streamline the logic as follows:
// Pick a random pending nonce -nonce := 0 -switch { -case pendingNonces.NonceHigh <= 1: - nonce = int(pendingNonces.NonceLow) -case pendingNonces.NonceLow == 0: - nonce = r.Intn(int(pendingNonces.NonceHigh)) -default: - nonce = r.Intn(int(pendingNonces.NonceHigh)-int(pendingNonces.NonceLow)) + int(pendingNonces.NonceLow) -} +nonceRange := int(pendingNonces.NonceHigh - pendingNonces.NonceLow) +if nonceRange <= 0 { + return simtypes.NoOpMsg( + types.ModuleName, + types.TypeMsgAddOutboundTracker, + "no valid pending nonces", + ), nil, nil +} +nonce := r.Intn(nonceRange) + int(pendingNonces.NonceLow)x/observer/simulation/operation_vote_tss.go (1)
55-57: Update function comment to matchSimulateMsgVoteTSSThe function comment refers to
SimulateVoteOutboundandMsgVoteOutbound, which is incorrect. Update the comment to reflectSimulateMsgVoteTSSandMsgVoteTSS.Apply this diff to correct the comment:
-// SimulateVoteOutbound generates a MsgVoteOutbound with random values +// SimulateMsgVoteTSS generates a MsgVoteTSS with random values // This is the only operation which saves a cctx directly to the store. func SimulateMsgVoteTSS(k keeper.Keeper) simtypes.Operation {x/crosschain/simulation/operation_vote_inbound.go (1)
84-85: Ensure consistent usage of message types inNoOpMsgIn the
NoOpMsgconstruction at lines 84 and 152, the message type usesauthz.InboundVoter.String()instead ofmsg.Type(). For consistency and clarity, usemsg.Type().Apply this diff to standardize the message type:
return simtypes.NoOpMsg( types.ModuleName, - authz.InboundVoter.String(), + msg.Type(), "unable to get asset", ), nil, errAnd
return simtypes.NoOpMsg( types.ModuleName, - authz.InboundVoter.String(), + msg.Type(), "observer set not found", ), nil, nilAlso applies to: 152-153
x/crosschain/simulation/operation_vote_outbound.go (3)
40-40: Provide a Non-Nil Codec intxCtxIn the
txCtxinitialization, theCdcfield is set tonil. Providing a non-nil codec could be beneficial if any codec-related functionality is required during transaction generation or delivery.
120-122: Avoid Redundant Assignment toOutboundTssNonceThe
OutboundTssNonceis assigned tomsgon line 120 after already being set during initialization on line 103 without any intermediate modifications tocctx. Consider removing the redundant assignment to enhance code clarity.
178-179: Use Descriptive Constants for Simulation State IndicesThe use of magic numbers for
curNumVotesStateandstatePercentageArraycan reduce code readability. Defining descriptive constants or enumerations for these state indices would improve maintainability and clarity.simulation/state.go (2)
Line range hint
217-224: Ensure Non-Empty Account List Before Random SelectionThe function
updateAuthorityStateselects a random account fromaccswithout verifying if the list is non-empty. Ifaccsis empty,r.Intn(len(accs))will panic. Please add a check to ensure thataccscontains at least one account before selection.Apply this diff to prevent a potential panic:
+ if len(accs) == 0 { + return authorityState + } randomAccount := accs[r.Intn(len(accs))]
Line range hint
377-377: Avoid Modifying Package-Level VariablesDirectly altering
sdk.DefaultPowerReductioncan lead to unintended side effects across the codebase. Instead, consider using a local variable or passing the value as a parameter to functions that require it.Refactor to prevent modifying global state:
- sdk.DefaultPowerReduction = initialStake.Sub(sdk.OneInt()) + customPowerReduction := initialStake.Sub(sdk.OneInt())testutil/sample/crosschain.go (1)
389-445: Optimize Unsigned Integer Generation inCCTXfromRandConsider using
r.Uint64()directly instead of castingr.Int63()touint64when generating unsigned integers. This eliminates unnecessary casting and utilizes the fulluint64range, enhancing the function's robustness.Apply the following changes for optimization:
// amount := math.NewUint(uint64(r.Int63())) + amount := math.NewUint(r.Uint64())- Amount: math.NewUint(uint64(r.Int63())), + Amount: math.NewUint(r.Uint64()),- GasPrice: math.NewUint(uint64(r.Int63())).String(), + GasPrice: math.NewUint(r.Uint64()).String(),x/crosschain/simulation/operations_test.go (1)
11-23: Rename Test Function to Adhere to Go Naming ConventionsThe test function
Test_Matrixdoes not follow Go's standard naming conventions. Renaming it toTestBallotVoteSimulationMatrixenhances clarity and aligns with best practices.Apply the following change to rename the function:
-func Test_Matrix(t *testing.T) { +func TestBallotVoteSimulationMatrix(t *testing.T) {pkg/chains/chain_filters.go (1)
21-24: LGTM! Consider enhancing documentation.The implementation is clean and consistent with other filter functions. Consider adding an example usage in the documentation comment to improve clarity.
-// FilterByVM filters chains by VM type +// FilterByVM filters chains by VM type. +// Example: +// evmChains := FilterChains(chains, FilterByVM(Vm_evm))x/crosschain/simulation/operation_update_rate_limiter_flags.go (1)
20-21: Fix incomplete commentThe comment ends abruptly with a curly brace.
Apply this change:
-// Fetch the account from the auth keeper which can then be used to fetch spendable coins} +// Fetch the account from the auth keeper which can then be used to fetch spendable coinsx/observer/simulation/operation_update_chain_params.go (1)
29-32: Enhance error handling specificityThe error handling for
GetExternalChaincould be more specific about the failure reason.Consider enhancing the error message:
randomChain, err := GetExternalChain(ctx, k, r) if err != nil { - return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgUpdateChainParams, err.Error()), nil, nil + return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgUpdateChainParams, + "failed to get external chain: " + err.Error()), nil, nil }x/observer/types/expected_keepers.go (1)
22-22: Consider adding error return value for robustnessThe
GetAllValidatorsmethod should consider returning an error value to handle potential failures during validator retrieval, aligning with Cosmos SDK patterns.-GetAllValidators(ctx sdk.Context) (validators []stakingtypes.Validator) +GetAllValidators(ctx sdk.Context) (validators []stakingtypes.Validator, err error)x/observer/simulation/operation_update_keygen.go (1)
35-37: Extract magic numbers as named constantsThe block height calculation uses magic numbers. These should be constants with meaningful names for better maintainability.
+const ( + MinBlockHeightOffset = 11 + MaxBlockHeightOffset = 1000 +) -blockHeightMin := ctx.BlockHeight() + 11 -blockHeightMax := ctx.BlockHeight() + 1000 +blockHeightMin := ctx.BlockHeight() + MinBlockHeightOffset +blockHeightMax := ctx.BlockHeight() + MaxBlockHeightOffsetx/crosschain/simulation/operation_remove_outbound_tracker.go (2)
37-38: Extract random selection logic to helper functionThe random tracker selection logic could be moved to a helper function for better reusability and testing.
+func getRandomTracker(r *rand.Rand, trackers []types.OutboundTracker) types.OutboundTracker { + return trackers[r.Intn(len(trackers))] +} -randomTracker := trackers[r.Intn(len(trackers))] +randomTracker := getRandomTracker(r, trackers)
47-52: Define error messages as constantsError messages should be defined as constants to maintain consistency and make updates easier.
+const ( + ErrNoOutboundTrackers = "no outbound trackers found" + ErrInvalidMsg = "unable to validate MsgRemoveOutboundTracker msg" +) return simtypes.NoOpMsg( types.ModuleName, msg.Type(), - "unable to validate MsgRemoveOutboundTracker msg", + ErrInvalidMsg, ), nil, errx/crosschain/simulation/decoders_test.go (1)
44-51: Enhance test readability and maintainabilityConsider restructuring the test cases for better maintainability and clarity:
- Move test data setup to a separate helper function
- Use constants for magic values
- Structure test cases with input and expected output fields
+ const ( + finalizedInboundValue = byte(1) + ) + + type decodeStoreTestCase struct { + name string + key []byte + value []byte + expected string + } + tests := []struct { - name string - expectedLog string + decodeStoreTestCase }{ - {"CrossChainTx", fmt.Sprintf("cctx key %v\n%v", *cctx, *cctx)}, + { + name: "CrossChainTx", + key: types.KeyPrefix(types.CCTXKey), + value: cdc.MustMarshal(cctx), + expected: fmt.Sprintf("cctx key %v\n%v", *cctx, *cctx), + }, // ... similar changes for other test cases }testutil/keeper/mocks/crosschain/authority.go (1)
57-83: Add documentation and improve error handling consistencyThe new GetPolicies method implementation looks good, but consider these improvements:
- Add documentation explaining the method's purpose and return values
- Consolidate error handling pattern with other methods
+ // GetPolicies provides a mock implementation that returns the authority policies and existence flag. + // It panics if no return value is specified, similar to other mock methods. + // Returns: + // - authoritytypes.Policies: The authority policies + // - bool: True if policies exist, false otherwise func (_m *CrosschainAuthorityKeeper) GetPolicies(ctx types.Context) (authoritytypes.Policies, bool) { ret := _m.Called(ctx) if len(ret) == 0 { - panic("no return value specified for GetPolicies") + panic("no return values specified for GetPolicies") }x/crosschain/simulation/operation_add_inbound_tracker.go (1)
23-26: Consider adding context to error handlingThe error from
GetRandomAccountAndObserveris silently ignored. Consider logging the error or including it in the NoOpMsg for better debugging during simulation failures.if err != nil { - return simtypes.OperationMsg{}, nil, nil + return simtypes.NoOpMsg( + types.ModuleName, + types.TypeMsgAddInboundTracker, + fmt.Sprintf("failed to get random account and observer: %v", err), + ), nil, nil }x/observer/simulation/operation_remove_chain_params.go (1)
65-78: Consider adding transaction gas estimationThe transaction context setup could benefit from gas estimation to ensure realistic simulation conditions.
txCtx := simulation.OperationInput{ R: r, App: app, TxGen: moduletestutil.MakeTestEncodingConfig().TxConfig, Cdc: nil, Msg: &msg, MsgType: msg.Type(), Context: ctx, SimAccount: policyAccount, AccountKeeper: k.GetAuthKeeper(), Bankkeeper: k.GetBankKeeper(), ModuleName: types.ModuleName, CoinsSpentInMsg: spendable, + GasLimit: simulation.DefaultGasLimit * 2, // Double default for safety }x/observer/types/keys.go (2)
74-76: Document the purpose of new block header constantsThe newly added
BlockHeaderKeyandBlockHeaderStateKeyconstants lack documentation explaining their purpose and usage.// TODO remove unused keys +// BlockHeaderKey is the key prefix for storing block headers BlockHeaderKey = "BlockHeader-value-" +// BlockHeaderStateKey is the key prefix for storing block header states BlockHeaderStateKey = "BlockHeaderState-value-"
74-74: Consider tracking TODO items in issue trackerThe TODO comment about removing unused keys should be tracked in the issue tracker for better visibility and follow-up.
Would you like me to create a GitHub issue to track the removal of unused keys?
x/crosschain/types/keys.go (1)
29-30: Document the rationale for MaxOutboundTrackerHashes valueThe value
5appears to be a magic number. Please add a comment explaining why this specific limit was chosen and what implications it has for the system.x/crosschain/simulation/operation_refund_aborted_cctx.go (2)
31-51: Consider optimizing CCTX lookupThe current implementation performs a linear search through all cross-chain transactions. Consider adding an index or filter at the keeper level for aborted transactions to improve simulation performance.
24-26: Enhance error handling specificityThe error message could be more specific about why the policy account retrieval failed.
- return simtypes.NoOpMsg(types.ModuleName, types.RefundAborted, err.Error()), nil, nil + return simtypes.NoOpMsg(types.ModuleName, types.RefundAborted, + fmt.Sprintf("failed to get policy account: %v", err)), nil, nilx/crosschain/simulation/operation_gas_price_voter.go (1)
65-78: Consider adding simulation parametersThe transaction context could benefit from configurable simulation parameters for more controlled testing.
Consider adding simulation parameters to control:
- Gas price ranges
- Block number ranges
- Supply ranges
- Transaction fee ranges
x/crosschain/keeper/msg_server_add_outbound_tracker.go (1)
Case-sensitive hash comparison is required for security
The codebase implements strict hash validation through
ValidateHashForChainwhich enforces exact format requirements for different chains:
- Ethereum/Zeta chains: Valid hex format via
hexutil.Decode- Bitcoin chains: Exact 64-character hex string via regex
^[a-fA-F0-9]{64}$Using case-insensitive comparison with
strings.EqualFoldcontradicts these strict validation rules and could potentially lead to security issues. Consider using case-sensitive comparison:if hash.TxHash == msg.TxHash {🔗 Analysis chain
Line range hint
71-75: Consider using case-insensitive hash comparisonThe current implementation uses
strings.EqualFoldfor hash comparison, which is appropriate for case-insensitive comparison. However, consider documenting this behavior or using case-sensitive comparison if exact matching is required for security reasons.Let's verify the hash comparison requirements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other hash comparisons in the codebase rg -A 2 "EqualFold.*[Hh]ash" rg -A 2 "strings\.Compare.*[Hh]ash"Length of output: 866
Script:
#!/bin/bash # Let's check for any hash validation or comparison patterns in the codebase ast-grep --pattern 'if $_ == $hash { $$$$ }' # Also check for any hash-related constants or types that might indicate the expected format rg -i "type.*hash" -A 2 rg -i "const.*hash" -A 2 # Look for hash validation patterns rg -i "validate.*hash" -A 2Length of output: 101811
Script:
#!/bin/bash # Let's check for hash validation functions and their implementations ast-grep --pattern 'func ValidateHashForChain($$$)' # Also check for any hash validation patterns in the specific file rg -A 5 "strings\.EqualFold.*[Hh]ash" x/crosschain/keeper/msg_server_add_outbound_tracker.goLength of output: 1621
x/observer/keeper/msg_server_vote_tss.go (1)
Line range hint
106-113: Verify edge cases in ballot discarding.The check for older pending ballots is a good addition. However, consider adding logging to help track these cases.
if msg.KeygenZetaHeight != keygen.BlockNumber { + k.Logger(ctx).Info("Discarding vote for older pending ballot", + "keygen_height", keygen.BlockNumber, + "ballot_height", msg.KeygenZetaHeight) return &types.MsgVoteTSSResponse{ VoteFinalized: isFinalized, BallotCreated: ballotCreated, KeygenSuccess: false, }, nil }x/observer/genesis.go (2)
16-18: Consider simplifying empty observer set initialization.The empty observer set initialization can be more concise by using a ternary-like approach.
- if genState.Observers.Len() > 0 { - k.SetObserverSet(ctx, genState.Observers) - } else { - k.SetObserverSet(ctx, types.ObserverSet{}) - } + observers := genState.Observers + if observers.Len() == 0 { + observers = types.ObserverSet{} + } + k.SetObserverSet(ctx, observers)
20-24: Consider reordering observer count calculation.The observer count calculation should precede the conditional block for better code organization and to avoid recalculating
Len().- observerCount := uint64(genState.Observers.Len()) - if genState.LastObserverCount != nil { - k.SetLastObserverCount(ctx, genState.LastObserverCount) - } else { - k.SetLastObserverCount(ctx, &types.LastObserverCount{LastChangeHeight: 0, Count: observerCount}) - } + observerCount := uint64(genState.Observers.Len()) + lastObserverCount := genState.LastObserverCount + if lastObserverCount == nil { + lastObserverCount = &types.LastObserverCount{LastChangeHeight: 0, Count: observerCount} + } + k.SetLastObserverCount(ctx, lastObserverCount)testutil/sample/crypto.go (2)
76-87: Extract common error handling logic.The error handling pattern is duplicated. Consider extracting the common logic into a helper function.
+func pubKeyFromPrivKey(privKey *ed25519.PrivKey) (string, error) { + s, err := cosmos.Bech32ifyPubKey(cosmos.Bech32PubKeyTypeAccPub, privKey.PubKey()) + if err != nil { + return "", err + } + return crypto.NewPubKey(s) +} func PubkeyStringFromRand(r *rand.Rand) string { priKey := Ed25519PrivateKeyFromRand(r) - s, err := cosmos.Bech32ifyPubKey(cosmos.Bech32PubKeyTypeAccPub, priKey.PubKey()) - if err != nil { - panic(err) - } - pubkey, err := crypto.NewPubKey(s) + pubkey, err := pubKeyFromPrivKey(priKey) if err != nil { panic(err) } return pubkey.String() }
155-158: Fix duplicate function comment.The comment for
HashFromRandis identical to the comment forHash. Consider updating it to be more specific.-// Hash returns a sample hash +// HashFromRand returns a sample hash generated from a random source func HashFromRand(r *rand.Rand) ethcommon.Hash {pkg/chains/chain_filters_test.go (1)
51-65: Consider adding edge cases to VM filter testWhile the test case correctly validates the basic VM filtering functionality, consider adding edge cases such as:
- Empty chain list
- List with no EVM chains
- List with mixed VM types
testutil/sample/observer.go (1)
326-335: Consider parameterizing hardcoded valuesThe function could be improved by:
- Extracting min/max values as constants
- Making GasPriceIncreasePercent and MaxPendingCctxs configurable through parameters
+const ( + MinGasPriceValue = 1 + MaxGasPriceValue = 100 + DefaultGasPriceIncreasePercent = 1 + DefaultMaxPendingCctxs = 100 +) -func GasPriceIncreaseFlagsFromRand(r *rand.Rand) types.GasPriceIncreaseFlags { +func GasPriceIncreaseFlagsFromRand(r *rand.Rand, gasPriceIncreasePercent, maxPendingCctxs int64) types.GasPriceIncreaseFlags { return types.GasPriceIncreaseFlags{ - EpochLength: int64(r.Intn(maxValue-minValue) + minValue), - RetryInterval: time.Duration(r.Intn(maxValue-minValue) + minValue), + EpochLength: int64(r.Intn(MaxGasPriceValue-MinGasPriceValue) + MinGasPriceValue), + RetryInterval: time.Duration(r.Intn(MaxGasPriceValue-MinGasPriceValue) + MinGasPriceValue), - GasPriceIncreasePercent: 1, - MaxPendingCctxs: 100, + GasPriceIncreasePercent: gasPriceIncreasePercent, + MaxPendingCctxs: maxPendingCctxs, } }simulation/simulation_test.go (1)
Line range hint
539-565: Consider extracting common setup logicThe setup code for the new simulation app is duplicated across test functions. Consider extracting this into a helper function to improve maintainability.
+func setupNewSimulationApp(t *testing.T, config simulation.Config, appOptions cosmossimutils.AppOptionsMap, logger log.Logger) (*app.App, db.DB, string) { + newDB, newDir, _, _, err := cosmossimutils.SetupSimulation( + config, + SimDBBackend+"_new", + SimDBName+"_new", + zetasimulation.FlagVerboseValue, + zetasimulation.FlagEnabledValue, + ) + require.NoError(t, err, "simulation setup failed") + + newSimApp, err := zetasimulation.NewSimApp( + logger, + newDB, + appOptions, + interBlockCacheOpt(), + baseapp.SetChainID(SimAppChainID), + ) + require.NoError(t, err) + + return newSimApp, newDB, newDir +}Makefile (1)
433-438: Consider adding test dependenciesThe quick test target should explicitly depend on the
runsimtarget to ensure all required tools are available.-test-sim-quick: +test-sim-quick: runsimzetaclient/chains/evm/observer/outbound.go (1)
Line range hint
512-512: Remove unused LastBlock() callThis appears to be an orphaned call to
LastBlock()that doesn't store or use the return value.-ob.LastBlock()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (65)
Makefile(2 hunks)changelog.md(1 hunks)pkg/chains/chain_filters.go(1 hunks)pkg/chains/chain_filters_test.go(1 hunks)pkg/coin/coin.go(1 hunks)pkg/coin/coint_test.go(1 hunks)simulation/simulation_test.go(7 hunks)simulation/state.go(11 hunks)testutil/keeper/mocks/crosschain/authority.go(2 hunks)testutil/keeper/mocks/observer/staking.go(1 hunks)testutil/sample/crosschain.go(4 hunks)testutil/sample/crypto.go(4 hunks)testutil/sample/observer.go(5 hunks)testutil/sample/sample.go(1 hunks)x/crosschain/genesis.go(2 hunks)x/crosschain/keeper/cctx_utils.go(0 hunks)x/crosschain/keeper/msg_server_add_outbound_tracker.go(1 hunks)x/crosschain/keeper/msg_server_add_outbound_tracker_test.go(1 hunks)x/crosschain/keeper/msg_server_refund_aborted_tx.go(0 hunks)x/crosschain/keeper/msg_server_vote_inbound_tx.go(1 hunks)x/crosschain/keeper/msg_server_whitelist_erc20.go(1 hunks)x/crosschain/keeper/refund.go(1 hunks)x/crosschain/simulation/decoders.go(1 hunks)x/crosschain/simulation/decoders_test.go(1 hunks)x/crosschain/simulation/operation_abort_stuck_cctx.go(1 hunks)x/crosschain/simulation/operation_add_inbound_tracker.go(1 hunks)x/crosschain/simulation/operation_add_outbound_tracker.go(1 hunks)x/crosschain/simulation/operation_gas_price_voter.go(1 hunks)x/crosschain/simulation/operation_migrate_erc20_custody_funds.go(1 hunks)x/crosschain/simulation/operation_refund_aborted_cctx.go(1 hunks)x/crosschain/simulation/operation_remove_outbound_tracker.go(1 hunks)x/crosschain/simulation/operation_update_erc20_pause_status.go(1 hunks)x/crosschain/simulation/operation_update_rate_limiter_flags.go(1 hunks)x/crosschain/simulation/operation_update_tss_address.go(1 hunks)x/crosschain/simulation/operation_vote_inbound.go(1 hunks)x/crosschain/simulation/operation_vote_outbound.go(1 hunks)x/crosschain/simulation/operation_whitelist_erc20.go(1 hunks)x/crosschain/simulation/operations.go(6 hunks)x/crosschain/simulation/operations_test.go(1 hunks)x/crosschain/types/expected_keepers.go(2 hunks)x/crosschain/types/keys.go(1 hunks)x/crosschain/types/outbound_tracker.go(1 hunks)x/crosschain/types/outbound_tracker_test.go(1 hunks)x/fungible/keeper/deposits.go(2 hunks)x/fungible/simulation/operations.go(1 hunks)x/observer/genesis.go(1 hunks)x/observer/keeper/msg_server_update_observer.go(2 hunks)x/observer/keeper/msg_server_vote_tss.go(2 hunks)x/observer/keeper/tss.go(1 hunks)x/observer/simulation/decoders.go(1 hunks)x/observer/simulation/operation_add_observer.go(1 hunks)x/observer/simulation/operation_add_observer_node_account.go(1 hunks)x/observer/simulation/operation_disable_cctx.go(1 hunks)x/observer/simulation/operation_enable_cctx.go(1 hunks)x/observer/simulation/operation_remove_chain_params.go(1 hunks)x/observer/simulation/operation_reset_chain_nonces.go(1 hunks)x/observer/simulation/operation_update_chain_params.go(1 hunks)x/observer/simulation/operation_update_gas_price_increase_flags.go(1 hunks)x/observer/simulation/operation_update_keygen.go(1 hunks)x/observer/simulation/operation_update_observer.go(1 hunks)x/observer/simulation/operation_vote_tss.go(1 hunks)x/observer/simulation/operations.go(2 hunks)x/observer/types/expected_keepers.go(1 hunks)x/observer/types/keys.go(1 hunks)zetaclient/chains/evm/observer/outbound.go(1 hunks)
💤 Files with no reviewable changes (2)
- x/crosschain/keeper/cctx_utils.go
- x/crosschain/keeper/msg_server_refund_aborted_tx.go
✅ Files skipped from review due to trivial changes (1)
- x/observer/keeper/tss.go
🧰 Additional context used
📓 Path-based instructions (60)
x/crosschain/types/outbound_tracker.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/expected_keepers.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/decoders_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/coin/coin.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/chains/chain_filters.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/keys.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/expected_keepers.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/decoders.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operations_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/chains/chain_filters_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/mocks/observer/staking.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/refund.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/types/outbound_tracker_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/mocks/crosschain/authority.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/genesis.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
pkg/coin/coint_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_update_chain_params.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/genesis.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_update_rate_limiter_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_update_gas_price_increase_flags.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/keeper/msg_server_update_observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_add_outbound_tracker.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/sample/sample.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_whitelist_erc20.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_add_observer_node_account.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/deposits.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_add_outbound_tracker_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_remove_chain_params.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_update_erc20_pause_status.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/keeper/msg_server_vote_tss.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_add_outbound_tracker.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_enable_cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_disable_cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_update_keygen.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_update_tss_address.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_vote_tss.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_gas_price_voter.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_migrate_erc20_custody_funds.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_add_observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_add_inbound_tracker.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_update_observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_vote_inbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/decoders.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/keeper/msg_server_vote_inbound_tx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_vote_outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/sample/observer.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_whitelist_erc20.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_remove_outbound_tracker.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operation_reset_chain_nonces.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_refund_aborted_cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
zetaclient/chains/evm/observer/outbound.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operation_abort_stuck_cctx.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/sample/crypto.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
simulation/simulation_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/simulation/operations.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/types/keys.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
simulation/state.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/sample/crosschain.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/simulation/operations.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/simulation/operations.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
🔇 Additional comments (32)
x/fungible/simulation/operations.go (1)
Line range hint 54-124: Verify single-run behavior and enhance error handling
The function claims to run only once in the first block, but this behavior isn't explicitly enforced in the code.
Additionally, consider making error messages more specific and actionable:
if !found {
return simtypes.NoOpMsg(
types.ModuleName,
types.TypeMsgDeploySystemContracts,
- "policies object not found",
+ "global policies not initialized in state",
), nil, nil
}
if len(policies.Items) == 0 {
return simtypes.NoOpMsg(
types.ModuleName,
types.TypeMsgDeploySystemContracts,
- "no policies found",
+ "no policy items found in global policies",
), nil, nil
}testutil/sample/crosschain.go (4)
57-85: Addition of RateLimiterFlagsFromRand Function: Approved
The new RateLimiterFlagsFromRand function enhances flexibility by allowing the generation of RateLimiterFlags with a provided random source, promoting better control in testing scenarios.
Line range hint 361-378: Enhanced InboundVoteFromRand with Asset Parameter: Approved
The updated InboundVoteFromRand function now accepts an asset parameter, increasing its versatility in generating inbound vote messages tailored to specific assets.
383-387: Efficient Random Coin Type Selection: Approved
The CoinTypeFromRand function efficiently selects a random CoinType from predefined options, ensuring varied and comprehensive test coverage.
447-465: Addition of OutboundVoteSim Function: Approved
The OutboundVoteSim function is well-defined, generating simulated outbound vote messages accurately based on provided cross-chain transactions.
x/crosschain/types/outbound_tracker.go (1)
3-5: Proper Implementation of IsMaxed Method: Approved
The IsMaxed method in OutboundTracker correctly determines if the maximum number of hashes has been reached, improving code readability and maintainability by encapsulating this logic.
x/crosschain/types/outbound_tracker_test.go (1)
9-48: Comprehensive Testing in TestOutboundTracker_IsMaxed: Approved
The test cases in TestOutboundTracker_IsMaxed methodically verify the IsMaxed function across various scenarios, ensuring reliable behavior when the hash list is below, at, and above the maximum threshold.
x/observer/simulation/operation_update_gas_price_increase_flags.go (1)
26-28: 🛠️ Refactor suggestion
Add spendable coins validation
The code fetches spendable coins but doesn't validate if they're sufficient for the transaction.
Add validation before proceeding with the transaction:
spendable := k.GetBankKeeper().SpendableCoins(ctx, authAccount.GetAddress())
+if spendable.IsZero() {
+ return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgUpdateGasPriceIncreaseFlags, "insufficient funds"), nil, nil
+}x/observer/simulation/operation_update_keygen.go (1)
26-28:
Verify sufficient funds before transaction
The code retrieves spendable coins but doesn't validate if they're sufficient for the transaction.
Add validation before proceeding with the transaction.
spendable := k.GetBankKeeper().SpendableCoins(ctx, authAccount.GetAddress())
+if spendable.IsZero() {
+ return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgUpdateKeygen, "insufficient funds"), nil, nil
+}x/crosschain/simulation/operation_remove_outbound_tracker.go (1)
24-25:
Add funds validation before transaction
Similar to the previous file, spendable coins should be validated before proceeding with the transaction.
spendable := k.GetBankKeeper().SpendableCoins(ctx, authAccount.GetAddress())
+if spendable.IsZero() {
+ return simtypes.NoOpMsg(types.ModuleName, types.TypeMsgRemoveOutboundTracker, "insufficient funds"), nil, nil
+}x/crosschain/genesis.go (1)
13-14: Add validation for ZetaAccounting initialization
Consider adding validation to ensure ZetaAccounting is properly initialized.
x/crosschain/simulation/operation_add_inbound_tracker.go (1)
55-68: Consider validating spendable coins before transaction setup
The operation sets up the transaction context with spendable coins without validating if the account has sufficient balance.
Consider adding balance validation:
spendable := k.GetBankKeeper().SpendableCoins(ctx, authAccount.GetAddress())
+if spendable.IsZero() {
+ return simtypes.NoOpMsg(
+ types.ModuleName,
+ types.TypeMsgAddInboundTracker,
+ "insufficient balance",
+ ), nil, nil
+}x/crosschain/simulation/decoders.go (1)
22-57: LGTM! Enhanced log readability
The addition of descriptive prefixes to each case's output improves log readability and debugging capabilities.
x/crosschain/simulation/operation_refund_aborted_cctx.go (1)
53-57: Validate generated refund address
The refund address is generated using sample.EthAddressFromRand without additional validation. Consider adding validation to ensure the generated address meets all requirements for the target chain.
testutil/keeper/mocks/observer/staking.go (1)
18-36: LGTM: Mock implementation follows best practices
The implementation includes proper error handling and follows the standard mock pattern.
x/crosschain/keeper/msg_server_add_outbound_tracker.go (1)
Line range hint 88-95: LGTM: Clean refactor to use IsMaxed()
The refactoring to use IsMaxed() improves encapsulation and maintainability.
x/observer/keeper/msg_server_update_observer.go (2)
Line range hint 39-43: LGTM: Robust validator verification added
The addition of validator verification enhances system security by ensuring new observers meet the required criteria.
76-81: LGTM: Enhanced error reporting
The improved error message now includes both current and last block observer counts, facilitating better debugging.
x/fungible/keeper/deposits.go (1)
Line range hint 103-113: LGTM: Clear gas coin handling logic
The implementation properly handles both gas coins and no-asset calls with appropriate error handling.
Let's verify the error handling consistency:
✅ Verification successful
Error handling is consistent and well-implemented across the codebase
The verification shows that both ErrGasCoinNotFound and ErrForeignCoinNotFound errors are:
- Properly registered in their respective modules (
x/fungibleandx/crosschain) - Consistently used across keeper methods with appropriate context in error messages
- Well-covered by unit tests in both modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent error handling patterns across the codebase
rg -A 2 "ErrGasCoinNotFound|ErrForeignCoinNotFound"
Length of output: 6905
x/observer/keeper/msg_server_vote_tss.go (1)
Line range hint 30-35: LGTM! Improved authorization check.
The simplified authorization check using node account verification is more robust and maintainable.
x/crosschain/keeper/msg_server_vote_inbound_tx.go (2)
67-67: LGTM! Excellent error handling improvement.
Using a temporary context to prevent committing state changes on error is a robust approach.
Line range hint 82-94: LGTM! Critical security enhancement.
The double-spending prevention check is a crucial security feature. The error message is clear and includes all necessary context.
✅ Verification successful
Double-spending prevention mechanism is correctly implemented and thoroughly tested
The verification confirms that:
- The
IsFinalizedInboundcheck is consistently used across the codebase for preventing double-spending - The implementation is well-tested with both positive and negative test cases in
finalized_inbounds_test.go - The mechanism is properly integrated into the inbound transaction processing flow
- The code maintains a finalized inbound store specifically for this security feature
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if there are any other places in the codebase where inbound transactions are processed
# to ensure consistent double-spending prevention
# Search for functions that process inbound transactions
ast-grep --pattern 'func $_($_, $_) {
$$$
InboundHash
$$$
}'
# Search for other potential double-spending checks
rg -A 3 "IsFinalizedInbound|double.*spend"
Length of output: 6298
x/crosschain/keeper/msg_server_whitelist_erc20.go (1)
Line range hint 110-117: LGTM! Critical validation for deployed contract address.
The added validation ensures that the deployed ZRC20 contract address is not zero, preventing potential issues with invalid contract deployments. The error message is clear and informative.
x/crosschain/types/expected_keepers.go (1)
16-16: LGTM: Clean interface extension
The addition of GetPolicies method follows the established keeper interface patterns and maintains consistency with other methods.
Also applies to: 228-228
testutil/sample/observer.go (2)
73-80: LGTM: Well-structured random generator
The function follows established patterns and properly generates random values for testing.
159-176: LGTM: Improved error handling
The changes properly implement Go's error handling patterns and improve the function's reliability.
simulation/simulation_test.go (2)
67-72: Well-documented test objectives
Clear and comprehensive documentation of the test's purpose and steps, improving maintainability.
572-572: Consider adding error checks for simulation parameters
The simulation parameters should be validated before running the simulation.
Makefile (1)
412-412: LGTM: Improved test coverage
Increased test parameters (100 blocks, 200 transactions) provide better test coverage.
changelog.md (1)
14-14: LGTM!
The changelog entry is clear, concise, and properly references the PR number.
x/crosschain/keeper/msg_server_add_outbound_tracker_test.go (1)
229-230: LGTM: Well-structured test for maximum hash limit
The test correctly validates the behavior when the maximum number of outbound tracker hashes is reached, using the appropriate constant from the types package.
zetaclient/chains/evm/observer/outbound.go (1)
128-131: LGTM: Improved encapsulation with IsMaxed method
The code now properly delegates the maximum hash check to the tracker object, following good object-oriented principles while maintaining clear error reporting.
| for _, elem := range genState.CrossChainTxs { | ||
| if elem != nil { | ||
| cctx := *elem | ||
| k.SetCrossChainTx(ctx, cctx) |
There was a problem hiding this comment.
While working on this PR, I realized that We were saving many different parts of the state and doing calculations during genesis.
I have the opinion now that the init genesis should be as simple as possible, and we should only be importing the exported state without modifying it.
I am open to hearing other thoughts and opinions.
Save update did the following
- SetNonceToCCTX - Part of the observer state and should be managed by observer module
- SetCrossChainTx - We are still doing this
- UpdateInboundHashToCCTX - This is already being set separately using
genState.InboundHashToCctxList - UpdateZetaAccounting - This can be set using genstate.ZetaAccounting , (This was remove in this pr https://github.com/zeta-chain/node/pull/3230/files#diff-2dce2b3ecc78bbb08e9523c64c1ad6ec82694ea0d821d032570383d951c5989f , but I think it makes more sense to keep it )
There was a problem hiding this comment.
Yes, it's correct, genesis logic should only use getter/setter function but it seems it would makemore sense to create a dedicated PR for these changes
There was a problem hiding this comment.
Happy to create a separate pr for this , and merge that before trying to merge this if needed .
However since it does nto change any logic, I considered putting it in this pr directly , but if it helps with reviewing , I can create a pr
| // Return early if inbound is not enabled. | ||
| if !cf.IsInboundEnabled { | ||
| return simtypes.NoOpMsg(types.ModuleName, msg.Type(), "inbound is not enabled"), nil, nil | ||
| } | ||
|
|
There was a problem hiding this comment.
Added this check in this PR, which would fail this transaction if inbound is disabled , instead of erroring when delivering the message( which causes the the test to stop )
This would allow us to reduce the weight we used for MsgEnableCCTX , without causing the tests to break . Add the cost of increased failures for VoteInbound
This comment thread from the last pr for reference
#3095 (comment)
However, as mentioned in the comment thread, I think adding a flag would be the best way forward
| {"ERC20", coin.CoinType_ERC20, true}, | ||
| {"Gas", coin.CoinType_Gas, true}, | ||
| {"Zeta", coin.CoinType_Zeta, true}, | ||
| {"Cmd", coin.CoinType_Cmd, false}, | ||
| {"Unknown", coin.CoinType(100), false}, |
There was a problem hiding this comment.
I think it would be better to keep this as it is , since these are all proper nouns
| @@ -36,6 +34,9 @@ const ( | |||
| InitiallyBondedValidators = "initially_bonded_validators" | |||
| ) | |||
|
|
|||
| // updateBankState updates the bank genesis state. | |||
There was a problem hiding this comment.
The update naming seems off in this file, we return a new genesis variable instead of update a variable, should rather be named
bankGenesisState
evmGenesisState
...
I would also rename the file into genesis.go
There was a problem hiding this comment.
We extract the module specific portion of the genesis from the rawstate, update and return it. The default values are still sustained . I do think update matches the logic more in this case .
The main function for this file is the AppStateFn which , basically returns the application state and is used by SimulateFromSeed function by the sdk .
| } | ||
|
|
||
| cctx, found := k.GetCrossChainTx(ctx, nonceToCctx.CctxIndex) | ||
| if !found { |
There was a problem hiding this comment.
We should check that the CCTX is pending, purpose of simulation tests is to send message that triggers state transitions
There was a problem hiding this comment.
We are using pending nonces on the current block to fetch the CCTX .
If the CCTX is not pending , then it would be a bug in the logic and should be reported
x/crosschain/simulation/operation_migrate_erc20_custody_funds.go
Outdated
Show resolved
Hide resolved
| authAccount := k.GetAuthKeeper().GetAccount(ctx, policyAccount.Address) | ||
| spendable := k.GetBankKeeper().SpendableCoins(ctx, authAccount.GetAddress()) | ||
|
|
||
| msg := types.MsgDisableCCTX{ |
There was a problem hiding this comment.
Can't we had randomness to disable outbound as well
There was a problem hiding this comment.
We can , however I do not want to add more disable Inbound messages into the simulation as the Disable inbound is already a problem as mentioned above
#3207 (comment)
If we keep DisableInbound: false . DisableOutbound has to be true .
Description
The pr
Adds all the operations for crosschain module except
MsgMigrateTssFunds. MigrateTSSFunds expects inbound to be paused and no new pending nonces to be generated, which does not fit in well with how simulation tests runAdds all the operations for the observer module except .
MsgRemoveChainParams. Removing or disabling a chain causes a lot of other simulations to fail, I dont think its required to be added into simulation testsSimplifies the init genesis for cross-chain module
Closes
#2973
#2974
#2975
#3100
How Has This Been Tested?
Summary by CodeRabbit
Release Notes
New Features
/systemtimeadded to enhance monitoring capabilities.Improvements
Bug Fixes
Documentation