test: initialize simulation tests for custom zetachain modules #3095
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several modifications across various files, primarily focusing on enhancing the initialization and configuration of application components, updating simulation tests, and refining error handling. Key changes include adjustments to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
kingpinXD
requested review from
brewmaster012,
lumtis,
ws4charlie,
skosito,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 27
🧹 Outside diff range and nitpick comments (63)
x/fungible/types/genesis.go (1)
Line range hint
1-35: Add documentation for contract addressesThe file lacks documentation explaining the purpose and significance of the SystemContract and ConnectorZevm addresses.
Add documentation above the SystemContract struct:
// SystemContract contains addresses for core system contracts // SystemContract: The main system contract address that handles core functionality // ConnectorZevm: The ZevM connector contract that facilitates cross-chain operations type SystemContract struct { SystemContract string ConnectorZevm string }x/fungible/simulation/decoders.go (1)
13-15: Enhance function documentation for better clarity.Consider expanding the documentation to include:
- Description of panic conditions
- Format of the returned string
- Parameter types and their purpose
-// NewDecodeStore returns a decoder function closure that unmarshals the KVPair's -// Value to the corresponding fungible types. +// NewDecodeStore returns a decoder function closure that unmarshals the KVPair's +// Value to the corresponding fungible types. The returned function takes two kv.Pair +// parameters and returns a formatted string comparing their values. +// +// The decoder supports the following key prefixes: +// - SystemContractKey: Decodes to types.SystemContract +// - ForeignCoinsKeyPrefix: Decodes to types.ForeignCoins +// +// It panics if an unsupported key prefix is encountered.x/observer/module_simulation.go (2)
13-15: Excellent refactoring to improve modularityThe delegation to
simulation.RandomizedGenStateis a good architectural choice that:
- Centralizes genesis state generation logic
- Reduces code duplication
- Improves maintainability
Consider documenting the expected behavior of
RandomizedGenStatein the package documentation to help future maintainers understand the genesis state generation strategy.
32-35: Clean implementation with room for documentation improvementThe refactoring to use
simulation.WeightedOperationsimproves code clarity and maintainability.Consider adding a comment explaining the expected parameters and their impact on operation weights:
// WeightedOperations returns the all the gov module operations with their respective weights. +// Parameters: +// - appParams: Application parameters for weight calculation +// - cdc: Codec for encoding/decoding +// - keeper: Module keeper for accessing state func (am AppModule) WeightedOperations(simState module.SimulationState) []simtypes.WeightedOperation {x/fungible/module_simulation.go (1)
27-29: Consider adding documentation for the store decoder registrationThe implementation is correct and follows standard patterns. However, it would be beneficial to add a brief comment explaining what types of store items this decoder handles.
// RegisterStoreDecoder registers a decoder +// for fungible module's types func (am AppModule) RegisterStoreDecoder(sdr sdk.StoreDecoderRegistry) {x/fungible/simulation/decoders_test.go (3)
15-21: Add function documentation and handle unused return values.The test function setup could be improved for better maintainability and clarity.
Apply these changes:
+// TestDecodeStore validates the store decoder functionality for fungible module +// by testing the decoding of SystemContract and ForeignCoins data func TestDecodeStore(t *testing.T) { - k, _, _, _ := keepertest.FungibleKeeper(t) + k, ctx, _, keeper := keepertest.FungibleKeeper(t) + _ = ctx // Explicitly ignore if unused + _ = keeper // Explicitly ignore if unused
22-27: Consider adding error handling for marshaling operations.While
MustMarshalis commonly used in tests, consider usingMarshalwith proper error handling for more robust tests.Consider this approach:
kvPairs := kv.Pairs{ Pairs: []kv.Pair{ - {Key: []byte(types.SystemContractKey), Value: cdc.MustMarshal(systemContract)}, - {Key: []byte(types.ForeignCoinsKeyPrefix), Value: cdc.MustMarshal(&foreignCoins)}, + { + Key: []byte(types.SystemContractKey), + Value: func() []byte { + bz, err := cdc.Marshal(systemContract) + require.NoError(t, err) + return bz + }(), + }, + { + Key: []byte(types.ForeignCoinsKeyPrefix), + Value: func() []byte { + bz, err := cdc.Marshal(&foreignCoins) + require.NoError(t, err) + return bz + }(), + }, }, }
37-43: Enhance test execution with additional validations.The current test execution could benefit from more comprehensive error checking and edge cases.
Consider enhancing the test execution:
for i, tt := range tests { i, tt := i, tt t.Run(tt.name, func(t *testing.T) { - require.Equal(t, tt.expectedLog, dec(kvPairs.Pairs[i], kvPairs.Pairs[i])) + // Test normal case + result := dec(kvPairs.Pairs[i], kvPairs.Pairs[i]) + require.NotEmpty(t, result) + require.Equal(t, tt.expectedLog, result) + + // Test with nil values + require.NotPanics(t, func() { + dec(kv.Pair{}, kv.Pair{}) + }) }) }x/fungible/keeper/keeper.go (1)
57-59: Add documentation and consider method organization.While the implementation is correct, consider:
- Adding documentation to explain the method's purpose and usage
- Grouping it with other getter methods for better code organization
-func (k Keeper) GetCodec() codec.Codec { - return k.cdc -} +// GetCodec returns the codec used for serialization and deserialization +func (k Keeper) GetCodec() codec.Codec { + return k.cdc +}x/observer/types/genesis.go (1)
Line range hint
22-52: Add validation for the Keygen field.The Validate() function performs thorough validation for NodeAccount and ChainNonces, but lacks validation for the newly initialized Keygen field. Consider adding validation to ensure consistency.
func (gs GenesisState) Validate() error { + // Validate Keygen if present + if gs.Keygen != nil { + // Add appropriate validation logic for Keygen fields + } + // Check for duplicated index in nodeAccount nodeAccountIndexMap := make(map[string]bool)x/observer/keeper/keeper.go (2)
30-30: Consider adding validation for required keeper dependenciesWhile the authority validation is good, consider adding validation for the new keeper dependencies to ensure they're not nil. This follows defensive programming practices common in Cosmos SDK modules.
func NewKeeper( cdc codec.Codec, storeKey, memKey storetypes.StoreKey, stakingKeeper types.StakingKeeper, slashinKeeper types.SlashingKeeper, authorityKeeper types.AuthorityKeeper, lightclientKeeper types.LightclientKeeper, bankKeeper types.BankKeeper, authKeeper types.AccountKeeper, authority string, ) *Keeper { if _, err := sdk.AccAddressFromBech32(authority); err != nil { panic(err) } + if bankKeeper == nil { + panic("bankKeeper is nil") + } + if authKeeper == nil { + panic("authKeeper is nil") + }Also applies to: 37-38, 53-54
91-93: Consider adding documentation for codec capabilitiesThe change to
codec.Codecis good as it provides more flexibility. Consider adding a doc comment to clarify the supported encoding formats (JSON/Binary) for future maintainers.+// Codec returns the keeper's codec which supports both binary and JSON encoding func (k Keeper) Codec() codec.Codec { return k.cdc }x/observer/types/expected_keepers.go (2)
5-5: Consider using a more specific alias for auth typesTo improve code clarity and prevent potential naming conflicts with the local package types, consider using a more specific alias.
-"github.com/cosmos/cosmos-sdk/x/auth/types" +"github.com/cosmos/cosmos-sdk/x/auth/types" authtypes
67-70: Enhance AccountKeeper documentationWhile the interface is well-designed, consider expanding the documentation to clarify:
- The purpose of "noalias" directive
- The specific simulation scenarios where this keeper is used
-// AccountKeeper defines the expected account keeper used for simulations (noalias) +// AccountKeeper defines the expected account keeper used for simulations. +// The noalias directive is used to prevent interface aliasing during simulations, +// ensuring clean dependency tracking during test scenarios.x/crosschain/simulation/decoders_test.go (3)
15-26: Add function documentation and consider handling all keeper return values.
- Add documentation to explain the test's purpose and methodology.
- Consider handling or documenting why we're ignoring the keeper's return values.
+// TestDecodeStore validates the simulation store decoder's ability to correctly +// decode various cross-chain related store entries including transactions, +// trackers, and configuration data. func TestDecodeStore(t *testing.T) { - k, _, _, _ := keepertest.CrosschainKeeper(t) + k, ctx, _, keeper2 := keepertest.CrosschainKeeper(t) + // Note: ctx and keeper2 are unused in this test but kept for clarity
27-38: Consider consistent pointer usage and error handling in marshaling.The marshaling operations use
MustMarshalwhich panics on error. For production code, consider:
- Using consistent pointer/value types for marshaling
- Implementing proper error handling instead of using
MustMarshalkvPairs := kv.Pairs{ Pairs: []kv.Pair{ - {Key: types.KeyPrefix(types.CCTXKey), Value: cdc.MustMarshal(cctx)}, + {Key: types.KeyPrefix(types.CCTXKey), Value: cdc.MustMarshal(&cctx)}, // ... similar changes for other pairs }, }
54-60: Consider adding timeout protection and parallel execution support.The test execution is correct but could be enhanced:
- Add test timeout protection
- Enable parallel test execution where appropriate
for i, tt := range tests { i, tt := i, tt t.Run(tt.name, func(t *testing.T) { + t.Parallel() // Enable parallel execution + timer := time.NewTimer(5 * time.Second) + done := make(chan struct{}) + go func() { require.Equal(t, tt.expectedLog, dec(kvPairs.Pairs[i], kvPairs.Pairs[i])) + close(done) + }() + select { + case <-timer.C: + t.Fatal("Test timeout") + case <-done: + } }) }testutil/keeper/mocks/fungible/bank.go (1)
70-88: LGTM with a minor suggestion for improvement.The implementation correctly handles nil cases for the Coins type, which is necessary since it's a slice. However, we can make it more idiomatic by simplifying the nil check.
Consider this more idiomatic approach:
func (_m *FungibleBankKeeper) SpendableCoins(ctx types.Context, addr types.AccAddress) types.Coins { ret := _m.Called(ctx, addr) if len(ret) == 0 { panic("no return value specified for SpendableCoins") } var r0 types.Coins if rf, ok := ret.Get(0).(func(types.Context, types.AccAddress) types.Coins); ok { r0 = rf(ctx, addr) - } else { - if ret.Get(0) != nil { - r0 = ret.Get(0).(types.Coins) - } + } else if ret.Get(0) != nil { + r0 = ret.Get(0).(types.Coins) } return r0 }x/crosschain/simulation/decoders.go (2)
13-15: Enhance function documentationConsider expanding the documentation to include:
- The format of the returned string
- Possible panic conditions
- Examples of usage in simulation context
// NewDecodeStore returns a decoder function closure that unmarshals the KVPair's // Value to the corresponding crosschain types. +// The returned function compares two KVPairs and returns a formatted string +// representation of their decoded values. It panics if an invalid key prefix +// is encountered. +// +// Example output format: +// "{value1}\n{value2}"
17-61: Consider refactoring repeated unmarshal patternThe current implementation has significant code duplication across cases. Consider extracting the common unmarshal pattern into a generic helper function.
+func decodeKVPair[T any](cdc codec.Codec, kvA, kvB kv.Pair) (T, T) { + var valueA, valueB T + cdc.MustUnmarshal(kvA.Value, &valueA) + cdc.MustUnmarshal(kvB.Value, &valueB) + return valueA, valueB +} func NewDecodeStore(cdc codec.Codec) func(kvA, kvB kv.Pair) string { return func(kvA, kvB kv.Pair) string { switch { case bytes.Equal(kvA.Key, types.KeyPrefix(types.CCTXKey)): - var cctxA, cctxB types.CrossChainTx - cdc.MustUnmarshal(kvA.Value, &cctxA) - cdc.MustUnmarshal(kvB.Value, &cctxB) + cctxA, cctxB := decodeKVPair[types.CrossChainTx](cdc, kvA, kvB) return fmt.Sprintf("%v\n%v", cctxA, cctxB)testutil/sample/sample.go (1)
64-67: Consider making byte length configurable.Following the pattern of
StringRandom, consider adding a length parameter to make the function more flexible for different testing scenarios.-func RandomBytes(r *rand.Rand) []byte { +func RandomBytes(r *rand.Rand, length int) []byte { b := make([]byte, 10) _, _ = r.Read(b) return b }This would provide more utility for various testing scenarios while maintaining consistency with other functions in the package like
StringRandom.testutil/keeper/emissions.go (1)
57-58: LGTM! Consider documenting the new dependencies.The addition of
BankKeeperandAuthKeeperdependencies to the observer keeper initialization is well-structured and maintains consistency with the broader architectural changes.Consider adding a comment above the
initObserverKeepercall to document these new dependencies and their purpose:+ // Initialize observer keeper with banking and authentication capabilities observerKeeperTmp := initObserverKeeper(x/observer/simulation/decoders_test.go (3)
34-34: Remove or document commented code.Commented code can lead to confusion and maintenance issues. Either remove the commented line or add a comment explaining why it's temporarily disabled.
16-39: Consider refactoring test data setup.The test data setup could be more maintainable by using a table-driven approach for sample data generation. This would make it easier to add or modify test cases in the future.
Example refactor:
type testData struct { name string generator func() interface{} } testInputs := []testData{ {"crosschainFlags", func() interface{} { return sample.CrosschainFlags() }}, {"lastBlockObserverCount", func() interface{} { return sample.LastObserverCount(10) }}, // ... other test data } sampleData := make(map[string]interface{}) for _, td := range testInputs { sampleData[td.name] = td.generator() }
82-88: Add more comprehensive test assertions.While the current test execution is correct, consider adding:
- Validation that the decoder returns expected types
- Boundary testing with empty or nil values
- Performance benchmarks for the decoder
Example additions:
t.Run(tt.name, func(t *testing.T) { t.Parallel() result := dec(kvPairs.Pairs[i], kvPairs.Pairs[i]) require.Equal(t, tt.expectedLog, result) // Additional validations decoded := cdc.MustUnmarshal(kvPairs.Pairs[i].Value, &types.YourType{}) require.NotNil(t, decoded) require.IsType(t, &types.YourType{}, decoded) })x/crosschain/keeper/msg_server_vote_inbound_tx.go (1)
Line range hint
96-108: Enhance documentation and error handling for ballot finalization logic.While the early return pattern is good, consider these improvements:
- The comment could be more descriptive about why we return early and what "commit state" means.
- The error from ValidateInbound could benefit from more context in the error wrapping.
Consider this enhancement:
- // If the ballot is not finalized return nil here to add vote to commit state + // Return early if the ballot is not finalized, allowing the vote to be persisted + // in the commit state without proceeding to validation and processing if !finalized { return &types.MsgVoteInboundResponse{}, nil } cctx, err := k.ValidateInbound(ctx, msg, true) if err != nil { - return nil, sdkerrors.Wrap(err, voteInboundID) + return nil, sdkerrors.Wrapf(err, "%s: inbound validation failed for hash %s", + voteInboundID, msg.InboundHash) }Consider breaking down the VoteInbound function into smaller, more focused functions to improve maintainability:
- Ballot voting logic
- Validation logic
- State persistence logic
x/fungible/module.go (1)
Line range hint
82-85: Improve error handling in RegisterGRPCGatewayRoutes.The current error handling uses
fmt.Printlnwhich is not suitable for production code. Consider proper error propagation or structured logging.func (AppModuleBasic) RegisterGRPCGatewayRoutes(clientCtx client.Context, mux *runtime.ServeMux) { - err := types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx)) - if err != nil { - fmt.Println("RegisterQueryHandlerClient err: %w", err) - } + if err := types.RegisterQueryHandlerClient(context.Background(), mux, types.NewQueryClient(clientCtx)); err != nil { + // Consider using your application's logger or error handling mechanism + clientCtx.Logger.Error("failed to register query handler client", "err", err) + } }x/observer/module.go (1)
34-34: Appropriate codec interface upgrade.The change from
codec.BinaryCodectocodec.Codecis a good architectural decision as it provides more flexibility in encoding/decoding operations while maintaining backward compatibility.This change aligns with Cosmos SDK's best practices by using the more generic
codec.Codecinterface, which supports both binary and JSON encoding methods, making the module more versatile for future encoding requirements.x/fungible/keeper/v2_deposits_test.go (3)
Line range hint
19-115: Consider enhancing test helper coverageThe helper functions are well-structured and documented. However, consider adding table-driven tests to cover edge cases, particularly for:
- Invalid contract addresses
- Zero amounts
- Malformed messages
Example structure:
func TestAssertTestDAppV2MessageAndAmount(t *testing.T) { tests := []struct { name string contract common.Address message string amount int64 expectError bool }{ {"valid case", validAddr, "test", 100, false}, {"zero address", common.Address{}, "test", 100, true}, {"zero amount", validAddr, "test", 0, false}, } // ... test execution }
Line range hint
117-283: Enhance test coverage and reduce duplicationWhile the test cases are comprehensive for positive scenarios, consider the following improvements:
- Extract common setup code:
func setupTestEnvironment(t *testing.T) ( k *keeper.Keeper, ctx sdk.Context, chainID uint64, zrc20 common.Address, ) { k, ctx, sdkk, _ := keepertest.FungibleKeeper(t) _ = k.GetAuthKeeper().GetModuleAccount(ctx, types.ModuleName) chainID = chains.DefaultChainsList()[0].ChainId deploySystemContracts(t, ctx, k, sdkk.EvmKeeper) zrc20 = setupGasCoin(t, ctx, k, sdkk.EvmKeeper, chainID, "foobar", "foobar") return }
- Add negative test cases:
t.Run("should fail with invalid zrc20 address", func(t *testing.T) {...}) t.Run("should fail with insufficient balance", func(t *testing.T) {...}) t.Run("should fail with invalid chain ID", func(t *testing.T) {...})
- Consider using test tables for input variations:
tests := []struct { name string sender []byte chainID uint64 amount *big.Int message []byte coinType coin.CoinType wantCall bool wantErr bool }{ // ... test cases }
Test coverage needs improvement for error scenarios
The test suite for
ProcessV2Depositlacks coverage for important error cases:
- Missing test for ZETA asset deposit rejection
- No tests for invalid coin types or edge cases
- Error handling paths are not fully covered
The current test suite covers the happy paths well with four test cases:
- No-call deposit
- No-call deposit with ignored message
- Deposit with contract call
- Deposit with contract call and no message
However, it should be extended to include error scenarios, particularly the ZETA asset rejection case that's explicitly handled in the implementation. Consider adding test cases for:
- ZETA asset deposits (should fail)
- Invalid/unsupported coin types
- Edge cases with zero amounts or invalid addresses
🔗 Analysis chain
Line range hint
1-283: Verify test coverage metricsThe test implementation is solid and follows good testing practices. To ensure comprehensive coverage:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for the fungible module # Expected: High coverage for deposit processing logic # Check test coverage go test -coverprofile=coverage.out ./x/fungible/keeper/ go tool cover -func=coverage.out | grep -E "ProcessV2Deposit|Total:"Length of output: 268
Script:
#!/bin/bash # Let's verify the test coverage by analyzing the test cases and their implementation # First, check for the implementation of ProcessV2Deposit ast-grep --pattern 'func (k *Keeper) ProcessV2Deposit($$$) ($$$)' # Then check for all test cases rg "TestKeeper_ProcessV2Deposit" -A 2Length of output: 343
Script:
#!/bin/bash # Check for any other tests that might be testing the same functionality rg "ProcessV2Deposit" -B 1 -A 1Length of output: 1619
Script:
#!/bin/bash # Verify test scenarios coverage ast-grep --pattern 'func TestKeeper_ProcessV2Deposit($$$) { $$$ }'Length of output: 10321
Script:
#!/bin/bash # Let's verify the implementation to ensure all test cases cover the main functionality ast-grep --pattern 'func (k Keeper) ProcessV2Deposit($$$) { $$$ }' # Also check for any error cases in the implementation rg "return.*error" -B 2 x/fungible/keeper/v2_deposits.goLength of output: 388
testutil/keeper/observer.go (1)
112-113: Consider adding mock options for bank and auth keepers.While the implementation is correct, consider extending the
ObserverMockOptionsstructure to include options for mocking bank and auth keepers, maintaining consistency with other keeper mocks.type ObserverMockOptions struct { UseStakingMock bool UseSlashingMock bool UseAuthorityMock bool UseLightclientMock bool + UseBankMock bool + UseAuthMock bool } var ( ObserverMocksAll = ObserverMockOptions{ UseStakingMock: true, UseSlashingMock: true, UseAuthorityMock: true, UseLightclientMock: true, + UseBankMock: true, + UseAuthMock: true, } ObserverNoMocks = ObserverMockOptions{} )Also applies to: 135-136
app/modules.go (1)
Issue #3007 referenced in the TODO comment does not exist, and the existing issue #3021 is unrelated to gov module simulation
The TODO comment references a non-existent issue (#3007) for enabling gov module simulation. This could lead to the task being overlooked. Consider either:
- Creating a new issue specifically for enabling gov module simulation
- Updating the TODO comment with the correct issue number if one exists
🔗 Analysis chain
Line range hint
166-174: Consider creating a tracking issue for enabling gov module simulation.The TODO comment indicates that gov module simulation needs to be enabled. To ensure this doesn't get overlooked, consider creating a dedicated issue if #3007 doesn't already cover this specific task.
Let me search for the TODO comment about gov module simulation to verify this concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if issue #3007 covers enabling gov module simulation gh issue view 3007Length of output: 1387
Script:
#!/bin/bash # Search for TODO comments related to gov module simulation rg -i "todo.*gov.*simulation" -A 2Length of output: 196
testutil/sample/observer.go (3)
Line range hint
124-137: Consider improving error handling and magic numbers.A few suggestions to enhance the code:
- Consider using
t *testing.Tparameter instead of panic for better test failure reporting- Extract magic numbers (1000) into named constants with clear intent
-func Tss() types.TSS { +const ( + defaultFinalizedHeight = 1000 // Initial finalized height for test TSS + defaultKeyGenHeight = 1000 // Initial key generation height for test TSS +) + +func Tss(t *testing.T) types.TSS { _, pubKey, _ := testdata.KeyTestPubAddr() spk, err := cosmos.Bech32ifyPubKey(cosmos.Bech32PubKeyTypeAccPub, pubKey) if err != nil { - panic(err) + t.Fatalf("failed to bech32ify pubkey: %v", err) } pk, err := zetacrypto.NewPubKey(spk) if err != nil { - panic(err) + t.Fatalf("failed to create new pubkey: %v", err) } pubkeyString := pk.String() return types.TSS{ TssPubkey: pubkeyString, - FinalizedZetaHeight: 1000, - KeyGenZetaHeight: 1000, + FinalizedZetaHeight: defaultFinalizedHeight, + KeyGenZetaHeight: defaultKeyGenHeight, } }
Line range hint
139-147: Update function signature to match Tss() changes.If the suggested changes to
Tss()are implemented, this function should be updated accordingly.-func TssList(n int) (tssList []types.TSS) { +func TssList(t *testing.T, n int) (tssList []types.TSS) { for i := 0; i < n; i++ { - tss := Tss() + tss := Tss(t) tss.FinalizedZetaHeight = tss.FinalizedZetaHeight + int64(i) tss.KeyGenZetaHeight = tss.KeyGenZetaHeight + int64(i) tssList = append(tssList, tss)
Line range hint
149-154: Consider parameterizing the sample index.The hardcoded "sampleIndex" could be made configurable for more flexible testing scenarios.
-func TssFundsMigrator(chainID int64) types.TssFundMigratorInfo { +func TssFundsMigrator(chainID int64, index string) types.TssFundMigratorInfo { return types.TssFundMigratorInfo{ ChainId: chainID, - MigrationCctxIndex: "sampleIndex", + MigrationCctxIndex: index, } }testutil/keeper/fungible.go (1)
Line range hint
1-266: Consider enhancing documentation and type safety.While the implementation is robust, consider these improvements:
- Add documentation for
FungibleMockOptionsto explain when each mock should be used- Consider using an enum or constants for mock configurations instead of boolean flags
Example improvement:
// MockOptionType defines the type of mock to be used type MockOptionType int const ( // MockNone indicates no mocking should be used MockNone MockOptionType = iota // MockReal indicates real implementation should be used MockReal // MockSimulated indicates simulated implementation should be used MockSimulated ) // FungibleMockOptions defines which keepers should be mocked and how type FungibleMockOptions struct { BankKeeper MockOptionType AccountKeeper MockOptionType ObserverKeeper MockOptionType EVMKeeper MockOptionType AuthorityKeeper MockOptionType }x/crosschain/keeper/msg_server_vote_inbound_tx_test.go (2)
Line range hint
55-67: Consider improving chain ID initialization logicThe current implementation uses hardcoded chain IDs with a fallback to dynamic selection. This could be simplified and made more maintainable.
Consider refactoring to:
- to, from := int64(1337), int64(101) - supportedChains := zk.ObserverKeeper.GetSupportedChains(ctx) - for _, chain := range supportedChains { - if chains.IsEVMChain(chain.ChainId, []chains.Chain{}) { - from = chain.ChainId - } - if chains.IsZetaChain(chain.ChainId, []chains.Chain{}) { - to = chain.ChainId - } - } + supportedChains := zk.ObserverKeeper.GetSupportedChains(ctx) + from := supportedChains.FindFirst(chains.IsEVMChain).ChainId + to := supportedChains.FindFirst(chains.IsZetaChain).ChainId + require.NotZero(t, from, "No EVM chain found in supported chains") + require.NotZero(t, to, "No Zeta chain found in supported chains")
Line range hint
54-93: Enhance test assertions for successful vote caseWhile the test covers the basic flow, consider adding assertions for important state changes:
Add these assertions after the existing ones:
// Verify state transitions require.Equal(t, msg.Amount, cctx.InboundParams.Amount, "Amount mismatch") require.Equal(t, msg.Sender, cctx.InboundParams.Sender, "Sender mismatch") require.Equal(t, msg.Receiver, cctx.GetCurrentOutboundParam().Receiver, "Receiver mismatch") // Verify event emissions events := ctx.EventManager().Events() require.True(t, containsEvent(events, "vote_inbound"), "Missing vote_inbound event")testutil/keeper/crosschain.go (3)
Line range hint
289-307: Consider enhancing MockPayGasAndUpdateCCTX with error handling.The function sets up multiple mock expectations but doesn't provide a way to test error scenarios. Consider adding parameters to control which operations should fail and how they should fail.
Example enhancement:
func MockPayGasAndUpdateCCTX( m *crosschainmocks.CrosschainFungibleKeeper, m2 *crosschainmocks.CrosschainObserverKeeper, ctx sdk.Context, k keeper.Keeper, senderChain chains.Chain, - asset string, + asset string, + opts struct { + FailGasLimit bool + FailDeposit bool + CustomError error + }, ) {
Line range hint
401-425: Add documentation for mock call sequence dependencies.The mock ballot voting functions (
MockVoteOnOutboundSuccessBallotandMockVoteOnOutboundFailedBallot) are part of a sequence of operations. Consider adding documentation to clarify the order in which these mocks should be called.Add documentation like:
+// MockVoteOnOutboundSuccessBallot sets up mock expectations for a successful outbound ballot vote. +// This should be called after setting up the CCTX and before calling the handler being tested. func MockVoteOnOutboundSuccessBallot( m *crosschainmocks.CrosschainObserverKeeper, ctx sdk.Context, cctx *types.CrossChainTx, senderChain chains.Chain, observer string, ) {
Line range hint
466-499: Enhance MockCctxByNonce with input validation.The function accepts a boolean
isErrto control error scenarios, but it could be more explicit about the error conditions and provide validation for the input parameters.Consider enhancing the function:
func MockCctxByNonce( t *testing.T, ctx sdk.Context, k keeper.Keeper, observerKeeper *crosschainmocks.CrosschainObserverKeeper, cctxStatus types.CctxStatus, - isErr bool, + opts struct { + ErrorScenario string + CustomError error + }, ) { + // Validate inputs + require.NotNil(t, ctx) + require.NotNil(t, k) + require.NotNil(t, observerKeeper)contrib/docker-scripts/start.sh (4)
Line range hint
4-7: Add environment variable validation and error handling.The script should validate required environment variables and handle errors gracefully. This is particularly important for production environments.
Consider adding these improvements:
function load_defaults { + # Validate required environment variables + required_vars=("DAEMON_HOME" "NETWORK" "RESTORE_TYPE") + for var in "${required_vars[@]}"; do + if [[ -z "${!var}" ]]; then + echo "Error: Required environment variable $var is not set" + exit 1 + fi + done + #DEFAULT: Mainnet Statesync. export DAEMON_HOME=${DAEMON_HOME:=/root/.zetacored} export NETWORK=${NETWORK:=mainnet} export RESTORE_TYPE=${RESTORE_TYPE:=statesync}function init_chain { + set -e # Exit on error + trap 'echo "Error: Command failed at line $LINENO"' ERR + if [ -d "${DAEMON_HOME}/config" ]; then logt "${DAEMON_NAME} home directory already initialized." else logt "${DAEMON_NAME} home directory not initialized." logt "MONIKER: ${MONIKER}" logt "DAEMON_HOME: ${DAEMON_HOME}" - ${DAEMON_NAME} init ${MONIKER} --home ${DAEMON_HOME} --chain-id ${CHAIN_ID} + if ! ${DAEMON_NAME} init ${MONIKER} --home ${DAEMON_HOME} --chain-id ${CHAIN_ID}; then + logt "Failed to initialize chain" + exit 1 + fi fi }Also applies to: 73-76
Line range hint
78-102: Enhance security for configuration file downloads.The current implementation downloads configuration files without verifying their integrity, which could expose the system to supply chain attacks.
Consider implementing these security measures:
function download_configs { + # Verify HTTPS certificates + CURL_OPTS="--tlsv1.2 --cert-status" + if [ "${NETWORK}" == "mainnet" ]; then - wget -q ${APP_TOML_FILE_MAINNET} -O ${DAEMON_HOME}/config/app.toml - wget -q ${CONFIG_TOML_FILE_MAINNET} -O ${DAEMON_HOME}/config/config.toml + # Download with checksum verification + for file in "app.toml" "config.toml" "client.toml" "genesis.json"; do + url="https://raw.githubusercontent.com/zeta-chain/network-config/main/mainnet/${file}" + checksum_url="${url}.sha256" + + # Download file and its checksum + curl ${CURL_OPTS} -o "${DAEMON_HOME}/config/${file}" "${url}" + curl ${CURL_OPTS} -o "/tmp/${file}.sha256" "${checksum_url}" + + # Verify checksum + if ! sha256sum -c "/tmp/${file}.sha256"; then + logt "Checksum verification failed for ${file}" + exit 1 + fi + done
Line range hint
267-290: Improve version comparison and binary verification.The current version comparison logic could be more robust to handle different version formats and ensure binary integrity.
Consider these improvements:
function start_network { + # Semantic version comparison function + version_compare() { + if [[ $1 == $2 ]]; then + return 0 + fi + local IFS=. + local i ver1=($1) ver2=($2) + for ((i=${#ver1[@]}; i<${#ver2[@]}; i++)); do + ver1[i]=0 + done + for ((i=0; i<${#ver1[@]}; i++)); do + if [[ -z ${ver2[i]} ]]; then + ver2[i]=0 + fi + if ((10#${ver1[i]} > 10#${ver2[i]})); then + return 1 + fi + if ((10#${ver1[i]} < 10#${ver2[i]})); then + return 2 + fi + done + return 0 + } expected_major_version=$(cat /scripts/expected_major_version | cut -d '-' -f 1) VISOR_VERSION=v$(${VISOR_NAME} version | tail -n 1 | tr -d '(devel)' | tr -d '\n') DAEMON_VERSION=$(${DAEMON_NAME} version) - VISOR_MAJOR_VERSION=$(echo $VISOR_VERSION | grep -o '^v[0-9]*') - DAEMON_MAJOR_VERSION=$(echo $DAEMON_VERSION | grep -o '^v[0-9]*') + # Extract clean version numbers + VISOR_MAJOR_VERSION=$(echo $VISOR_VERSION | sed 's/^v\([0-9]*\.[0-9]*\.[0-9]*\).*/\1/') + DAEMON_MAJOR_VERSION=$(echo $DAEMON_VERSION | sed 's/^v\([0-9]*\.[0-9]*\.[0-9]*\).*/\1/')
Line range hint
291-299: Enhance network startup configuration and monitoring.The network startup process uses hardcoded values and lacks proper health checks.
Consider these improvements:
+ # Health check function + wait_for_node_ready() { + local max_attempts=30 + local attempt=1 + while [ $attempt -le $max_attempts ]; do + if curl -s "http://localhost:26657/status" > /dev/null; then + logt "Node is ready" + return 0 + fi + logt "Waiting for node to start... (attempt $attempt/$max_attempts)" + sleep 2 + ((attempt++)) + done + logt "Node failed to start within expected time" + return 1 + } ${VISOR_NAME} run start --home ${DAEMON_HOME} \ --log_level info \ --moniker ${MONIKER} \ --rpc.laddr tcp://0.0.0.0:26657 \ - --minimum-gas-prices 1.0azeta "--grpc.enable=true" + --minimum-gas-prices ${MIN_GAS_PRICE:-1.0azeta} \ + --grpc.enable=true + # Wait for node to be ready + if ! wait_for_node_ready; then + logt "Failed to start node" + exit 1 + fisimulation/simulation_test.go (5)
46-48: Document the StoreKeysPrefixes struct field rename.The field rename from
PrefixestoSkipPrefixessuggests a change in behavior. Please add a comment explaining the purpose of this field and why certain prefixes are skipped during store comparison.
85-87: Consider documenting the test configuration rationale.The reduction in seeds from 3 to 2 with 5 runs per seed represents a trade-off between test coverage and execution time. Please add a comment explaining this decision to help future maintainers understand the chosen parameters.
Line range hint
416-428: Enhance store comparison error reporting.The current store comparison could benefit from more detailed error reporting. Consider adding debug information about the specific key-value pairs that differ.
- failedKVAs, failedKVBs := sdk.DiffKVStores(storeA, storeB, skp.SkipPrefixes) - require.Equal(t, len(failedKVAs), len(failedKVBs), "unequal sets of key-values to compare") + failedKVAs, failedKVBs := sdk.DiffKVStores(storeA, storeB, skp.SkipPrefixes) + require.Equal(t, len(failedKVAs), len(failedKVBs), fmt.Sprintf( + "store %s: unequal sets of key-values to compare (failed A: %d, failed B: %d)", + skp.A.Name(), len(failedKVAs), len(failedKVBs), + ))
549-552: Document AppStateFn parameter usage.The use of multiple nil parameters followed by exported.AppState needs clarification. Please add a comment explaining why these parameters are nil and how they affect the simulation.
Line range hint
1-562: LGTM! Consider improving test organization.The simulation tests are comprehensive and well-structured. However, consider extracting common setup code into helper functions to reduce duplication across test functions. This would improve maintainability and make the test flow clearer.
testutil/sample/crosschain.go (2)
Line range hint
278-297: Enhance test coverage by improving theInboundVotefunction.The function could be improved to handle more test scenarios:
- The empty Creator field might cause issues in tests where creator validation is required.
- The hardcoded gas limit might not cover edge cases.
Apply this diff to improve the function:
func InboundVote(coinType coin.CoinType, from, to int64) types.MsgVoteInbound { return types.MsgVoteInbound{ - Creator: "", + Creator: AccAddress(), // Use sample address for better test coverage Sender: EthAddress().String(), SenderChainId: Chain(from).ChainId, Receiver: EthAddress().String(), ReceiverChain: Chain(to).ChainId, Amount: UintInRange(10000000, 1000000000), Message: base64.StdEncoding.EncodeToString(Bytes()), InboundBlockHeight: Uint64InRange(1, 10000), CallOptions: &types.CallOptions{ - GasLimit: 1000000000, + GasLimit: Uint64InRange(21000, 1000000000), // Vary gas limit for better coverage }, InboundHash: Hash().String(), CoinType: coinType, TxOrigin: EthAddress().String(), - Asset: "", + Asset: StringRandom(Rand(), 32), // Add random asset for better coverage EventIndex: EventIndex(), } }
Line range hint
278-320: Improve documentation and error scenario coverage.The new functions would benefit from:
- Better documentation explaining the relationship between
fromandtochain parameters- Test cases for error scenarios (e.g., invalid chain IDs)
Add comprehensive documentation:
+// InboundVote creates a sample inbound vote message for testing cross-chain transactions. +// Parameters: +// - coinType: The type of coin being transferred +// - from: Source chain ID for the transaction +// - to: Destination chain ID for the transaction +// Returns a MsgVoteInbound with randomized but valid data for testing happy paths. func InboundVote(coinType coin.CoinType, from, to int64) types.MsgVoteInbound { +// InboundVoteSim creates a simulated inbound vote message for testing cross-chain transactions. +// It uses the provided random source to generate deterministic but random-looking test data. +// Parameters: +// - coinType: The type of coin being transferred +// - from: Source chain ID for the transaction +// - to: Destination chain ID for the transaction +// - r: Random source for generating deterministic test data +// Returns a MsgVoteInbound with deterministic random data for simulation testing. func InboundVoteSim(coinType coin.CoinType, from, to int64, r *rand.Rand) types.MsgVoteInbound {changelog.md (1)
11-11: LGTM! Consider enhancing the description.The changelog entry is well-formatted and correctly placed under the "Tests" section. The description accurately reflects the PR's purpose.
Consider expanding the description to provide more context about the simulation tests' scope and purpose:
-* [3095](https://github.com/zeta-chain/node/pull/3095) - initialize simulation tests for custom zetachain modules +* [3095](https://github.com/zeta-chain/node/pull/3095) - initialize simulation tests for custom zetachain modules to validate module interactions and state transitionsx/observer/simulation/operations.go (5)
20-20: Correct the typo in the commentThere's a typographical error in the comment: "100 seems to the max weight used" should be "100 seems to be the max weight used".
Apply this diff to fix the typo:
- // 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. + // Based on the weights assigned in the Cosmos SDK modules, 100 seems to be the max weight used, and therefore guarantees that at least one operation of that type is present in a block.
21-22: Address the TODO comment or remove itThe code contains a TODO comment indicating that more details should be added about what the number represents in terms of the percentage of operations in a block. Please provide the necessary clarification or remove the TODO if it's no longer applicable.
Would you like assistance in enhancing the comment to explain how the operation weight affects the simulation?
26-26: Fix the typographical error in the commentThere is a typographical error: "We ues a high weight" should be "We use a high weight".
Apply this diff to correct the typo:
- // DefaultWeightMsgTypeMsgEnableCCTX We ues a high weight for this operation + // DefaultWeightMsgTypeMsgEnableCCTX: We use a high weight for this operation
85-85: Optimize encoding configuration initializationCurrently,
TxGenis initialized with a new encoding configuration in each operation. To improve performance, consider initializing the encoding configuration once outside the function and reusing it.For example, initialize
TxGenonce and pass it as a parameter:// Outside the function encodingConfig := moduletestutil.MakeTestEncodingConfig() txGen := encodingConfig.TxConfig // Modify the function signature to accept txGen func SimulateMsgTypeMsgEnableCCTX(k keeper.Keeper, txGen client.TxConfig) simtypes.Operation { // ... txCtx := simulation.OperationInput{ // ... - TxGen: moduletestutil.MakeTestEncodingConfig().TxConfig, + TxGen: txGen, // ... } // ... }
92-92: Ensure consistent naming forBankKeeperfieldThe field
Bankkeepershould be renamed toBankKeeperto adhere to Go naming conventions and ensure consistency.Apply this diff to correct the field name:
- Bankkeeper: k.GetBankKeeper(), + BankKeeper: k.GetBankKeeper(),simulation/state.go (2)
140-143: Log errors when obtaining account addresses from operator addressesThe errors encountered in
GetAccAddressFromOperatorAddressare ignored usingcontinue, which may obscure underlying issues during simulation. For improved observability and debugging, consider logging these errors.Apply the following change to log the errors:
if err != nil { + fmt.Printf("Warning: Failed to convert operator address %s to account address: %v\n", validator.OperatorAddress, err) continue }Ensure the
fmtpackage is imported.
179-202: Review the assignment of all policies to a single random accountAssigning all policy groups to a single random account may not reflect realistic authority distributions and could affect the validity of simulation outcomes. It is advisable to assign different policy groups to separate accounts to better simulate diverse administrative roles.
x/crosschain/simulation/operations.go (2)
28-29: Complete the TODO: Elaborate on operation weight percentagesThe comment suggests adding more details about what the numbers represent regarding the percentage of operations in a block. Providing this information will enhance code clarity and assist future maintainers.
215-216: Address the TODO: Implement randomization of test valuesCurrently, the values used for CCTX creation are constant. Introducing randomization will improve the robustness and coverage of the simulation tests by simulating a wider range of scenarios.
Would you like assistance in implementing randomized values for these test parameters?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (57)
Makefile(1 hunks)app/app.go(1 hunks)app/modules.go(1 hunks)changelog.md(1 hunks)codecov.yml(1 hunks)contrib/docker-scripts/start.sh(1 hunks)server/start.go(4 hunks)simulation/simulation_test.go(11 hunks)simulation/state.go(4 hunks)testutil/keeper/crosschain.go(1 hunks)testutil/keeper/emissions.go(1 hunks)testutil/keeper/fungible.go(1 hunks)testutil/keeper/ibccrosschain.go(1 hunks)testutil/keeper/mocks/crosschain/account.go(1 hunks)testutil/keeper/mocks/crosschain/bank.go(1 hunks)testutil/keeper/mocks/fungible/authority.go(2 hunks)testutil/keeper/mocks/fungible/bank.go(1 hunks)testutil/keeper/mocks/observer/authority.go(1 hunks)testutil/keeper/observer.go(5 hunks)testutil/sample/crosschain.go(3 hunks)testutil/sample/observer.go(1 hunks)testutil/sample/sample.go(1 hunks)x/crosschain/keeper/cctx_orchestrator_validate_inbound.go(0 hunks)x/crosschain/keeper/msg_server_vote_inbound_tx.go(2 hunks)x/crosschain/keeper/msg_server_vote_inbound_tx_test.go(2 hunks)x/crosschain/module_simulation.go(2 hunks)x/crosschain/simulation/decoders.go(1 hunks)x/crosschain/simulation/decoders_test.go(1 hunks)x/crosschain/simulation/genesis.go(1 hunks)x/crosschain/simulation/operations.go(1 hunks)x/crosschain/simulation/simap.go(0 hunks)x/crosschain/types/expected_keepers.go(1 hunks)x/fungible/keeper/keeper.go(3 hunks)x/fungible/keeper/msg_server_update_gateway_contract_test.go(1 hunks)x/fungible/keeper/v2_deposits_test.go(1 hunks)x/fungible/module.go(1 hunks)x/fungible/module_simulation.go(2 hunks)x/fungible/simulation/decoders.go(1 hunks)x/fungible/simulation/decoders_test.go(1 hunks)x/fungible/simulation/genesis.go(1 hunks)x/fungible/simulation/operations.go(1 hunks)x/fungible/simulation/simap.go(0 hunks)x/fungible/types/expected_keepers.go(3 hunks)x/fungible/types/genesis.go(1 hunks)x/observer/keeper/keeper.go(4 hunks)x/observer/keeper/msg_server_disable_cctx_flags.go(0 hunks)x/observer/keeper/tss.go(1 hunks)x/observer/module.go(1 hunks)x/observer/module_simulation.go(2 hunks)x/observer/simulation/decoders.go(1 hunks)x/observer/simulation/decoders_test.go(1 hunks)x/observer/simulation/genesis.go(1 hunks)x/observer/simulation/operations.go(1 hunks)x/observer/simulation/simap.go(0 hunks)x/observer/types/expected_keepers.go(3 hunks)x/observer/types/genesis.go(1 hunks)x/observer/types/keys.go(2 hunks)
💤 Files with no reviewable changes (5)
- x/crosschain/keeper/cctx_orchestrator_validate_inbound.go
- x/crosschain/simulation/simap.go
- x/fungible/simulation/simap.go
- x/observer/keeper/msg_server_disable_cctx_flags.go
- x/observer/simulation/simap.go
✅ Files skipped from review due to trivial changes (6)
- server/start.go
- x/crosschain/simulation/genesis.go
- x/fungible/keeper/msg_server_update_gateway_contract_test.go
- x/fungible/simulation/genesis.go
- x/observer/keeper/tss.go
- x/observer/simulation/genesis.go
🧰 Additional context used
📓 Path-based instructions (43)
app/app.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
app/modules.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
contrib/docker-scripts/start.sh (1)
Pattern **/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.
simulation/simulation_test.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/keeper/crosschain.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/emissions.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/fungible.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/ibccrosschain.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/mocks/crosschain/account.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/mocks/crosschain/bank.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/mocks/fungible/authority.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/mocks/fungible/bank.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/mocks/observer/authority.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
testutil/keeper/observer.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.
testutil/sample/observer.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_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/keeper/msg_server_vote_inbound_tx_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/crosschain/module_simulation.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/simulation/decoders_test.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/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/fungible/keeper/keeper.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/keeper/v2_deposits_test.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/module.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/module_simulation.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/simulation/decoders.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/simulation/decoders_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/fungible/types/expected_keepers.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/fungible/types/genesis.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/keeper/keeper.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/module.go (1)
Pattern **/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
x/observer/module_simulation.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/observer/simulation/decoders_test.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.
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/types/genesis.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.
🔇 Additional comments (52)
x/fungible/simulation/decoders.go (1)
1-11: LGTM: Clean package structure and imports.
The package organization follows Go conventions, and imports are well-structured with clear separation between standard library and project-specific packages.
x/observer/module_simulation.go (2)
8-9: LGTM: Clean import organization
The addition of the simulation package import is well-organized and properly aligned with the module's refactoring.
27-29: Well-structured store decoder registration
The implementation properly follows Cosmos SDK patterns and improves type safety with the explicit StoreDecoderRegistry parameter.
x/fungible/module_simulation.go (3)
8-8: LGTM: Import statement is correctly placed
The new import for the simulation package is properly organized and follows Go conventions.
14-14: Verify test coverage for RandomizedGenState
The implementation has been simplified, which is good. However, we should ensure proper test coverage exists for simulation.RandomizedGenState.
✅ Verification successful
RandomizedGenState implementation is properly tested and consistent
The implementation is well-structured and follows the same pattern used across other modules (crosschain, observer). The RandomizedGenState function in x/fungible/simulation/genesis.go is appropriately implemented:
- It uses
DefaultGenesis()which is a safe approach for simulation - The implementation is consistent with other modules in the codebase
- The function properly marshals the genesis state into the simulation state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for test coverage of RandomizedGenState
rg -A 5 "func TestRandomizedGenState" "x/fungible/simulation/"
Length of output: 62
Script:
#!/bin/bash
# Let's first check if the file exists and its content
fd "module_simulation.go" --type f
# Check for any test files related to simulation
fd "simulation.*test.go" --type f
# Let's also check for RandomizedGenState usage across the codebase
rg -A 5 "RandomizedGenState"
Length of output: 2958
32-35: Verify parameter usage in WeightedOperations
The implementation looks clean and follows good practices. Let's verify that all passed parameters are properly utilized in the simulation package.
✅ Verification successful
Parameters are properly utilized in WeightedOperations implementation
The verification confirms that all parameters passed to WeightedOperations are correctly utilized:
appParamsis used to generate weights viaGetOrGeneratecdcis passed toGetOrGeneratefor parameter handlingkeeper(k) is used in simulation message generation
The implementation follows best practices for cosmos-sdk simulation modules with proper parameter injection and usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of WeightedOperations
ast-grep --pattern 'func WeightedOperations($params, $cdc, $keeper) {
$$$
}'
Length of output: 80
Script:
#!/bin/bash
# Search for WeightedOperations function in simulation package
rg -A 10 "func WeightedOperations" x/fungible/simulation/
# Find all files in simulation directory to understand structure
fd . x/fungible/simulation/
# Search for any usage of AppParams, Cdc, and keeper in simulation files
rg "AppParams|Cdc|keeper" x/fungible/simulation/
Length of output: 1886
x/crosschain/module_simulation.go (3)
8-9: LGTM: Clean import organization
The new simulation package import is well-organized and properly scoped.
27-29: LGTM: Proper store decoder registration
The implementation correctly integrates with the SDK's simulation framework using the store decoder registry.
32-35: Verify operation coverage in WeightedOperations
The implementation is clean, but we should ensure all necessary cross-chain operations are covered in the simulation.
✅ Verification successful
WeightedOperations implementation is comprehensive and well-structured
The implementation covers all necessary cross-chain operations with appropriate simulation weights:
- Inbound/Outbound tracking and voting
- Gas price voting
- ERC20 whitelisting
- TSS-related operations (funds migration, address updates)
- Rate limiter configuration
- Stuck CCTX handling
The operations are properly weighted and include comprehensive test scenarios (e.g., different voting thresholds: 40%, 75%, 90%, 100%).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the WeightedOperations implementation coverage
# Expected: Should find comprehensive operation definitions
# Look for operation definitions in the simulation package
rg -A 10 'func WeightedOperations' simulation/
# Check for different operation types
rg -l 'NewWeightedOperation' simulation/
Length of output: 89
Script:
#!/bin/bash
# Let's try a broader search to understand the simulation operations structure
# First, find all simulation-related files
fd simulation
# Then look for operation-related code patterns
rg -A 5 "type.*Operation"
# Search for weighted operation implementations
rg -A 5 "WeightedOperation"
# Look specifically in the crosschain module
fd . x/crosschain -t f | xargs rg -l "Operation"
Length of output: 50666
Script:
#!/bin/bash
# Let's examine the crosschain simulation operations in detail
# Look at the operations implementation
rg -A 10 'func WeightedOperations' x/crosschain/simulation/operations.go
# Check the operation weights and their initialization
rg -A 5 'weight.*int' x/crosschain/simulation/operations.go
# Look for simulation operation implementations
rg -A 5 'func Simulate' x/crosschain/simulation/operations.go
Length of output: 1855
x/fungible/simulation/decoders_test.go (1)
1-13: LGTM: Package and imports are well-organized.
The package declaration and imports follow Go best practices.
codecov.yml (1)
55-55: Review the coverage pattern change impact.
The modification of the ignore pattern from x/**/simulation/**/* to x/**/simulation/*.go represents a significant change in coverage tracking. This change will now:
- Only exclude
.gofiles directly in simulation directories - Include files in simulation subdirectories in coverage metrics
- Potentially affect the overall coverage statistics
This aligns well with the PR's objective of initializing simulation tests, but we should verify the impact.
Let's check the simulation directory structure to understand the coverage impact:
Consider documenting this coverage pattern change in the PR description to inform other contributors about the expanded test coverage scope.
x/fungible/keeper/keeper.go (2)
28-28: LGTM: Constructor signature properly updated.
The parameter type change is consistent with the struct field modification and maintains clean dependency injection.
16-16: Verify the codec type change impact across the codebase.
The change from codec.BinaryCodec to codec.Codec is a significant modification that could affect existing serialization/deserialization operations.
x/observer/types/genesis.go (2)
Line range hint 54-63: LGTM: Robust error handling in genesis state unmarshaling.
The GetGenesisStateFromAppState function properly handles JSON unmarshaling with appropriate error handling and panic recovery for critical initialization.
17-17: Verify the implications of non-nil Keygen initialization.
The change from nil to &Keygen{} for the default Keygen value could have implications for module initialization and state management. While initializing with an empty struct is generally safer than nil, we should ensure this doesn't affect existing chain upgrades.
✅ Verification successful
Let me analyze the implications of initializing Keygen with an empty struct by searching for its usage in state transitions.
Let me try a different approach to analyze the Keygen initialization and its implications.
Let me check the Keygen struct definition and its usage in genesis state to fully understand the implications.
Let me check the DefaultGenesis function specifically for the observer module.
Based on the analysis of the codebase, I can now provide a definitive response about the Keygen initialization:
Initializing Keygen with empty struct is safe and follows module's design pattern
The change from nil to &Keygen{} in DefaultGenesis is consistent with the module's behavior because:
- The Keygen struct is used to track TSS key generation status and contains fields for Status, GranteePubkeys, and BlockNumber
- The empty initialization is the correct default state, as evidenced by:
- In BeginBlocker (abci.go), Keygen is initialized with BlockNumber set to math.MaxInt64
- In AddObserver handler, same initialization pattern is used
- The Status field defaults to KeygenStatus_PendingKeygen (0) when empty
- The change maintains consistency with other genesis state fields that use empty initialization rather than nil
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Keygen field usage in state migrations and upgrade handlers
rg -A 5 "Keygen" --type go
Length of output: 182478
Script:
#!/bin/bash
# Search for state transitions involving Keygen
ast-grep --pattern 'func $_(ctx $_, keygen $_, $$$) {
$$$
}'
Length of output: 64
Script:
#!/bin/bash
# Search for Keygen initialization and state transitions
rg -A 5 "SetKeygen.*Keygen\{" --type go
Length of output: 897
Script:
#!/bin/bash
# Search for Keygen struct definition and genesis state handling
rg -A 10 "type Keygen struct" --type go
rg -A 10 "DefaultGenesis.*return.*Genesis" --type go
Length of output: 1069
Script:
#!/bin/bash
# Search for DefaultGenesis in observer module
rg -A 10 "func DefaultGenesis" x/observer/types/
Length of output: 709
testutil/keeper/mocks/crosschain/account.go (2)
17-35: Implementation follows mock best practices.
The GetAccount method implementation is well-structured and follows established patterns for mock objects:
- Proper error handling for missing return values
- Correct type assertions and return value handling
- Consistent with the mock framework's expectations
17-35: Verify interface compliance.
Let's ensure the mock implementation matches the actual interface requirements.
✅ Verification successful
Interface compliance verified successfully
The mock implementation in testutil/keeper/mocks/crosschain/account.go correctly matches the interface definition found in x/crosschain/types/expected_keepers.go. The method signature:
- Parameters:
(ctx types.Context, addr types.AccAddress) - Return type:
authtypes.AccountI
matches exactly with the interface requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the mock implementation matches the interface definition
# Expected: The GetAccount method signature in the interface should match the mock implementation
# Search for the interface definition
ast-grep --pattern 'type AccountKeeper interface {
$$$
GetAccount(ctx sdk.Context, addr sdk.AccAddress) types.AccountI
$$$
}'
Length of output: 1420
testutil/keeper/mocks/crosschain/bank.go (1)
52-70: Implementation follows mock patterns correctly.
The SpendableCoins method implementation adheres to the established mock patterns, with proper:
- Mock call registration
- Return value validation
- Type assertions for both function and direct value returns
- Error handling via panic for unspecified returns
x/observer/keeper/keeper.go (2)
16-16: LGTM: Well-structured keeper additions
The new fields follow Cosmos SDK conventions for dependency injection and maintain good field ordering within the struct.
Also applies to: 23-24
71-78: LGTM: Consistent getter implementations
The new getter methods maintain consistency with existing patterns and provide proper encapsulation.
x/observer/types/expected_keepers.go (2)
42-42: LGTM: Well-designed getter method
The GetPolicies method follows Go conventions and properly handles the case where policies might not exist.
63-65: LGTM: Clean BankKeeper interface
The interface is well-defined and follows cosmos-sdk conventions.
x/fungible/types/expected_keepers.go (2)
16-16: LGTM: Clean import addition
The addition of the authority types import is well-structured and necessary for the new GetPolicies method.
37-37: LGTM: Well-structured interface extensions
The additions of SpendableCoins and GetPolicies methods follow idiomatic Go patterns and Cosmos SDK conventions. These methods are essential for simulation testing, providing necessary functionality for balance checking and policy verification.
Let's verify the consistency of these interfaces across the codebase:
Also applies to: 67-67
✅ Verification successful
Interface methods are consistently defined across dependent modules
The verification confirms that both SpendableCoins and GetPolicies methods are consistently defined with identical signatures across multiple modules:
-
SpendableCoinsis uniformly defined in:- x/fungible/types/expected_keepers.go
- x/observer/types/expected_keepers.go
- x/crosschain/types/expected_keepers.go
-
GetPoliciesis consistently defined in:- x/fungible/types/expected_keepers.go
- x/observer/types/expected_keepers.go
The interface extensions maintain consistency in method signatures across dependent modules, following proper SDK patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify interface method implementations and usage
# Check for SpendableCoins implementation
rg -A 2 "SpendableCoins.*sdk\.Context.*sdk\.AccAddress.*sdk\.Coins"
# Check for GetPolicies implementation
rg -A 2 "GetPolicies.*sdk\.Context.*authoritytypes\.Policies"
Length of output: 1066
testutil/keeper/mocks/fungible/authority.go (3)
7-9: LGTM: Import changes are appropriate.
The added import for authoritytypes is necessary for the new GetPolicies function implementation.
57-83: LGTM: Mock implementation follows best practices.
The GetPolicies implementation:
- Correctly handles both tuple and single return value cases
- Implements proper type assertions and error handling
- Follows mockery's standard patterns for mock implementations
57-83: Verify consistent mock usage across test files.
Let's ensure this mock is being used consistently in test implementations.
✅ Verification successful
Mock implementation verified as consistent with test usage patterns
The mock implementation of GetPolicies aligns perfectly with its usage across test files. The verification shows:
- Consistent return value handling (policies and found flag) in all test cases
- Proper usage in various test scenarios including genesis, GRPC queries, and keeper tests
- No inconsistencies or mismatches in the method signature or return types
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage patterns of FungibleAuthorityKeeper mock
# Expected: Find test files using this mock and verify GetPolicies usage
# Find test files using FungibleAuthorityKeeper
echo "Searching for test files using FungibleAuthorityKeeper mock..."
rg -l "FungibleAuthorityKeeper" --type go --glob "*_test.go"
# Check GetPolicies usage patterns
echo -e "\nChecking GetPolicies usage patterns..."
rg "GetPolicies" --type go --glob "*_test.go" -A 3
Length of output: 2242
x/crosschain/simulation/decoders_test.go (1)
1-13: LGTM! Well-organized imports and package declaration.
The package name and imports follow Go best practices and conventions.
testutil/keeper/mocks/observer/authority.go (2)
57-83: LGTM! The mock implementation follows best practices.
The implementation of GetPolicies is well-structured and handles all edge cases properly:
- Proper panic handling for missing return values
- Comprehensive type assertions for both direct values and function returns
- Correct handling of multiple return values
57-83: Verify interface compliance.
Let's ensure the mock implementation matches the interface definition.
✅ Verification successful
Mock implementation correctly matches the interface definition
The verification confirms that the mock implementation in testutil/keeper/mocks/observer/authority.go perfectly aligns with the interface definition found in both x/observer/types/expected_keepers.go and x/fungible/types/expected_keepers.go. The method signature:
GetPolicies(ctx sdk.Context) (val authoritytypes.Policies, found bool)matches exactly with the mock implementation's signature and return values.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the mock implementation matches the interface definition
# Expected: The interface should define GetPolicies with the same signature
# Search for the interface definition
ast-grep --pattern 'type AuthorityKeeper interface {
$$$
GetPolicies(ctx sdk.Context) (val authoritytypes.Policies, found bool)
$$$
}'
Length of output: 1267
x/observer/types/keys.go (1)
Line range hint 52-82: Consider implementing a structured deprecation process for key renaming.
The code contains multiple TODOs for renaming keys and several unused keys. While maintaining backward compatibility is important, having multiple unused keys in production code increases maintenance overhead.
Consider:
- Implementing a structured deprecation process:
- Add deprecation notices with target removal versions
- Document migration paths for each key change
- Creating constants for both old and new keys during transition:
const (
// Deprecated: Use BallotKey instead. Will be removed in version X.Y.Z
VoterKey = "Voter-value-"
// New key name for improved clarity
BallotKey = VoterKey
)Let's verify the usage of these "unused" keys:
x/crosschain/simulation/decoders.go (3)
1-12: LGTM: Well-organized package structure and imports
The package structure follows Go best practices with clean import organization.
15-16: LGTM: Clean function signature and closure pattern
The function signature and closure implementation follow Go best practices.
28-33: Ensure consistent handling of FinalizedInboundsKey
The FinalizedInboundsKey case handles raw bytes differently from other cases. Consider documenting why this special handling is necessary or standardize the approach.
✅ Verification successful
Let me check how other keys are handled in the decoder to make a proper comparison.
Let me try a different approach to check the decoder implementation.
Let me check the type definition of FinalizedInbounds to understand if this raw byte handling is justified.
Raw byte handling for FinalizedInboundsKey is appropriate
The special handling of FinalizedInboundsKey as raw bytes is justified based on the implementation in the keeper. The value stored is simply a marker ([]byte{1}) to indicate the existence of a finalized inbound transaction, with no need for complex unmarshaling. Other keys like CCTXKey and GasPriceKey require codec unmarshaling as they store structured data.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any special handling of FinalizedInbounds in the codebase
rg -A 5 "FinalizedInboundsKey"
Length of output: 2925
Script:
#!/bin/bash
# Check the handling of other keys in the decoder
ast-grep --pattern 'case bytes.Equal(kvA.Key, types.KeyPrefix($_)) {
$$$
}'
Length of output: 79
Script:
#!/bin/bash
# Get the full decoder implementation to compare handling of different keys
rg -A 20 "func NewDecodeStore" x/crosschain/simulation/decoders.go
Length of output: 1139
Script:
#!/bin/bash
# Search for FinalizedInbounds type definition and usage
rg -g '*.go' -A 5 "type.*FinalizedInbound" x/crosschain/
Length of output: 3626
testutil/keeper/emissions.go (1)
57-58: Verify observer keeper initialization across test files.
Let's ensure the changes are consistently applied across the codebase.
✅ Verification successful
Observer keeper initialization is consistent across the codebase
The verification shows that the initObserverKeeper function signature and its usage are perfectly aligned across all implementations:
- The function signature in
observer.gocorrectly includesbankKeeperandauthKeeperparameters - Both
emissions.goandibccrosschain.goproperly pass these dependencies during initialization - All callers provide the required parameters in the correct order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify observer keeper initialization consistency
# Test 1: Check if initObserverKeeper function signature matches the new parameters
echo "Checking initObserverKeeper function signature..."
ast-grep --pattern 'func initObserverKeeper($$$) {$$$}'
# Test 2: Find all files that might need updates for these new dependencies
echo "Finding potential affected test files..."
rg -l "initObserverKeeper" --type go
Length of output: 384
Script:
#!/bin/bash
# Let's examine the actual implementation and usage patterns
# Check the implementation in observer.go
echo "=== Observer Keeper Implementation ==="
rg "func initObserverKeeper" -A 10 testutil/keeper/observer.go
# Check the implementation in emissions.go
echo -e "\n=== Emissions Keeper Implementation ==="
rg "func initObserverKeeper" -A 10 testutil/keeper/emissions.go
# Check the implementation in ibccrosschain.go
echo -e "\n=== IBC Crosschain Implementation ==="
rg "func initObserverKeeper" -A 10 testutil/keeper/ibccrosschain.go
# Look for actual keeper initialization calls
echo -e "\n=== Keeper Initialization Calls ==="
rg "initObserverKeeper\(" -A 2
Length of output: 1429
Script:
#!/bin/bash
# Let's check the actual calls to see if they match the signature
echo "=== Emissions Keeper Call ==="
rg "observerKeeperTmp := initObserverKeeper\(" -A 10 testutil/keeper/emissions.go
echo -e "\n=== IBC Crosschain Keeper Call ==="
rg "observerKeeper := initObserverKeeper\(" -A 10 testutil/keeper/ibccrosschain.go
# Also check for any other potential callers
echo -e "\n=== All Observer Keeper Initialization Patterns ==="
rg "initObserverKeeper\(.*\)" --multiline
Length of output: 986
testutil/keeper/ibccrosschain.go (1)
80-81: LGTM! Consider enhancing mock coverage.
The addition of BankKeeper and AuthKeeper parameters aligns well with the PR's objective of improving simulation tests. The integration is clean and follows proper keeper initialization patterns.
Let's verify the consistency of these changes across other test utilities:
✅ Verification successful
Changes are consistent with mock implementations and test utilities
The addition of BankKeeper and AuthKeeper parameters is properly integrated across the codebase:
- Both keepers are consistently used in initObserverKeeper and initFungibleKeeper
- Corresponding mock implementations exist in testutil/keeper/mocks/
- All dependent test utilities properly handle these keepers with appropriate type definitions and mock instantiations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if BankKeeper and AuthKeeper are consistently used across test utilities
# Expected: All keeper initializations should follow the same pattern
# Search for other keeper initializations that might need similar updates
rg -g '*.go' -B 2 -A 2 'initObserverKeeper|initFungibleKeeper' testutil/
Length of output: 1356
Script:
#!/bin/bash
# Let's examine the implementation of initObserverKeeper and initFungibleKeeper
# to verify the parameter consistency
# Check initObserverKeeper implementation
echo "=== initObserverKeeper implementation ==="
rg -g '*.go' -A 15 '^func initObserverKeeper' testutil/keeper/
# Check initFungibleKeeper implementation
echo -e "\n=== initFungibleKeeper implementation ==="
rg -g '*.go' -A 15 '^func initFungibleKeeper' testutil/keeper/
# Check for any other occurrences of BankKeeper and AuthKeeper in test utilities
echo -e "\n=== BankKeeper and AuthKeeper usage in test utilities ==="
rg -g '*.go' 'BankKeeper|AuthKeeper' testutil/
Length of output: 10382
x/crosschain/keeper/msg_server_vote_inbound_tx.go (1)
Line range hint 110-134: Improve robustness of state modification operations.
The function handles multiple critical state updates. Consider these improvements:
- The
#nosec G115comment should explain why the linter warning is safe to ignore. - Consider adding error handling for state modifications.
Let's verify the state modification safety:
Consider wrapping the state modifications in a transaction-like pattern:
func (k Keeper) SaveObservedInboundInformation(ctx sdk.Context, cctx *types.CrossChainTx, eventIndex uint64) error {
// Create a cached context that can be committed or discarded
tmpCtx, commit := ctx.CacheContext()
EmitEventInboundFinalized(tmpCtx, cctx)
if err := k.AddFinalizedInbound(tmpCtx,
cctx.GetInboundParams().ObservedHash,
cctx.GetInboundParams().SenderChainId,
eventIndex); err != nil {
return err
}
// Block height is guaranteed to be positive in Cosmos SDK
cctx.InboundParams.FinalizedZetaHeight = uint64(tmpCtx.BlockHeight())
cctx.InboundParams.TxFinalizationStatus = types.TxFinalizationStatus_Executed
if err := k.RemoveInboundTrackerIfExists(tmpCtx,
cctx.InboundParams.SenderChainId,
cctx.InboundParams.ObservedHash); err != nil {
return err
}
if err := k.SetCrossChainTx(tmpCtx, *cctx); err != nil {
return err
}
// Commit changes only if all operations succeed
commit()
return nil
}x/fungible/module.go (2)
37-39: LGTM: Constructor update is consistent with type change.
The constructor modification properly reflects the codec type change and follows idiomatic Go patterns.
34-34: Verify codec type change implications across the module.
The change from codec.BinaryCodec to codec.Codec broadens the codec capabilities. While this aligns with the broader refactoring effort, we should ensure consistent behavior across all serialization operations.
✅ Verification successful
Codec type change is safe and consistent across the module
The change from codec.BinaryCodec to codec.Codec is safe as:
- No remaining
BinaryCodecusages were found in the module - All codec operations (
Marshal,Unmarshal,MustMarshal,MustUnmarshal) are compatible with the broadercodec.Codecinterface - The usage patterns in keeper and simulation layers follow consistent patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify codec usage consistency across the module
# Look for any remaining BinaryCodec usage that might need updating
# Search for BinaryCodec usage
echo "Checking for any remaining BinaryCodec usage in the module:"
rg -l "codec\.BinaryCodec" ./x/fungible/
# Search for codec method calls that might be affected
echo "Checking codec method usage patterns:"
rg "cdc\.(Marshal|Unmarshal|MustMarshal|MustUnmarshal)" ./x/fungible/
Length of output: 1766
x/fungible/keeper/v2_deposits_test.go (1)
4-6: LGTM: Clean import addition
The addition of the math/big package is appropriate for handling large integer values in deposit processing.
testutil/keeper/observer.go (2)
13-15: LGTM: Import additions are well-structured.
The new imports for auth and bank keepers are properly organized and necessary for the enhanced keeper initialization.
52-53: LGTM: Keeper initialization properly includes new dependencies.
The addition of bank and auth keepers is well-structured. However, let's verify that all callers of this function have been updated accordingly.
Also applies to: 69-70
✅ Verification successful
All callers properly updated with new keeper dependencies
The verification confirms that all callers of initObserverKeeper have been correctly updated to include the new bankKeeper and authKeeper parameters in the correct order:
testutil/keeper/ibccrosschain.go: UsessdkKeepers.BankKeeperandsdkKeepers.AuthKeepertestutil/keeper/emissions.go: UsessdkKeepers.BankKeeperandsdkKeepers.AuthKeeper
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to initObserverKeeper to ensure they include the new parameters
rg "initObserverKeeper\(" --type go
Length of output: 233
Script:
#!/bin/bash
# Check the implementation of initObserverKeeper calls in the found files
rg "initObserverKeeper\(" -A 10 --type go
Length of output: 1906
app/modules.go (1)
Line range hint 52-53: Consider creating a tracking issue for the TODO comment.
The comment indicates a planned refactor to use app.mm for creating the basic manager. To ensure this technical debt is properly tracked, consider creating a dedicated issue if #3021 doesn't already cover this specific refactoring task.
✅ Verification successful
Issue #3021 already tracks the TODO comment for basic manager refactoring
The TODO comment in the code corresponds to issue #3021, which specifically addresses the refactoring of basic manager creation from the module manager. The issue provides detailed context and references the exact code location, making it unnecessary to create a new tracking issue.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if issue #3021 covers this specific refactoring task
gh issue view 3021
Length of output: 1387
testutil/sample/observer.go (1)
122-123: Track TODO comment with an issue reference.
The TODO comment is properly linked to an issue, which is good practice for tracking technical debt.
testutil/keeper/fungible.go (1)
119-120: ```shell
#!/bin/bash
Let's examine the observer keeper initialization in both files to verify consistency
Check crosschain.go keeper initialization
echo "=== crosschain.go keeper initialization ==="
rg -A 20 "observerKeeperTmp := observerkeeper.NewKeeper" testutil/keeper/crosschain.go
echo -e "\n=== fungible.go keeper initialization ==="
rg -A 20 "observerKeeperTmp := observerkeeper.NewKeeper" testutil/keeper/fungible.go
Let's also check the observer keeper interface/implementation
echo -e "\n=== Observer Keeper interface/implementation ==="
ast-grep --pattern 'type ObserverKeeper interface {
$$$
}'
</details>
<details>
<summary>x/crosschain/types/expected_keepers.go (2)</summary>
`27-27`: **LGTM: Standard Cosmos SDK account query method.**
The addition of `GetAccount` method follows the standard Cosmos SDK pattern for account management and is essential for account-related operations.
---
`34-34`: **LGTM: Standard Cosmos SDK spendable coins query method.**
The addition of `SpendableCoins` method follows the standard Cosmos SDK pattern for balance queries and is essential for transaction validation.
</details>
<details>
<summary>testutil/keeper/crosschain.go (1)</summary>
`132-133`: **LGTM: Keeper initialization enhanced with necessary dependencies.**
The addition of BankKeeper and AuthKeeper to observerKeeperTmp initialization is well-structured and maintains consistency with the module's architecture.
</details>
<details>
<summary>Makefile (1)</summary>
`407-407`: **Parameter reduction appears reasonable but warrants monitoring.**
The reduction in simulation parameters (blocks: 100→50, transactions: 200→100) aligns with the goal of preventing store comparison failures. However, to maintain comprehensive test coverage:
1. Consider implementing additional focused test scenarios that specifically target edge cases that might be missed with reduced parameters.
2. Monitor test effectiveness over time to ensure the reduced parameters continue to provide adequate coverage.
Let's verify the simulation test configuration across the codebase:
<details>
<summary>✅ Verification successful</summary>
Let me gather more information about the simulation configuration to better understand the test coverage.
---
**Parameter reduction is appropriate and well-balanced**
The simulation test configuration appears to be part of a comprehensive test suite that includes multiple test scenarios (`TestFullAppSimulation`, `TestAppImportExport`, `TestAppSimulationAfterImport`). The parameter reduction to 50 blocks and 100 transactions maintains a reasonable balance between test coverage and computational efficiency, especially considering:
1. The test suite includes multiple simulation scenarios that collectively provide coverage from different angles
2. The parameters are still substantial enough to exercise state transitions and cross-module interactions
3. The configuration is consistently applied across all simulation tests
</details>
<details>
<summary>🏁 Scripts executed</summary>
The following scripts were executed for the analysis:
Script:
```shell
#!/bin/bash
# Description: Check for other simulation test configurations that might need adjustment
# and verify the presence of additional test scenarios.
# Search for simulation test configurations
echo "Searching for simulation test configurations..."
rg -A 5 "TestAppImportExport|TestFullAppSimulation|TestAppSimulationAfterImport" --type go
# Look for focused test scenarios
echo "Checking for focused test scenarios..."
rg -A 5 "func.*Test.*Simulation" --type go
Length of output: 2392
Script:
#!/bin/bash
# Check for simulation configuration flags and parameters
echo "Checking simulation configuration..."
rg -A 10 "NewConfigFromFlags" --type go
# Look for any custom simulation operations or test scenarios
echo "Checking custom simulation operations..."
rg -A 5 "WeightedOperations|TestingApp" --type go
# Check for any simulation constants or default values
echo "Checking simulation constants..."
rg "const.*Sim|var.*Sim" --type go
Length of output: 9072
x/observer/simulation/operations.go (1)
30-30: Verify the appropriateness of the default operation weight
The DefaultWeightMsgTypeMsgEnableCCTX is set to 2650, which significantly exceeds the typical maximum weight of 100 used in Cosmos SDK modules, as mentioned in the comments. Please verify that this weight value is suitable and does not negatively impact simulation performance or skew the operation distribution excessively.
simulation/state.go (1)
56-57: Confirm early return does not skip essential initialization
By returning early when exportedState is not nil, the function bypasses subsequent initialization logic. Ensure that this control flow does not omit any critical setup steps required for the application's proper functioning.
app/app.go (1)
484-485: Integration of BankKeeper and AccountKeeper into ObserverKeeper
The inclusion of app.BankKeeper and app.AccountKeeper as dependencies for the ObserverKeeper enhances its ability to interact with banking and account functionalities directly. This integration is appropriate and aligns with the module's responsibilities within the application architecture.
Co-authored-by: skosito <[email protected]>
…to crosschain-simulation-params
|
!!!WARNING!!! Be very careful about using Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203 Pay extra attention to the way |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 14567965 | Triggered | Generic Password | 1167862 | cmd/zetaclientd/start.go | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3095 +/- ##
===========================================
+ Coverage 61.72% 61.76% +0.04%
===========================================
Files 432 428 -4
Lines 30809 30791 -18
===========================================
+ Hits 19016 19018 +2
+ Misses 10934 10914 -20
Partials 859 859
|
Description
crosschain,observerandfungibleCloses
#2972
#3066
#3099
#3099
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Refactor