feat(appchains): IBC packets #187
Conversation
x/avs handles the assetID validation
- Anyone can create a subchain via a simple transaction. The genesis file is available at the end of the current epoch. - If no operators opt in by that time, it is dropped. - If the state is available, it is queried via CLI and added to the genesis file of the subchain. - The genesis file, with data from other modules, starts the chain once enough vote power is live
The AVS module now validates the assetIDs passed to it
negative duration is also not supported
The wrapped consensus key object has utility in the subscriber module as well, and, may have some utility in the coordinator module. Hence, it is better located in a `types` subpackage.
- handles slashing, rewards, validator set, maturity, unbonding and opt out
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in this pull request introduce a comprehensive update to the appchain coordination module within the Exocore application. Key modifications include the addition of a Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| for _, update := range m { | ||
| out = append(out, update) | ||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| frac, err := sdk.NewDecFromStr(k.GetSubscriberParams(ctx).SubscriberRedistributionFraction) | ||
| if err != nil { | ||
| // should not happen since we validated this in the params | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if err != nil { | ||
| // It is the common behavior in cosmos-sdk to panic if SendCoinsFromModuleToModule | ||
| // returns error. | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if err != nil { | ||
| // It is the common behavior in cosmos-sdk to panic if SendCoinsFromModuleToModule | ||
| // returns error. | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if err != nil { | ||
| // This should never happen as the pubkey is assumed | ||
| // to be stored correctly in ApplyCCValidatorChanges. | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| if err != nil { | ||
| // This should never happen as the pubkey is assumed | ||
| // to be stored correctly in ApplyCCValidatorChanges. | ||
| panic(err) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| "height", height, "vscID", vscID, | ||
| ) | ||
|
|
||
| am.keeper.TrackHistoricalInfo(ctx) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
| // EndBlock contains the logic that is automatically triggered at the end of each block | ||
| func (am AppModule) EndBlock(ctx sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate { | ||
| // send rewards to coordinator | ||
| am.keeper.EndBlockSendRewards(ctx) |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
🛑 Comments failed to post (83)
x/appchain/coordinator/keeper/distribution.go (1)
11-14: 🛠️ Refactor suggestion
Consider adding error handling and improving readability.
The implementation is concise, but it makes some assumptions that may not always hold true. Consider the following improvements:
- Add error handling to account for cases where the module account might not exist.
- Split the chained method calls for better readability.
Here's a suggested implementation:
func (k Keeper) GetSubscriberRewardsPoolAddressStr(ctx sdk.Context) (string, error) { moduleAcc := k.accountKeeper.GetModuleAccount(ctx, types.SubscriberRewardsPool) if moduleAcc == nil { return "", fmt.Errorf("subscriber rewards pool module account not found") } return moduleAcc.GetAddress().String(), nil }This version handles potential errors and improves readability by breaking down the chained calls.
x/appchain/common/types/keys.go (1)
7-7: 🛠️ Refactor suggestion
Consider a more flexible approach to versioning.
The current version is hardcoded as
ModuleName + "-1". For better maintainability and easier updates, consider using a separate version constant or environment variable.Example:
const ( ModuleVersion = "1" Version = ModuleName + "-" + ModuleVersion )This approach allows for easier version updates in the future.
x/appchain/subscriber/keeper/ibc_client.go (1)
11-21: 🛠️ Refactor suggestion
Consider adding input validation for
portIDandchannelID.While the current implementation looks good, it might be beneficial to add checks for empty
portIDandchannelIDat the beginning of the function. This can help catch potential issues earlier and provide more specific error messages.Here's a suggested improvement:
func (k Keeper) ChanCloseInit(ctx sdk.Context, portID, channelID string) error { + if portID == "" { + return errorsmod.Wrap(channeltypes.ErrInvalidPort, "port ID cannot be empty") + } + if channelID == "" { + return errorsmod.Wrap(channeltypes.ErrInvalidChannel, "channel ID cannot be empty") + } capName := host.ChannelCapabilityPath(portID, channelID) chanCap, ok := k.scopedKeeper.GetCapability(ctx, capName) if !ok {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) ChanCloseInit(ctx sdk.Context, portID, channelID string) error { if portID == "" { return errorsmod.Wrap(channeltypes.ErrInvalidPort, "port ID cannot be empty") } if channelID == "" { return errorsmod.Wrap(channeltypes.ErrInvalidChannel, "channel ID cannot be empty") } capName := host.ChannelCapabilityPath(portID, channelID) chanCap, ok := k.scopedKeeper.GetCapability(ctx, capName) if !ok { return errorsmod.Wrapf( channeltypes.ErrChannelCapabilityNotFound, "could not retrieve channel capability at: %s", capName, ) } return k.channelKeeper.ChanCloseInit(ctx, portID, channelID, chanCap) }x/feedistribution/keeper/allocation.go (1)
7-7: 💡 Codebase verification
Issue Detected: Incomplete Removal of
avstypesThe
avstypespackage is still referenced in multiple files across the codebase. Removing its import inx/feedistribution/keeper/allocation.gowithout addressing these references may cause build failures and functionality issues.Files with remaining
avstypesreferences:
x/operator/types/genesis.gox/feedistribution/keeper/hooks_test.gox/feedistribution/keeper/allocation.gox/operator/keeper/grpc_query.gox/operator/keeper/slash_test.gox/dogfood/keeper/genesis.go- (and others as listed in the shell script output)
Please review the removal of
avstypesand ensure all dependencies are appropriately handled.🔗 Analysis chain
LGTM: Import changes look good.
The addition of the
utilsimport and removal ofavstypessuggest a refactoring of utility functions. This change appears to improve code organization.To ensure this change doesn't introduce any issues, please run the following script to check for any remaining references to
avstypes:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to avstypes in the codebase # Test: Search for avstypes usage rg --type go 'avstypes\.'Length of output: 4499
x/operator/types/keys.go (1)
209-216: 💡 Codebase verification
Optimization Requires Additional Changes
The proposed optimization to use the
chainIDwithout its length may introduce issues because existing functions rely on thechainIDlength for key parsing.Affected Areas:
ParseKeyForOperatorKeyRemovalinx/operator/types/keys.goexpects the key to include the chainID length.- Multiple usages in
x/operator/keeper/consensus_keys.godepend on the current key structure.Please ensure all dependent functions and key handling logic are updated accordingly before implementing this optimization.
🔗 Analysis chain
LGTM: Consistent implementation with added optimization note.
The changes align with previous updates, using
utils.AppendManyandutils.ChainIDWithLenKeyfor consistency and improved readability.Regarding the TODO comment about potentially using the chainID without its length:
This optimization could indeed be beneficial, as it would reduce key size and simplify the key structure. However, we need to verify if this change would affect any existing queries or iterations over these keys.
To investigate this, let's run the following script:
This script will help us understand if removing the chainID length from the key would impact any existing code. Based on the results, we can decide whether to proceed with the optimization or keep the current implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any code that might rely on the current key structure with chainID length # Search for usages of KeyForOperatorKeyRemovalForChainID echo "Usages of KeyForOperatorKeyRemovalForChainID:" rg "KeyForOperatorKeyRemovalForChainID" --type go # Search for any key parsing or iteration logic related to operator key removal echo "\nRelated key parsing or iteration logic:" rg "BytePrefixForOperatorKeyRemovalForChainID" --type go -A 10Length of output: 4330
x/appchain/subscriber/keeper/distribution.go (1)
22-29: 🛠️ Refactor suggestion
Enhance error message with channel identifiers
Including
srcPortandsrcChanin the error message will provide more context when a channel is not found, aiding in debugging.Apply this diff to improve the error message:
return []string{}, errorsmod.Wrapf(commontypes.ErrChannelNotFound, - "cannot get connection hops from non-existent channel") + "cannot get connection hops from non-existent channel %s:%s", srcPort, srcChan)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) GetConnectionHops(ctx sdk.Context, srcPort, srcChan string) ([]string, error) { ch, found := k.channelKeeper.GetChannel(ctx, srcPort, srcChan) if !found { return []string{}, errorsmod.Wrapf(commontypes.ErrChannelNotFound, "cannot get connection hops from non-existent channel %s:%s", srcPort, srcChan) } return ch.ConnectionHops, nil }x/appchain/common/types/validator.go (2)
21-23:
⚠️ Potential issueInconsistent field name casing:
Pubkeyshould bePubKeyFor consistency with Go naming conventions and the rest of the code, consider renaming the field
PubkeytoPubKey.Apply this diff to correct the field name:
return SubscriberValidator{ ConsAddress: address, Power: power, - Pubkey: pkAny, + PubKey: pkAny, }Ensure that all references to this field are updated accordingly throughout the codebase.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ConsAddress: address, Power: power, PubKey: pkAny,
36-37:
⚠️ Potential issueAdd nil check for
Pubkeybefore accessingGetCachedValue()To prevent a potential nil pointer dereference, consider adding a check to ensure
ocv.Pubkeyis not nil before callingGetCachedValue().Apply this diff to include the nil check:
func (ocv SubscriberValidator) ConsPubKey() (cryptotypes.PubKey, error) { + if ocv.Pubkey == nil { + return nil, errorsmod.Wrap(sdkerrors.ErrInvalidType, "Pubkey is nil") + } pk, ok := ocv.Pubkey.GetCachedValue().(cryptotypes.PubKey) if !ok { return nil, errorsmod.Wrapf( sdkerrors.ErrInvalidType, "expecting cryptotypes.PubKey, got %T", pk, ) } return pk, nil }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (ocv SubscriberValidator) ConsPubKey() (cryptotypes.PubKey, error) { if ocv.Pubkey == nil { return nil, errorsmod.Wrap(sdkerrors.ErrInvalidType, "Pubkey is nil") } pk, ok := ocv.Pubkey.GetCachedValue().(cryptotypes.PubKey) if !ok { return nil, errorsmod.Wrapf( sdkerrors.ErrInvalidType, "expecting cryptotypes.PubKey, got %T", pk, ) } return pk, nil }x/appchain/coordinator/keeper/height.go (2)
16-21:
⚠️ Potential issueHandle nil values to prevent panic when retrieving height
In
GetHeightForChainVscID, if the key does not exist in the store,store.Get(key)returnsnil. Passingniltosdk.BigEndianToUint64can cause a panic. It's important to check if the key exists before attempting to decode the value.Consider modifying the function as follows:
func (k Keeper) GetHeightForChainVscID(ctx sdk.Context, chainID string, vscID uint64) uint64 { store := ctx.KVStore(k.storeKey) key := types.HeightToChainVscIDKey(chainID, vscID) if !store.Has(key) { return 0 } return sdk.BigEndianToUint64(store.Get(key)) }
24-28:
⚠️ Potential issueEnsure safe retrieval of VscID to avoid potential panics
Similarly, in
GetVscIDForChain, if the key does not exist,store.Get(key)will returnnil, which may lead to a panic when decoded. Adding a check for the key's existence prevents this issue.Here's the suggested modification:
func (k Keeper) GetVscIDForChain(ctx sdk.Context, chainID string) uint64 { store := ctx.KVStore(k.storeKey) key := types.VscIDForChainKey(chainID) if !store.Has(key) { return 0 } return sdk.BigEndianToUint64(store.Get(key)) }x/appchain/common/types/types.go (2)
13-25:
⚠️ Potential issueEnsure safe conversion of
resto string to avoid invalid UTF-8 encodingConverting arbitrary byte slices to strings using
string(res)may result in invalid UTF-8 sequences, leading to logging errors or unexpected behavior. To prevent this, consider encodingresusing a safe method before logging.Apply this diff to safely log the result:
func NewResultAcknowledgementWithLog(ctx sdk.Context, res []byte) channeltypes.Acknowledgement { if len(res) > maxLogSize { ctx.Logger().Info( "IBC ResultAcknowledgement constructed", "res_size", len(res), - "res_preview", string(res[:maxLogSize]), + "res_preview", hex.EncodeToString(res[:maxLogSize]), ) } else { ctx.Logger().Info( "IBC ResultAcknowledgement constructed", "res_size", len(res), - "res", string(res), + "res", hex.EncodeToString(res), ) } return channeltypes.NewResultAcknowledgement(res) }Additionally, add the necessary import statement at the top of the file:
import ( "encoding/hex" // other imports... )This change uses
hex.EncodeToStringto convert the byte slices to a hexadecimal string representation, ensuring that the logged data is valid UTF-8 and preventing potential encoding issues.
48-49:
⚠️ Potential issueCorrect the function comment to match
NewVscMaturedPacketDataThe function comment incorrectly refers to
NewVscPacketDatainstead ofNewVscMaturedPacketData. This inconsistency may cause confusion.Apply this diff to correct the comment:
-// NewVscPacketData creates a new VscMaturedPacketData instance. +// NewVscMaturedPacketData creates a new VscMaturedPacketData instance.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// NewVscMaturedPacketData creates a new VscMaturedPacketData instance. func NewVscMaturedPacketData(proto/exocore/appchain/subscriber/v1/genesis.proto (1)
36-37:
⚠️ Potential issueAdjust field definition formatting for
maturity_time.The field options for
maturity_timeare placed on a new line, which may lead to parsing errors with some protobuf compilers. It's standard practice to keep field options on the same line as the field declaration.Apply this diff to correct the formatting:
- google.protobuf.Timestamp maturity_time = 2 - [ (gogoproto.stdtime) = true, (gogoproto.nullable) = false ]; + google.protobuf.Timestamp maturity_time = 2 [ (gogoproto.stdtime) = true, (gogoproto.nullable) = false ];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.google.protobuf.Timestamp maturity_time = 2 [ (gogoproto.stdtime) = true, (gogoproto.nullable) = false ];x/appchain/subscriber/keeper/genesis.go (1)
28-29: 🛠️ Refactor suggestion
Evaluate the use of
panicfor error handling during initializationWhile panicking on critical initialization failures ensures that the application doesn't proceed in an invalid state, consider whether returning errors might offer better flexibility. This allows for graceful degradation or retries in certain deployment scenarios.
For example, you could modify the error handling as:
if err := k.portKeeper.BindPort(ctx, commontypes.SubscriberPortID); err != nil { return nil, fmt.Errorf("could not claim port capability: %v", err) } ... if err != nil { return nil, fmt.Errorf("could not create client for coordinator chain: %v", err) }Also applies to: 36-37
x/appchain/coordinator/keeper/impl_epochs_hooks.go (4)
48-48:
⚠️ Potential issueHandle errors returned by
SendQueuedValidatorUpdatesAny errors from
SendQueuedValidatorUpdatesshould be handled to ensure that queued validator updates are sent successfully.Apply this diff to handle the error:
- wrapper.keeper.SendQueuedValidatorUpdates(ctx, epoch) + if err := wrapper.keeper.SendQueuedValidatorUpdates(ctx, epoch); err != nil { + // Handle the error appropriately, e.g., log it + ctx.Logger().Error("Failed to send queued validator updates", "epoch", epoch, "error", err) + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if err := wrapper.keeper.SendQueuedValidatorUpdates(ctx, epoch); err != nil { // Handle the error appropriately, e.g., log it ctx.Logger().Error("Failed to send queued validator updates", "epoch", epoch, "error", err) }
40-40:
⚠️ Potential issueHandle errors returned by
RemoveTimedoutSubscribersThe function
RemoveTimedoutSubscribersmay produce an error that is not being handled. Proper error handling ensures robustness and easier debugging.Apply this diff to handle the error:
- wrapper.keeper.RemoveTimedoutSubscribers(ctx, identifier, epoch) + if err := wrapper.keeper.RemoveTimedoutSubscribers(ctx, identifier, epoch); err != nil { + // Handle the error appropriately, e.g., log it + ctx.Logger().Error("Failed to remove timed-out subscribers", "error", err) + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if err := wrapper.keeper.RemoveTimedoutSubscribers(ctx, identifier, epoch); err != nil { // Handle the error appropriately, e.g., log it ctx.Logger().Error("Failed to remove timed-out subscribers", "error", err) }
28-28:
⚠️ Potential issueHandle errors returned by
ActivateScheduledChainsThe function
ActivateScheduledChainsmay return an error that is currently not being handled. Ignoring potential errors can lead to unexpected behavior or silent failures. It's important to capture and handle errors appropriately.Apply this diff to handle the error:
- wrapper.keeper.ActivateScheduledChains(ctx, identifier, epoch) + if err := wrapper.keeper.ActivateScheduledChains(ctx, identifier, epoch); err != nil { + // Handle the error appropriately, e.g., log it + ctx.Logger().Error("Failed to activate scheduled chains", "error", err) + // Depending on the severity, consider taking additional action + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if err := wrapper.keeper.ActivateScheduledChains(ctx, identifier, epoch); err != nil { // Handle the error appropriately, e.g., log it ctx.Logger().Error("Failed to activate scheduled chains", "error", err) // Depending on the severity, consider taking additional action }
45-45:
⚠️ Potential issueHandle errors returned by
QueueValidatorUpdatesForEpochIDIgnoring errors from
QueueValidatorUpdatesForEpochIDmay lead to unprocessed validator updates. Ensure errors are captured and handled.Apply this diff to handle the error:
- wrapper.keeper.QueueValidatorUpdatesForEpochID(ctx, identifier, epoch) + if err := wrapper.keeper.QueueValidatorUpdatesForEpochID(ctx, identifier, epoch); err != nil { + // Handle the error appropriately, e.g., log it + ctx.Logger().Error("Failed to queue validator updates for epoch", "epoch", epoch, "error", err) + }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if err := wrapper.keeper.QueueValidatorUpdatesForEpochID(ctx, identifier, epoch); err != nil { // Handle the error appropriately, e.g., log it ctx.Logger().Error("Failed to queue validator updates for epoch", "epoch", epoch, "error", err) }x/appchain/coordinator/types/expected_keepers.go (1)
58-61: 🛠️ Refactor suggestion
Consider using more specific types instead of
[]byteinDelegationKeepermethodsIn the
DelegationKeeperinterface, the methodsIncrementUndelegationHoldCountandDecrementUndelegationHoldCountaccept a[]byteparameter. Using a more specific type (e.g.,sdk.AccAddressor a defined identifier type) could improve type safety and code readability.x/appchain/subscriber/keeper/connection.go (1)
12-86: 🛠️ Refactor suggestion
Add unit tests for new keeper methods
The newly added methods in the keeper are crucial for managing interactions with the coordinator chain. To ensure their correct functionality and prevent future regressions, please consider adding unit tests for these methods.
Would you like assistance in creating unit tests for these methods?
x/appchain/coordinator/keeper/impl_operator_hooks.go (2)
56-78: 🛠️ Refactor suggestion
Refactor duplicated logic to reduce code repetition
Similarly, the method
AfterOperatorKeyRemovalInitiatedcontains logic that duplicates code found inAfterOperatorKeyReplaced. Refactoring to use the shared helper function improves consistency and maintainability.Apply this diff to refactor the code:
func (h OperatorHooksWrapper) AfterOperatorKeyRemovalInitiated( ctx sdk.Context, _ sdk.AccAddress, chainID string, key exocoretypes.WrappedConsKey, ) { - if chainID != utils.ChainIDWithoutRevision(ctx.ChainID()) { - consAddr := key.ToConsAddr() - _, found := h.keeper.GetSubscriberValidatorForChain(ctx, chainID, consAddr) - if found { - // schedule this consensus address for pruning at the maturity of the packet containing this vscID that will - // go out at the end of this epoch. - nextVscID := h.keeper.GetVscIDForChain(ctx, chainID) + 1 - h.keeper.AppendConsAddrToPrune(ctx, chainID, nextVscID, consAddr) - // reverse lookup - h.keeper.SetMaturityVscIDForChainIDConsAddr(ctx, chainID, consAddr, nextVscID) - } else { - // delete the reverse lookup of old cons addr + chain id -> operator addr, since it was never an active - // validator. - h.keeper.operatorKeeper.DeleteOperatorAddressForChainIDAndConsAddr( - ctx, chainID, consAddr, - ) - } - } + consAddr := key.ToConsAddr() + h.handleOperatorKeyChange(ctx, chainID, consAddr) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// AfterOperatorKeyRemovalInitiated is the implementation of the operator hooks. func (h OperatorHooksWrapper) AfterOperatorKeyRemovalInitiated( ctx sdk.Context, _ sdk.AccAddress, chainID string, key exocoretypes.WrappedConsKey, ) { consAddr := key.ToConsAddr() h.handleOperatorKeyChange(ctx, chainID, consAddr) }
30-54: 🛠️ Refactor suggestion
Refactor duplicated logic to reduce code repetition
The logic within
AfterOperatorKeyReplaced(lines 36-53) is nearly identical to that inAfterOperatorKeyRemovalInitiated. Consider extracting this shared logic into a separate helper function to avoid code duplication and enhance maintainability.Apply this diff to refactor the code:
+func (h OperatorHooksWrapper) handleOperatorKeyChange( + ctx sdk.Context, chainID string, consAddr sdk.ConsAddress, +) { + if chainID != utils.ChainIDWithoutRevision(ctx.ChainID()) { + _, found := h.keeper.GetSubscriberValidatorForChain(ctx, chainID, consAddr) + if found { + // Schedule this consensus address for pruning at the maturity of the packet containing this vscID + // that will go out at the end of this epoch. + nextVscID := h.keeper.GetVscIDForChain(ctx, chainID) + 1 + h.keeper.AppendConsAddrToPrune(ctx, chainID, nextVscID, consAddr) + // Reverse lookup + h.keeper.SetMaturityVscIDForChainIDConsAddr(ctx, chainID, consAddr, nextVscID) + } else { + // Delete the reverse lookup of old cons addr + chain id -> operator addr, since it was never an active validator. + h.keeper.operatorKeeper.DeleteOperatorAddressForChainIDAndConsAddr(ctx, chainID, consAddr) + } + } +} + func (h OperatorHooksWrapper) AfterOperatorKeyReplaced( ctx sdk.Context, _ sdk.AccAddress, oldKey exocoretypes.WrappedConsKey, _ exocoretypes.WrappedConsKey, chainID string, ) { - if chainID != utils.ChainIDWithoutRevision(ctx.ChainID()) { - consAddr := oldKey.ToConsAddr() - _, found := h.keeper.GetSubscriberValidatorForChain(ctx, chainID, consAddr) - if found { - // schedule this consensus address for pruning at the maturity of the packet containing this vscID that will - // go out at the end of this epoch. - nextVscID := h.keeper.GetVscIDForChain(ctx, chainID) + 1 - h.keeper.AppendConsAddrToPrune(ctx, chainID, nextVscID, consAddr) - // reverse lookup - h.keeper.SetMaturityVscIDForChainIDConsAddr(ctx, chainID, consAddr, nextVscID) - } else { - // delete the reverse lookup of old cons addr + chain id -> operator addr, since it was never an active - // validator. - h.keeper.operatorKeeper.DeleteOperatorAddressForChainIDAndConsAddr( - ctx, chainID, consAddr, - ) - } - } + consAddr := oldKey.ToConsAddr() + h.handleOperatorKeyChange(ctx, chainID, consAddr) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// AfterOperatorKeyReplaced is the implementation of the operator hooks. func (h OperatorHooksWrapper) handleOperatorKeyChange( ctx sdk.Context, chainID string, consAddr sdk.ConsAddress, ) { if chainID != utils.ChainIDWithoutRevision(ctx.ChainID()) { _, found := h.keeper.GetSubscriberValidatorForChain(ctx, chainID, consAddr) if found { // Schedule this consensus address for pruning at the maturity of the packet containing this vscID // that will go out at the end of this epoch. nextVscID := h.keeper.GetVscIDForChain(ctx, chainID) + 1 h.keeper.AppendConsAddrToPrune(ctx, chainID, nextVscID, consAddr) // Reverse lookup h.keeper.SetMaturityVscIDForChainIDConsAddr(ctx, chainID, consAddr, nextVscID) } else { // Delete the reverse lookup of old cons addr + chain id -> operator addr, since it was never an active validator. h.keeper.operatorKeeper.DeleteOperatorAddressForChainIDAndConsAddr(ctx, chainID, consAddr) } } } func (h OperatorHooksWrapper) AfterOperatorKeyReplaced( ctx sdk.Context, _ sdk.AccAddress, oldKey exocoretypes.WrappedConsKey, _ exocoretypes.WrappedConsKey, chainID string, ) { consAddr := oldKey.ToConsAddr() h.handleOperatorKeyChange(ctx, chainID, consAddr) }x/appchain/coordinator/keeper/validators.go (1)
78-83:
⚠️ Potential issueHandle potential nil value when retrieving MaxValidators to prevent panic
In the
GetMaxValidatorsForChainfunction, if theMaxValidatorskey does not exist in the store,bzwill benil. Passingniltosdk.BigEndianToUint64(bz)can cause a panic because it expects an 8-byte slice. To ensure robustness and prevent unexpected panics, it's safer to check ifbzis notnilbefore attempting the conversion.Consider adding a check for
bz == niland returning a default value or handling the situation appropriately.Apply this diff to fix the issue:
func (k Keeper) GetMaxValidatorsForChain( ctx sdk.Context, chainID string, ) uint32 { store := ctx.KVStore(k.storeKey) key := types.MaxValidatorsKey(chainID) bz := store.Get(key) + if bz == nil { + // Return a default value, for example, 0 + return 0 + } // #nosec G115 // we stored it, we trust it return uint32(sdk.BigEndianToUint64(bz)) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) GetMaxValidatorsForChain( ctx sdk.Context, chainID string, ) uint32 { store := ctx.KVStore(k.storeKey) key := types.MaxValidatorsKey(chainID) bz := store.Get(key) if bz == nil { // Return a default value, for example, 0 return 0 } // #nosec G115 // we stored it, we trust it return uint32(sdk.BigEndianToUint64(bz)) }x/appchain/coordinator/keeper/identifiers.go (2)
67-79:
⚠️ Potential issueEnsure dynamic handling of prefix length when slicing keys
Similar to
GetAllChainsWithClients, in theGetAllChainsWithChannelsmethod, you're slicing the key usingiterator.Key()[1:]. Consider using the dynamic length of the prefix to enhance code robustness.Apply this diff:
func (k Keeper) GetAllChainsWithChannels(ctx sdk.Context) []string { store := ctx.KVStore(k.storeKey) - iterator := sdk.KVStorePrefixIterator(store, []byte{types.ChannelForChainBytePrefix}) + prefix := []byte{types.ChannelForChainBytePrefix} + iterator := sdk.KVStorePrefixIterator(store, prefix) defer iterator.Close() var chains []string for ; iterator.Valid(); iterator.Next() { - chainID := string(iterator.Key()[1:]) + chainID := string(iterator.Key()[len(prefix):]) chains = append(chains, chainID) } return chains }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) GetAllChainsWithChannels(ctx sdk.Context) []string { store := ctx.KVStore(k.storeKey) prefix := []byte{types.ChannelForChainBytePrefix} iterator := sdk.KVStorePrefixIterator(store, prefix) defer iterator.Close() var chains []string for ; iterator.Valid(); iterator.Next() { chainID := string(iterator.Key()[len(prefix):]) chains = append(chains, chainID) } return chains }
35-47:
⚠️ Potential issueEnsure dynamic handling of prefix length when slicing keys
In the
GetAllChainsWithClientsmethod, you are slicing the key usingiterator.Key()[1:]to extract thechainID. This assumes that the prefix length is exactly one byte. To make the code more robust and maintainable, consider using the length of the prefix dynamically.Apply this diff to enhance robustness:
func (k Keeper) GetAllChainsWithClients(ctx sdk.Context) []string { store := ctx.KVStore(k.storeKey) - iterator := sdk.KVStorePrefixIterator(store, []byte{types.ClientForChainBytePrefix}) + prefix := []byte{types.ClientForChainBytePrefix} + iterator := sdk.KVStorePrefixIterator(store, prefix) defer iterator.Close() var chains []string for ; iterator.Valid(); iterator.Next() { - chainID := string(iterator.Key()[1:]) + chainID := string(iterator.Key()[len(prefix):]) chains = append(chains, chainID) } return chains }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) GetAllChainsWithClients(ctx sdk.Context) []string { store := ctx.KVStore(k.storeKey) prefix := []byte{types.ClientForChainBytePrefix} iterator := sdk.KVStorePrefixIterator(store, prefix) defer iterator.Close() var chains []string for ; iterator.Valid(); iterator.Next() { chainID := string(iterator.Key()[len(prefix):]) chains = append(chains, chainID) } return chains }x/appchain/subscriber/keeper/queue.go (4)
80-81: 🛠️ Refactor suggestion
Avoid repetition of comments about key prefixes.
The same note about the correct prefix is repeated. To adhere to the DRY (Don't Repeat Yourself) principle, consider consolidating this information.
You could extract common comments into a single location or improve documentation to reduce redundancy.
38-43: 🛠️ Refactor suggestion
Streamline packet retrieval in
GetPendingPackets.Since
GetPendingPacketsis primarily a wrapper aroundGetAllPendingPacketsWithIdx, consider optimizing to remove unnecessary intermediate steps.You could modify
GetAllPendingPacketsWithIdxto accept a parameter that determines whether indexes are needed, consolidating the logic.
85-90: 🛠️ Refactor suggestion
Optimize key deletion in
DeleteAllPendingDataPackets.Collecting all keys before deletion may consume unnecessary memory. You can delete keys during iteration to improve efficiency.
Apply this diff to delete keys while iterating:
iterator := sdk.KVStorePrefixIterator(store, []byte{types.PendingDataPacketsBytePrefix}) defer iterator.Close() -keysToDel := [][]byte{} for ; iterator.Valid(); iterator.Next() { - keysToDel = append(keysToDel, iterator.Key()) + store.Delete(iterator.Key()) } -for _, key := range keysToDel { - store.Delete(key) -}This approach reduces memory usage by eliminating the need to store all keys before deleting them.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.iterator := sdk.KVStorePrefixIterator(store, []byte{types.PendingDataPacketsBytePrefix}) defer iterator.Close() for ; iterator.Valid(); iterator.Next() { store.Delete(iterator.Key()) }
58-58:
⚠️ Potential issueHandle potential unmarshalling errors.
Using
k.cdc.MustUnmarshal(bz, &packet)may cause a panic if the data is corrupted. It's safer to usek.cdc.Unmarshaland handle any errors gracefully.Apply this diff to handle unmarshalling errors:
for ; iterator.Valid(); iterator.Next() { var packet commontypes.SubscriberPacketData bz := iterator.Value() - k.cdc.MustUnmarshal(bz, &packet) + if err := k.cdc.Unmarshal(bz, &packet); err != nil { + // Handle the error, e.g., skip this packet or log the error + continue + } packetWithIdx := types.SubscriberPacketDataWithIdx{ SubscriberPacketData: packet, Idx: sdk.BigEndianToUint64(iterator.Key()[1:]), } packets = append(packets, packetWithIdx) }Committable suggestion was skipped due to low confidence.
x/appchain/coordinator/keeper/impl_delegation_hooks.go (3)
56-59:
⚠️ Potential issueAddress the TODO regarding operators in the process of opting out
There's a TODO indicating that the current implementation doesn't handle operators who are in the process of opting out:
Ensure that this scenario is properly handled to avoid inconsistencies in undelegation timing and operator status.
Would you like me to assist in implementing the necessary changes or open a GitHub issue to track this task?
74-91: 🛠️ Refactor suggestion
Refactor logic for determining 'nextVscID' to improve readability
The current logic for setting
nextVscIDcan be refactored for clarity:Consider restructuring the conditional statements to reduce nesting and improve code readability.
Here's a suggested refactor:
if wrapper.keeper.operatorKeeper.IsOperatorRemovingKeyFromChainID( ctx, operator, chainID, ) { nextVscID = wrapper.keeper.GetMaturityVscIDForChainIDConsAddr(ctx, chainID, wrappedKey.ToConsAddr()) if nextVscID == 0 { logger.Error( "undelegation maturity epoch not set", "operator", operator, "chainID", chainID, "consAddr", wrappedKey.ToConsAddr(), "recordKey", fmt.Sprintf("%x", recordKey), ) // move on to the next chainID continue } } else { - nextVscID = wrapper.keeper.GetVscIDForChain(ctx, chainID) + 1 + } + if nextVscID == 0 { + nextVscID = wrapper.keeper.GetVscIDForChain(ctx, chainID) + 1 }This reduces nesting and clearly separates the logic for setting
nextVscID.
94-96:
⚠️ Potential issueEnsure consistent error handling when incrementing undelegation hold count
In previous error scenarios, the function logs the error and continues, but here it returns the error directly:
Consider maintaining consistent error handling throughout the function. Decide whether to return errors or handle them internally, and apply that approach consistently.
proto/exocore/appchain/common/v1/wire.proto (1)
5-5:
⚠️ Potential issueFix import statement: 'cosmos/staking/v1beta1/staking.proto' file does not exist
The static analysis tool indicates that the file
'cosmos/staking/v1beta1/staking.proto'cannot be found. Please verify that the file exists at the specified location or update the import path accordingly. This is crucial since theInfractionenum from this file is used in theSlashPacketDatamessage.🧰 Tools
🪛 buf
5-5: import "cosmos/staking/v1beta1/staking.proto": file does not exist
(COMPILE)
x/appchain/coordinator/keeper/keeper.go (2)
80-84:
⚠️ Potential issueHandle Potential Nil Return in
GetPortMethodIn the
GetPortmethod:store := ctx.KVStore(k.storeKey) return string(store.Get(types.PortKey()))If
store.Get(types.PortKey())returnsnil(e.g., when the port ID has not been set), convertingnilto a string results in an empty string. This might lead to unexpected behavior downstream. Consider handling thenilcase explicitly:
- Return an error if the port ID is not set.
- Provide a default value or document that an empty string indicates an unset port.
100-117: 🛠️ Refactor suggestion
Automate Field Validation in
mustValidateFieldsThe
mustValidateFieldsmethod manually checks each field fornil:utils.PanicIfNil(k.storeKey, "storeKey") // ...additional checks for each fieldThis approach requires updating the method every time a new field is added to the
Keeperstruct, which may result in missed validations if overlooked.Consider automating the validation process to ensure all fields are checked without manual intervention. Possible solutions include:
- Using reflection: Iterate over the struct fields to check for
nilvalues. This can be done carefully to address linter concerns.- Code generation: Employ a code generation tool to produce the
mustValidateFieldsmethod, ensuring it stays up-to-date with the struct fields.- Constructor enforcement: Modify the
NewKeeperfunction to enforce field initialization, potentially reducing the need for separate validation.x/appchain/coordinator/keeper/unbonding.go (3)
61-62:
⚠️ Potential issueCheck for nil value before converting to uint64
In
GetMaturityVscIDForChainIDConsAddr, if the key does not exist,store.Get(key)will returnnil. Passingniltosdk.BigEndianToUint64will cause a panic. Consider checking ifbzisnilbefore attempting the conversion.Apply this diff to ensure safe conversion:
bz := store.Get(key) +if bz == nil { + return 0 +} return sdk.BigEndianToUint64(bz)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.bz := store.Get(key) if bz == nil { return 0 } return sdk.BigEndianToUint64(bz)
26-27:
⚠️ Potential issueHandle nil value when unmarshalling consensus addresses
In
GetConsAddrsToPrune, if the key does not exist in the store,store.Get(key)will returnnil. Passingniltok.cdc.MustUnmarshalwill cause a panic. Please add a nil check before unmarshalling to prevent potential runtime errors.Apply this diff to handle the nil case safely:
key := types.ConsAddrsToPruneKey(chainID, vscID) -k.cdc.MustUnmarshal(store.Get(key), &res) +bz := store.Get(key) +if bz == nil { + return res +} +k.cdc.MustUnmarshal(bz, &res)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.key := types.ConsAddrsToPruneKey(chainID, vscID) bz := store.Get(key) if bz == nil { return res } k.cdc.MustUnmarshal(bz, &res) return res
92-93:
⚠️ Potential issuePrevent panic by handling nil value during unmarshalling
In
GetUndelegationsToRelease, similar to the previous functions,store.Get(key)may returnnilif the key is not found. This will causek.cdc.MustUnmarshalto panic. Adding a nil check before unmarshalling will prevent this issue.Apply this diff to handle the potential nil value:
key := types.UndelegationsToReleaseKey(chainID, vscID) -k.cdc.MustUnmarshal(store.Get(key), &res) +bz := store.Get(key) +if bz == nil { + return res +} +k.cdc.MustUnmarshal(bz, &res)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.key := types.UndelegationsToReleaseKey(chainID, vscID) bz := store.Get(key) if bz == nil { return res } k.cdc.MustUnmarshal(bz, &res) return resx/appchain/subscriber/types/keys.go (1)
94-96:
⚠️ Potential issueEnsure consistent use of
ConsAddressconversionIn
SubscriberValidatorKey, theaddressis appended directly:return append([]byte{SubscriberValidatorBytePrefix}, address...)In contrast,
OutstandingDowntimeKeyusesconsAddress.Bytes():return append([]byte{OutstandingDowntimeBytePrefix}, consAddress.Bytes()...)For consistency and to prevent potential issues, consider using
address.Bytes()inSubscriberValidatorKeyifsdk.ConsAddresssupports theBytes()method.Apply this diff if applicable:
func SubscriberValidatorKey(address sdk.ConsAddress) []byte { - return append([]byte{SubscriberValidatorBytePrefix}, address...) + return append([]byte{SubscriberValidatorBytePrefix}, address.Bytes()...) }x/appchain/subscriber/keeper/impl_sdk.go (3)
129-135:
⚠️ Potential issueAvoid using
panicin unimplemented methodsThe
Delegationmethod panics when called. Returningnilor an error is safer and prevents the application from crashing unexpectedly if this method is called.Apply this diff to handle the method safely:
-func (k Keeper) Delegation( - sdk.Context, - sdk.AccAddress, - sdk.ValAddress, -) stakingtypes.DelegationI { - panic("unimplemented on this keeper") +func (k Keeper) Delegation( + ctx sdk.Context, + delAddr sdk.AccAddress, + valAddr sdk.ValAddress, +) stakingtypes.DelegationI { + return nil }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) Delegation( ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValAddress, ) stakingtypes.DelegationI { return nil }
48-50:
⚠️ Potential issueAvoid using
panicin unimplemented methodsThe
Validatormethod panics when called, which can cause the application to crash unexpectedly if this method is invoked. Consider returningnilor an appropriate error to handle the unimplemented method more gracefully.Apply this diff to handle the method safely:
-func (k Keeper) Validator(sdk.Context, sdk.ValAddress) stakingtypes.ValidatorI { - panic("unimplemented on this keeper") +func (k Keeper) Validator(ctx sdk.Context, addr sdk.ValAddress) stakingtypes.ValidatorI { + return nil }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) Validator(ctx sdk.Context, addr sdk.ValAddress) stakingtypes.ValidatorI { return nil }
67-80:
⚠️ Potential issueClarify the usage of the
SlashmethodThe
Slashmethod is implemented but the comment indicates it "should not be called anywhere." If this method must exist to satisfy an interface, consider documenting its intended usage more clearly or returning immediately with a no-op to prevent unintended behavior.Apply this diff to clarify the method's usage:
-func (k Keeper) Slash( - ctx sdk.Context, - addr sdk.ConsAddress, - infractionHeight, power int64, - slashFactor sdk.Dec, -) math.Int { - return k.SlashWithInfractionReason( - ctx, addr, infractionHeight, - power, slashFactor, - stakingtypes.Infraction_INFRACTION_UNSPECIFIED, - ) +func (k Keeper) Slash( + ctx sdk.Context, + addr sdk.ConsAddress, + infractionHeight, power int64, + slashFactor sdk.Dec, +) math.Int { + // This method should not be called; add logging or handle appropriately. + return math.ZeroInt() }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Calls SlashWithInfractionReason with Infraction_INFRACTION_UNSPECIFIED. // It should not be called anywhere. func (k Keeper) Slash( ctx sdk.Context, addr sdk.ConsAddress, infractionHeight, power int64, slashFactor sdk.Dec, ) math.Int { // This method should not be called; add logging or handle appropriately. return math.ZeroInt() }x/appchain/coordinator/keeper/timeout.go (3)
121-129:
⚠️ Potential issueAvoid potential panic in
GetFirstVscTimeoutby handling errors during unmarshalingIn the loop, if the unmarshaling fails due to corrupted data, it could cause a panic. It would be prudent to handle the error to maintain application stability.
Suggested modification:
if iterator.Valid() { _, _, err := types.ParseVscTimeoutKey(iterator.Key()) if err != nil { return timeout, false } bz := iterator.Value() - k.cdc.MustUnmarshal(bz, &timeout) + err = k.cdc.Unmarshal(bz, &timeout) + if err != nil { + return timeout, false // Handle the error appropriately + } return timeout, true }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if iterator.Valid() { _, _, err := types.ParseVscTimeoutKey(iterator.Key()) if err != nil { return timeout, false } bz := iterator.Value() err = k.cdc.Unmarshal(bz, &timeout) if err != nil { return timeout, false // Handle the error appropriately } return timeout, true }
68-79:
⚠️ Potential issuePrevent potential panic in
GetChainInitTimeoutby handling unmarshal errorsUsing
k.cdc.MustUnmarshalcan cause a panic if the stored data is corrupted or improperly formatted. To enhance robustness, consider usingk.cdc.Unmarshaland handling the error accordingly.Apply this diff to handle the unmarshal error gracefully:
if bz == nil { return epoch, false } -k.cdc.MustUnmarshal(bz, &epoch) +err := k.cdc.Unmarshal(bz, &epoch) +if err != nil { + return epoch, false // Handle the error as appropriate +} return epoch, true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// GetChainInitTimeout returns the epoch by the end of which the chain must be initialized. func (k Keeper) GetChainInitTimeout( ctx sdk.Context, chainID string, ) (epoch epochstypes.Epoch, found bool) { store := ctx.KVStore(k.storeKey) bz := store.Get(types.ChainInitTimeoutKey(chainID)) if bz == nil { return epoch, false } err := k.cdc.Unmarshal(bz, &epoch) if err != nil { return epoch, false // Handle the error as appropriate } return epoch, true }
96-107:
⚠️ Potential issueEnsure safe unmarshaling in
GetVscTimeoutto prevent panicsSimilar to
GetChainInitTimeout, usingMustUnmarshalhere might lead to a panic if the data is invalid. It's safer to useUnmarshaland handle potential errors.Here’s the suggested change:
if bz == nil { return timeout, false } -k.cdc.MustUnmarshal(bz, &timeout) +err := k.cdc.Unmarshal(bz, &timeout) +if err != nil { + return timeout, false // Handle the error as appropriate +} return timeout, true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// GetVscTimeout returns the epoch by the end of which a response to a VSC must be received. func (k Keeper) GetVscTimeout( ctx sdk.Context, chainID string, vscID uint64, ) (timeout epochstypes.Epoch, found bool) { store := ctx.KVStore(k.storeKey) bz := store.Get(types.VscTimeoutKey(chainID, vscID)) if bz == nil { return timeout, false } err := k.cdc.Unmarshal(bz, &timeout) if err != nil { return timeout, false // Handle the error as appropriate } return timeout, true }x/appchain/coordinator/keeper/connection.go (1)
95-135: 🛠️ Refactor suggestion
Adjust log level for missing timeout in
SetSubscriberChainIn the
SetSubscriberChainfunction, when the initialization timeout is not found for a chain (lines 133-135), an error is logged:k.Logger(ctx).Error("timeout not found for chain", "chainID", chainID)If the absence of a timeout is a possible and expected condition, consider changing the log level to
WarnorInfoto prevent unnecessary error logging.x/appchain/coordinator/keeper/slash.go (1)
57-57: 🛠️ Refactor suggestion
Remove unnecessary AVS existence check
According to our previous learning, in
HandleSlashPacket, it's unnecessary to check if the AVS exists because the packet must be received over a channel belonging to a chain known to the coordinator module, which mandatorily registers the AVS. The call tok.avsKeeper.IsAVSByChainIDis redundant.Consider directly retrieving the
avsAddresswithout checking for existence.- _, avsAddress := k.avsKeeper.IsAVSByChainID(ctx, chainID) + avsAddress := k.avsKeeper.GetAVSAddress(ctx, chainID)Ensure that
GetAVSAddressis implemented to retrieve the AVS address associated with thechainID.Committable suggestion was skipped due to low confidence.
x/appchain/coordinator/keeper/relay.go (3)
98-123:
⚠️ Potential issueImplement the
HandleVscMaturedPacketfunctionThe
HandleVscMaturedPacketfunction currently contains only commented-out code and lacks an implementation. This function is critical for processing VSC matured packets, managing undelegations, and pruning consensus keys.Would you like assistance in implementing this function or opening a GitHub issue to ensure this task is tracked and completed?
149-166:
⚠️ Potential issueInconsistent channel identifiers in
OnTimeoutPacketIn the
OnTimeoutPacketfunction, there are inconsistencies with the use ofpacket.SourceChannelandpacket.DestinationChannel:
- At line 150, you retrieve the chain using
packet.SourceChannel.- At line 153, you log
packet.SourceChannelas the"channelID".- At line 156, the error message references
packet.DestinationChannel.To maintain consistency and accuracy, ensure that the same channel identifier is used throughout the function.
Apply the following diff to correct the inconsistencies:
- return types.ErrUnknownSubscriberChannelID.Wrapf( - "ack packet on %s", packet.DestinationChannel, - ) + return types.ErrUnknownSubscriberChannelID.Wrapf( + "ack packet on %s", packet.SourceChannel, + )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet) error { chainID, found := k.GetChainForChannel(ctx, packet.SourceChannel) if !found { k.Logger(ctx).Error( "packet timeout, unknown channel", "channelID", packet.SourceChannel, ) return types.ErrUnknownSubscriberChannelID.Wrapf( "ack packet on %s", packet.SourceChannel, ) } // stop chain and release unbondings k.Logger(ctx).Info( "packet timeout, removing the subscriber", "chainID", chainID, ) return k.StopSubscriberChain(ctx, chainID, false) }
131-145:
⚠️ Potential issueInconsistent use of packet channel identifiers in
OnAcknowledgementPacketThere is inconsistent use of
packet.SourceChannelandpacket.DestinationChannelwithin theOnAcknowledgementPacketfunction:
- At line 134, you log
packet.SourceChannelas the"channelID".- At line 137, you call
GetChainForChannelwithpacket.DestinationChannel.- At line 140, the error message references
packet.DestinationChannel.Ensure consistent use of either
SourceChannelorDestinationChannelto prevent confusion and potential errors.Apply the following diff to correct the inconsistencies:
- k.Logger(ctx).Error( - "recv ErrorAcknowledgement", - "channelID", packet.SourceChannel, - "error", err, - ) + k.Logger(ctx).Error( + "recv ErrorAcknowledgement", + "channelID", packet.DestinationChannel, + "error", err, + )- if chainID, ok := k.GetChainForChannel(ctx, packet.DestinationChannel); ok { + if chainID, ok := k.GetChainForChannel(ctx, packet.SourceChannel); ok {- return types.ErrUnknownSubscriberChannelID.Wrapf( - "ack packet on %s", packet.DestinationChannel, - ) + return types.ErrUnknownSubscriberChannelID.Wrapf( + "ack packet on %s", packet.SourceChannel, + )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if err := ack.GetError(); err != "" { k.Logger(ctx).Error( "recv ErrorAcknowledgement", "channelID", packet.DestinationChannel, "error", err, ) if chainID, ok := k.GetChainForChannel(ctx, packet.SourceChannel); ok { return k.StopSubscriberChain(ctx, chainID, false) } return types.ErrUnknownSubscriberChannelID.Wrapf( "ack packet on %s", packet.SourceChannel, ) } return nil }x/appchain/subscriber/keeper/keeper.go (5)
18-29: 🛠️ Refactor suggestion
Consider reducing dependencies in the
KeeperstructThe
Keeperstruct now includes multiple dependencies, which increases coupling and can affect maintainability. Assess whether all these dependencies are necessary for theKeeper. If possible, refactor to minimize dependencies or group related interfaces.
97-101:
⚠️ Potential issueEnhance error context in
ClaimCapabilityIn
ClaimCapability, if an error occurs while claiming the capability, consider adding contextual information to the error message. This will aid in debugging and provide clearer logs.Apply this diff to enhance error handling:
func (k Keeper) ClaimCapability( ctx sdk.Context, cap *capabilitytypes.Capability, name string, ) error { - return k.scopedKeeper.ClaimCapability(ctx, cap, name) + if err := k.scopedKeeper.ClaimCapability(ctx, cap, name); err != nil { + return fmt.Errorf("failed to claim capability %s: %w", name, err) + } + return nil }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.cap *capabilitytypes.Capability, name string, ) error { if err := k.scopedKeeper.ClaimCapability(ctx, cap, name); err != nil { return fmt.Errorf("failed to claim capability %s: %w", name, err) } return nil }
70-72:
⚠️ Potential issueHandle potential
nilreturn value fromstore.GetIn
GetPort, if the keytypes.PortKey()does not exist in the store,store.Getwill returnnil. Convertingnilto a string results in an empty string. Ensure this behavior is intentional. If not, handle thenilcase explicitly to avoid unexpected results.Apply this diff to handle
nilvalues:func (k Keeper) GetPort(ctx sdk.Context) string { store := ctx.KVStore(k.storeKey) - return string(store.Get(types.PortKey())) + bz := store.Get(types.PortKey()) + if bz == nil { + return "" + } + return string(bz) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) GetPort(ctx sdk.Context) string { store := ctx.KVStore(k.storeKey) bz := store.Get(types.PortKey()) if bz == nil { return "" } return string(bz) }
190-194: 🛠️ Refactor suggestion
Validate the value set in
SetOutstandingDowntimeIn
SetOutstandingDowntime, a byte value{1}is stored to represent the outstanding downtime flag. Confirm that this convention is consistently used throughout the code and that retrieving this flag handles the byte correctly. Consider defining a constant for the flag value for clarity.Apply this diff to use a constant:
+const OutstandingDowntimeValue = byte(1) func (k Keeper) SetOutstandingDowntime( ctx sdk.Context, consAddress sdk.ConsAddress, ) { store := ctx.KVStore(k.storeKey) - store.Set(types.OutstandingDowntimeKey(consAddress), []byte{1}) + store.Set(types.OutstandingDowntimeKey(consAddress), []byte{OutstandingDowntimeValue}) }Committable suggestion was skipped due to low confidence.
88-91:
⚠️ Potential issueCheck for
nilcapability when binding portIn
BindPort, after callingk.portKeeper.BindPort, the returnedcapabilitymight benilif the port binding fails. It's important to check for anilcapability before proceeding to claim it to prevent potentialnilpointer dereferences.Apply this diff to handle a
nilcapability:func (k Keeper) BindPort(ctx sdk.Context, portID string) error { capability := k.portKeeper.BindPort(ctx, portID) + if capability == nil { + return fmt.Errorf("failed to bind port: %s", portID) + } return k.ClaimCapability(ctx, capability, host.PortPath(portID)) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (k Keeper) BindPort(ctx sdk.Context, portID string) error { capability := k.portKeeper.BindPort(ctx, portID) if capability == nil { return fmt.Errorf("failed to bind port: %s", portID) } return k.ClaimCapability(ctx, capability, host.PortPath(portID)) }x/appchain/subscriber/module.go (2)
166-171:
⚠️ Potential issueHandle potential errors when retrieving the coordinator channel
The code retrieves the coordinator channel using
GetCoordinatorChannel, but it only checks if the channel is found. Consider handling scenarios where the retrieval might fail or return unexpected results to ensure robustness.
190-199:
⚠️ Potential issueEnsure error handling for packet transmission functions
The functions
EndBlockSendRewards,QueueVscMaturedPackets, andSendPacketsare called without checking for errors. To prevent silent failures and improve reliability, handle any errors returned by these functions.Consider applying the following changes to handle potential errors:
-am.keeper.EndBlockSendRewards(ctx) -am.keeper.QueueVscMaturedPackets(ctx) -am.keeper.SendPackets(ctx) +if err := am.keeper.EndBlockSendRewards(ctx); err != nil { + am.keeper.Logger(ctx).Error("Failed to send rewards at end block", "error", err) +} +if err := am.keeper.QueueVscMaturedPackets(ctx); err != nil { + am.keeper.Logger(ctx).Error("Failed to queue VSC matured packets", "error", err) +} +if err := am.keeper.SendPackets(ctx); err != nil { + am.keeper.Logger(ctx).Error("Failed to send packets", "error", err) +}Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 191-191: Panic in BeginBock or EndBlock consensus methods
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic call
path flow from Begin/EndBlock to a panic callx/appchain/subscriber/keeper/rewards.go (5)
22-22: 🛠️ Refactor suggestion
Consider handling the error instead of just logging it
In
EndBlockSendRewards, whenSendRewardsToCoordinatorreturns an error, the function logs the error but continues execution. Depending on the application's requirements, it might be more appropriate to handle the error or return it to ensure that issues are addressed promptly.
70-73:
⚠️ Potential issueAvoid panicking on errors from
SendCoinsFromModuleToModulePanicking during consensus-related operations can cause a chain halt. Instead of panicking when
SendCoinsFromModuleToModulefails, handle the error to maintain chain stability.Apply this diff to handle the error gracefully:
if err != nil { - // It is the common behavior in cosmos-sdk to panic if SendCoinsFromModuleToModule - // returns error. - panic(err) + k.Logger(ctx).Error("failed to send coins for subscriber redistribution", "error", err) + return }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if err != nil { k.Logger(ctx).Error("failed to send coins for subscriber redistribution", "error", err) return }🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 72-72: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
101-102:
⚠️ Potential issueHandle nil value when retrieving last reward transmission height
The
Getmethod might returnnilif the key doesn't exist, which could causeBigEndianToUint64to panic. Add a check to handle thenilcase and set a default value.Apply this diff:
bz := store.Get(types.LastRewardTransmissionHeightKey()) +if bz == nil { + return 0 // or an appropriate default value +} return int64(sdk.BigEndianToUint64(bz))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.bz := store.Get(types.LastRewardTransmissionHeightKey()) if bz == nil { return 0 // or an appropriate default value } return int64(sdk.BigEndianToUint64(bz)) }
50-53:
⚠️ Potential issueHandle parameter parsing errors without panicking
When parsing
SubscriberRedistributionFraction, the code panics if there is an error. Panicking in consensus-related methods can cause a chain halt. Instead, handle the error gracefully to enhance robustness.Apply this diff to handle the error without panicking:
frac, err := sdk.NewDecFromStr(k.GetSubscriberParams(ctx).SubscriberRedistributionFraction) if err != nil { - // should not happen since we validated this in the params - panic(err) + k.Logger(ctx).Error("failed to parse SubscriberRedistributionFraction", "error", err) + return }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.frac, err := sdk.NewDecFromStr(k.GetSubscriberParams(ctx).SubscriberRedistributionFraction) if err != nil { k.Logger(ctx).Error("failed to parse SubscriberRedistributionFraction", "error", err) return }🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 53-53: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
75-82:
⚠️ Potential issueConsistently handle errors without panicking
Similar to the previous comment, avoid panicking when sending remaining tokens to the coordinator module fails. Consistent error handling enhances the resilience of the application.
Apply this diff:
if err != nil { - // It is the common behavior in cosmos-sdk to panic if SendCoinsFromModuleToModule - // returns error. - panic(err) + k.Logger(ctx).Error("failed to send remaining tokens to coordinator module", "error", err) + return }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.remainingTokens := fpTokens.Sub(subsRedistrTokens...) err = k.bankKeeper.SendCoinsFromModuleToModule(ctx, k.feeCollectorName, types.SubscriberToSendToCoordinatorName, remainingTokens) if err != nil { k.Logger(ctx).Error("failed to send remaining tokens to coordinator module", "error", err) return }🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 81-81: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain haltx/appchain/subscriber/keeper/validators.go (2)
226-227:
⚠️ Potential issueAvoid Panics in Consensus Methods to Prevent Chain Halt
Using
panicin consensus methods likeBeginBlockorEndBlockcan cause the entire blockchain to halt if an unexpected error occurs. Instead of panicking, consider handling the error gracefully and logging the error. This ensures the node can continue operating or at least fail without affecting the entire network.Apply this fix to handle errors without panicking:
At lines 226-227:
- panic(err) + k.Logger(ctx).Error("failed to get consensus public key", "error", err) + returnAt lines 232-233:
- panic(err) + k.Logger(ctx).Error("failed to create new validator", "error", err) + returnAlso applies to: 232-233
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 226-226: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
205-212: 🛠️ Refactor suggestion
Optimize Pruning Logic in Historical Info Tracking
The current pruning loop iterates from
ctx.BlockHeight() - numHistoricalEntriesdown to0, checking each height for historical info to delete. This could be inefficient for large block heights. Consider optimizing by calculating the minimum height to keep and directly deleting entries below that height without iteration.Apply this refactored logic:
func (k Keeper) TrackHistoricalInfo(ctx sdk.Context) { numHistoricalEntries := int64(k.GetParams(ctx).HistoricalEntries) + if numHistoricalEntries == 0 { + return + } + minHeight := ctx.BlockHeight() - numHistoricalEntries + 1 + iterator := sdk.KVStorePrefixIterator(k.storeKey, types.HistoricalInfoPrefix) + defer iterator.Close() + for ; iterator.Valid(); iterator.Next() { + hiHeight := types.GetHeightFromHistoricalInfoKey(iterator.Key()) + if hiHeight < minHeight { + k.DeleteHistoricalInfo(ctx, hiHeight) + } + } // Existing logic to store new historical info... }Committable suggestion was skipped due to low confidence.
x/appchain/coordinator/module_ibc.go (1)
85-92: 🛠️ Refactor suggestion
Implement version negotiation for enhanced compatibility
Currently,
OnChanOpenTrystrictly checks thatcounterpartyVersionmatchescommontypes.Version. Implementing a version negotiation strategy would improve flexibility and allow compatibility with counterparties using different versions.Apply this diff to facilitate version negotiation:
- if counterpartyVersion != commontypes.Version { + // Accept any counterparty version for negotiation purposes + if err := im.keeper.ValidateCounterpartyVersion(counterpartyVersion); err != nil { return "", errorsmod.Wrapf( commontypes.ErrInvalidVersion, "invalid counterparty version: got: %s, expected %s", counterpartyVersion, commontypes.Version, ) }And define the
ValidateCounterpartyVersionmethod accordingly in your keeper.Committable suggestion was skipped due to low confidence.
utils/utils.go (2)
288-292:
⚠️ Potential issueAvoid Using
panicin Library CodeThe
PanicIfNilfunction usespanicto handle anilinput. In library code, it's recommended to return an error instead of panicking, to allow the caller to handle the error gracefully.Apply this diff to return an error instead:
-func PanicIfNil(x interface{}, msg string) { - if x == nil { - panic("zero or nil value for " + msg) - } -} +func ErrorIfNil(x interface{}, msg string) error { + if x == nil { + return fmt.Errorf("zero or nil value for %s", msg) + } + return nil +}Don't forget to import the
"fmt"package if it's not already imported.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func ErrorIfNil(x interface{}, msg string) error { if x == nil { return fmt.Errorf("zero or nil value for %s", msg) } return nil }
248-254: 🛠️ Refactor suggestion
Optimize Slice Allocation in
AppendManyFunctionIn the
AppendManyfunction, preallocating the capacity of the output sliceoutcan improve performance by reducing the number of memory allocations during the append operations.Apply this diff to preallocate the slice:
func AppendMany(byteses ...[]byte) (out []byte) { + var totalLen int + for _, bytes := range byteses { + totalLen += len(bytes) + } + out = make([]byte, 0, totalLen) for _, bytes := range byteses { out = append(out, bytes...) } return out }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// AppendMany appends a variable number of byte slices together func AppendMany(byteses ...[]byte) (out []byte) { var totalLen int for _, bytes := range byteses { totalLen += len(bytes) } out = make([]byte, 0, totalLen) for _, bytes := range byteses { out = append(out, bytes...) } return out }x/appchain/coordinator/keeper/validator_set_update.go (4)
87-88:
⚠️ Potential issueReview loop termination condition
In the loop starting at line 82, the condition
if power < 1 { break }will exit the loop when a validator has a power less than 1. This could skip subsequent validators with non-zero power if the list isn't sorted by power in descending order. Ensure that the validators are appropriately sorted before this loop, or adjust the condition to avoid unintentionally skipping valid validators.
216-219:
⚠️ Potential issueAvoid shadowing the
errvariableIn the
SendVscPacketsToChainfunction, theerrvariable is re-declared within theifblock at line 216, which shadows the outererrvariable. This can lead to confusion and potential bugs.Suggested fix:
Avoid re-declaring
errby using=instead of:=:- err := k.StopSubscriberChain(ctx, chainID, true) + err = k.StopSubscriberChain(ctx, chainID, true) if err != nil { panic(fmt.Errorf("subscriber chain failed to stop: %w", err)) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.err = k.StopSubscriberChain(ctx, chainID, true) if err != nil { panic(fmt.Errorf("subscriber chain failed to stop: %w", err)) }
216-220: 🛠️ Refactor suggestion
Consider handling errors without panicking
When
k.StopSubscriberChainreturns an error, the code panics. Panicking can halt the entire application, which might not be desirable in a production environment. Consider handling the error gracefully to maintain system stability.Suggested change:
Handle the error without panicking:
err = k.StopSubscriberChain(ctx, chainID, true) if err != nil { - panic(fmt.Errorf("subscriber chain failed to stop: %w", err)) + k.Logger(ctx).Error( + "failed to stop subscriber chain", + "chainID", chainID, + "error", err, + ) + // Additional error handling as necessary return }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.err := k.StopSubscriberChain(ctx, chainID, true) if err != nil { k.Logger(ctx).Error( "failed to stop subscriber chain", "chainID", chainID, "error", err, ) // Additional error handling as necessary return } return
242-245:
⚠️ Potential issueHandle nil values when unmarshaling
In the
GetPendingVscPacketsfunction, if thevalueretrieved from the store isnil, callingk.cdc.MustUnmarshal(value, &data)will panic. Add a check fornilbefore attempting to unmarshal to prevent potential crashes.Suggested fix:
value := store.Get(key) + if value == nil { + return types.ValidatorSetChangePackets{} + } k.cdc.MustUnmarshal(value, &data)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.key := types.ChainIDToVscPacketsKey(chainID) value := store.Get(key) if value == nil { return types.ValidatorSetChangePackets{} } k.cdc.MustUnmarshal(value, &data) return datax/appchain/coordinator/types/keys.go (4)
133-133:
⚠️ Potential issueComplete the incomplete comment
The comment on line 133 is incomplete:
// safe to do, since.... Please complete the comment to provide clarity on why this operation is safe.
187-193:
⚠️ Potential issueInconsistent key construction using
chainIDIn
SubscriberValidatorKey, thechainIDis processed usingutils.ChainIDWithLenKey(chainID), which includes the length of thechainIDin the key. However, in other key functions likeChannelForChainKeyandChainForChannelKey, thechainIDis appended directly without including its length. This inconsistency might lead to issues in key generation and retrieval.Consider standardizing the key construction method for
chainIDacross all functions to ensure consistency.
140-140:
⚠️ Potential issueRemove informal language from comment
The comment
// PortKey returns the key for the port (hello Harry Potter!)contains informal language. It's recommended to keep comments professional and focused on the code functionality.Update the comment to:
-// PortKey returns the key for the port (hello Harry Potter!) +// PortKey returns the key for the port📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// PortKey returns the key for the port
5-5:
⚠️ Potential issueRemove redundant import alias for 'fmt' package
The import statement
fmt "fmt"unnecessarily aliases thefmtpackage to itself. In Go, this is redundant and can be simplified by directly importing"fmt"without an alias.Apply this diff to remove the unnecessary alias:
-import fmt "fmt" +import "fmt"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements."fmt"x/appchain/subscriber/keeper/relay.go (2)
26-35:
⚠️ Potential issueReturn an error acknowledgement instead of
nilwhen receiving a VSC packet on a non-coordinator channel.In the
OnRecvVSCPacketmethod, when a VSC packet is received on a non-coordinator channel, the function logs an error and returnsnil. Returningnilmay not be appropriate, as the caller might expect an acknowledgement to handle the error properly. Consider returning an error acknowledgement to signal the issue.Apply this diff to return an error acknowledgement:
func (k Keeper) OnRecvVSCPacket( ctx sdk.Context, packet channeltypes.Packet, data commontypes.ValidatorSetChangePacketData, ) exported.Acknowledgement { coordinatorChannel, found := k.GetCoordinatorChannel(ctx) if found && packet.SourceChannel != coordinatorChannel { // should never happen k.Logger(ctx).Error( "received VSCPacket on non-coordinator channel", "source channel", packet.SourceChannel, "coordinator channel", coordinatorChannel, ) - return nil + return channeltypes.NewErrorAcknowledgement(fmt.Sprintf( + "received VSCPacket on non-coordinator channel: expected %s, got %s", + coordinatorChannel, packet.SourceChannel, + )) } if !found { // first message on channel📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.coordinatorChannel, found := k.GetCoordinatorChannel(ctx) if found && packet.SourceChannel != coordinatorChannel { // should never happen k.Logger(ctx).Error( "received VSCPacket on non-coordinator channel", "source channel", packet.SourceChannel, "coordinator channel", coordinatorChannel, ) return channeltypes.NewErrorAcknowledgement(fmt.Sprintf( "received VSCPacket on non-coordinator channel: expected %s, got %s", coordinatorChannel, packet.SourceChannel, )) }
252-283: 🛠️ Refactor suggestion
Continue processing remaining packets after an error occurs in
SendPackets.In the
SendPacketsmethod, if an error occurs while sending a packet, the function returns immediately, skipping the processing of remaining packets. This could prevent successfully sending other pending packets. Consider modifying the error handling to log the error and continue processing the rest of the packets. Additionally, if the IBC client is expired, it may be appropriate to break out of the loop.Apply this diff to adjust the error handling:
for i := range pending { p := pending[i] // Send packet over IBC err := commontypes.SendIBCPacket( ctx, k.scopedKeeper, k.channelKeeper, channelID, // source channel id commontypes.SubscriberPortID, // source port id commontypes.ModuleCdc.MustMarshalJSON(&p.SubscriberPacketData), timeoutPeriod, ) if err != nil { if clienttypes.ErrClientNotActive.Is(err) { // IBC client is expired! // leave the packet data stored to be sent once the client is upgraded k.Logger(ctx).Info( "IBC client is expired, cannot send IBC packet; leaving packet data stored:", "type", p.Type.String(), ) - return + break } // Not able to send packet over IBC! // Leave the packet data stored for the sent to be retried in the next block. // Note that if VSCMaturedPackets are not sent for long enough, the coordinator // will remove the subscriber anyway. k.Logger(ctx).Error( "cannot send IBC packet; leaving packet data stored:", "type", p.Type.String(), "err", err.Error(), ) - return + continue } if p.Type == commontypes.VscMaturedPacket { id := p.GetVscMaturedPacketData().ValsetUpdateID📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.for i := range pending { p := pending[i] // Send packet over IBC err := commontypes.SendIBCPacket( ctx, k.scopedKeeper, k.channelKeeper, channelID, // source channel id commontypes.SubscriberPortID, // source port id commontypes.ModuleCdc.MustMarshalJSON(&p.SubscriberPacketData), timeoutPeriod, ) if err != nil { if clienttypes.ErrClientNotActive.Is(err) { // IBC client is expired! // leave the packet data stored to be sent once the client is upgraded k.Logger(ctx).Info( "IBC client is expired, cannot send IBC packet; leaving packet data stored:", "type", p.Type.String(), ) break } // Not able to send packet over IBC! // Leave the packet data stored for the sent to be retried in the next block. // Note that if VSCMaturedPackets are not sent for long enough, the coordinator // will remove the subscriber anyway. k.Logger(ctx).Error( "cannot send IBC packet; leaving packet data stored:", "type", p.Type.String(), "err", err.Error(), ) continue } if p.Type == commontypes.VscMaturedPacket { id := p.GetVscMaturedPacketData().ValsetUpdateIDx/appchain/subscriber/module_ibc.go (5)
241-258: 🛠️ Refactor suggestion
Ensure consistent error handling in
OnChanCloseInitThe
OnChanCloseInitmethod permits relayers to close duplicate channels but returns an error for other cases. To maintain consistency and clarity, consider refining the error messages or adjusting the conditions under which errors are returned.Review the logic to ensure that valid channel closure requests are handled appropriately and provide more descriptive error messages where necessary.
225-239:
⚠️ Potential issueReconsider error handling in
OnChanOpenConfirmmethodThe
OnChanOpenConfirmmethod currently returns an error indicating that the channel handshake must be initiated by the subscriber chain. However, since the subscriber chain initiated the handshake, this method should proceed without error upon confirmation from the counterparty.Modify the method to allow successful channel confirmation:
func (im IBCModule) OnChanOpenConfirm( ctx sdk.Context, _ string, _ string, ) error { im.keeper.Logger(ctx).Debug( "OnChanOpenConfirm", ) - return errorsmod.Wrap( - commontypes.ErrInvalidChannelFlow, - "channel handshake must be initiated by subscriber chain", - ) + return nil }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// OnChanOpenConfirm implements the IBCModule interface func (im IBCModule) OnChanOpenConfirm( ctx sdk.Context, _ string, _ string, ) error { im.keeper.Logger(ctx).Debug( "OnChanOpenConfirm", ) return nil }
285-290:
⚠️ Potential issueInclude error details in packet unmarshal acknowledgement
When unmarshalling packet data fails, the error acknowledgement does not include the underlying error details. Including the error information can aid in debugging and provide clarity on the failure cause.
Update the error acknowledgement to include error details:
if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { - errAck := commontypes.NewErrorAcknowledgementWithLog( - ctx, fmt.Errorf("cannot unmarshal packet data"), - ) + errAck := commontypes.NewErrorAcknowledgementWithLog( + ctx, fmt.Errorf("cannot unmarshal packet data: %v", err), + ) ack = &errAck } else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.if err := types.ModuleCdc.UnmarshalJSON(packet.GetData(), &data); err != nil { errAck := commontypes.NewErrorAcknowledgementWithLog( ctx, fmt.Errorf("cannot unmarshal packet data: %v", err), ) ack = &errAck } else {
174-176:
⚠️ Potential issueValidate coordinator fee pool address before setting it
Before setting the
CoordinatorFeePoolAddr, it's crucial to validate that the address provided is a valid Bech32 address. This prevents potential issues arising from malformed or malicious data.Add address validation:
// This address is not required to be supplied at the time of chain registration. // Rather, it is set later by the coordinator chain. +if _, err := sdk.AccAddressFromBech32(md.CoordinatorFeePoolAddr); err != nil { + return errorsmod.Wrapf( + sdkerrors.ErrInvalidAddress, + "invalid coordinator fee pool address: %v", + err, + ) +} im.keeper.SetCoordinatorFeePoolAddrStr(ctx, md.CoordinatorFeePoolAddr)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// This address is not required to be supplied at the time of chain registration. // Rather, it is set later by the coordinator chain. if _, err := sdk.AccAddressFromBech32(md.CoordinatorFeePoolAddr); err != nil { return errorsmod.Wrapf( sdkerrors.ErrInvalidAddress, "invalid coordinator fee pool address: %v", err, ) } im.keeper.SetCoordinatorFeePoolAddrStr(ctx, md.CoordinatorFeePoolAddr)
198-205:
⚠️ Potential issueSpecify a valid signer for
MsgChannelOpenInitThe
MsgChannelOpenInitmessage requires a valid signer for authentication. Currently, the signer is set to an empty string, which may lead to issues during message handling.Provide a valid signer, such as the module's account address:
distrTransferMsg := channeltypes.NewMsgChannelOpenInit( transfertypes.PortID, transfertypes.Version, channeltypes.UNORDERED, connHops, transfertypes.PortID, - "", // signer unused + im.keeper.GetModuleAddress().String(), // specify the module account as signer )Ensure that
GetModuleAddress()returns the appropriate address for signing.Committable suggestion was skipped due to low confidence.
x/appchain/coordinator/keeper/ibc_client.go (1)
27-27:
⚠️ Potential issueInconsistent usage of
subscriberandsubscriber.ChainIDin logging statementsIn the logging statements at lines 27, 35, and 46, there is inconsistency in how
subscriberorsubscriber.ChainIDis used:
- Line 27:
"chainID", subscriber.ChainID- Lines 35 and 46:
"chainID", subscriberTo maintain consistency and ensure clarity in the logs, consider always using
subscriber.ChainIDwhen logging the chain ID.Apply this diff to fix the inconsistency:
// In lines 35 and 46, update subscriber to subscriber.ChainID - "chainID", subscriber, + "chainID", subscriber.ChainID,Also applies to: 35-35, 46-46
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days-before-close if no further activity occurs. |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Tests