-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: events for Callisto #276
Conversation
+ some rationalization of functions to prevent re-opening the db unnecessarily
TODO: undelegation record creation, deletion / maturity
TODO: slash states
WalkthroughThis pull request makes extensive, cross‑module improvements to operator and asset management. It adds a new field, Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Client
participant CLI as Command Line Interface
participant Keeper as Module Keeper
participant Store as State Store
participant EventMgr as Event Manager
User->>CLI: Send operator registration request
CLI->>Keeper: newBuildRegisterOperatorMsg(msg)
Keeper->>Keeper: Validate request (ensure addr matches EarningsAddr & ApproveAddr)
alt Validation passes
Keeper->>Store: Save OperatorInfo (with ApproveAddr)
Keeper->>EventMgr: Emit registration event (operator details, metadata)
else Validation fails
Keeper-->>CLI: Return error
end
sequenceDiagram
participant AVSModule as AVS Module
participant Keeper as Dogfood Keeper
participant Store as State Store
participant EventMgr as Event Manager
AVSModule->>Keeper: Trigger BeginBlock event
Keeper->>Store: Check flag via ShouldEmitAvsEvent()
alt Flag is set
Keeper->>EventMgr: Emit AVS event (chain ID, AVS address)
Keeper->>Store: ClearEmitAvsEventFlag()
else
Keeper-->>AVSModule: No event emitted
end
Possibly related PRs
Suggested reviewers
Poem
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
x/operator/types/validator.go (1)
22-33
: 💡 Verification agent🧩 Analysis chain
Verify address equality enforcement.
The code sets
OperatorApproveAddr
equal toOperatorEarningsAddr
, which aligns with the breaking change requirement. However, we should ensure this equality is enforced throughout the lifecycle of a Validator.
🏁 Script executed:
#!/bin/bash # Search for any validator update operations that might modify these addresses independently ast-grep --pattern 'func $_(v *Validator) { $$$ v.OperatorApproveAddr = $_ $$$ }' ast-grep --pattern 'func $_(v *Validator) { $$$ v.OperatorEarningsAddr = $_ $$$ }'Length of output: 180
Double-check operator address consistency.
The current implementation in
x/operator/types/validator.go
correctly initializesOperatorApproveAddr
to the same value asOperatorEarningsAddr
per the breaking change requirement. However, we should manually verify that no subsequent code updates these addresses independently. Please run the above script to ensure that there are no assignment operations modifying these fields separately in the codebase.
🏁 Script executed:
#!/bin/bash # Further verify that there are no independent assignments modifying the operator addresses. echo "Searching for assignments to OperatorApproveAddr:" rg --no-heading --line-number "OperatorApproveAddr\s*=" || true echo "Searching for assignments to OperatorEarningsAddr:" rg --no-heading --line-number "OperatorEarningsAddr\s*=" || trueLength of output: 693
Attention required on operator address assignments.
The constructor in
x/operator/types/validator.go
properly initializes both address fields with the same value. However, our verification reveals that downstream inx/operator/keeper/consensus_keys.go
, these fields are assigned independently fromops.EarningsAddr
andops.ApproveAddr
respectively without any explicit equality enforcement being checked. Please ensure that (a) the external sources (ops
) provide identical values for these addresses or (b) add validation in consensus_keys.go to enforce that both addresses remain equal.
🏁 Script executed:
#!/bin/bash # Verify if there's any check enforcing equality between OperatorEarningsAddr and OperatorApproveAddr in consensus_keys.go echo "Searching consensus_keys.go for equality check between ops.EarningsAddr and ops.ApproveAddr:" rg --no-heading --line-number "if\s*.*ops\.EarningsAddr\s*!=\s*ops\.ApproveAddr" x/operator/keeper/consensus_keys.go || true # Additionally, search the entire repository for any equality check comparing OperatorApproveAddr and OperatorEarningsAddr echo "Searching entire repository for equality check of OperatorApproveAddr and OperatorEarningsAddr:" rg --no-heading --line-number "OperatorEarningsAddr\s*==\s*OperatorApproveAddr" || trueLength of output: 610
Immediate action: Enforce operator address equality throughout the Validator lifecycle.
- The constructor in
x/operator/types/validator.go
correctly setsOperatorApproveAddr
to the same value asOperatorEarningsAddr
.- However, in
x/operator/keeper/consensus_keys.go
, the addresses are updated independently (val.OperatorEarningsAddr = ops.EarningsAddr
andval.OperatorApproveAddr = ops.ApproveAddr
) without any check to enforce equality.- Please verify that the external source (
ops
) guarantees that these values are identical or add explicit validation to enforce the invariant—ensuring both addresses remain equal after any update.x/delegation/keeper/share.go (1)
228-234
:⚠️ Potential issueConsider handling the error from UpdateStakerAssetState.
The error from
UpdateStakerAssetState
is being discarded. While the error is assigned to a variable, it's not being checked or propagated. This could lead to silent failures in state updates.Apply this diff to handle the error:
- _, err = k.assetsKeeper.UpdateStakerAssetState(ctx, stakerID, assetID, assetstype.DeltaStakerSingleAsset{ + _, err := k.assetsKeeper.UpdateStakerAssetState(ctx, stakerID, assetID, assetstype.DeltaStakerSingleAsset{ PendingUndelegationAmount: removeToken, }) - if err != nil { - return removeToken, err - } + return removeToken, err
🧹 Nitpick comments (34)
x/assets/keeper/bank.go (7)
22-26
: Clarify deposit/withdraw return behavior in docstring.
The new multiline comment and signature changes accurately communicate the function's updated return format. However, it may be helpful to explicitly state that partial or zero amounts are not valid inputs, and confirm that returningsdkmath.ZeroInt()
is the intended approach for all error cases.
28-30
: Guard negative or zero amounts earlier.
The check ensuresparams.OpAmount.IsPositive()
. Consider adding a explicit doc or comment to direct callers to validate amounts upstream. This helps avoid silent or repeated checks in calling code.
37-41
: Ensure no edge cases for ExocoreAssetID checks.
This code prevents deposit/withdraw of native EXO. If there's a future requirement to handle partial minted tokens or bridging scenarios for EXO, we might need to revise or remove this guard.
44-47
: Clarify reason for excluding native asset.
This explicit return for EXO suggests a policy decision. Consider grouping this with the staking asset check or referencing a design doc on why native assets are disallowed.
50-50
: Document negativity for withdrawals.
A short inline comment clarifying that a negativeactualOpAmount
indicates a withdrawal helps future maintainers.
74-79
: Double-check constraints on total staking asset updates.
After updating the staker's share, we update the total staking asset count. This is logically correct. Confirm in tests that no rounding discrepancies arise for large deposit/withdraw amounts.
80-84
: Consider local event emission for deposit/withdraw.
Even though emitted events may be handled upstream, a local event can help with debugging. Consider adding a new event for deposit/withdraw if transparency is desired.x/assets/keeper/client_chain.go (2)
4-5
: Remove unused formatting import if no longer needed.
Line 4 adds"fmt"
. Verify if you still require it or if you only need it for event attributes. Removing unused imports helps maintain clarity.
25-25
: Confirm concurrency safety if store is used concurrently.
Although Cosmos SDK typically handles single-threaded transactions, if methods like this are invoked from separate code paths, confirm no synchronization issues can occur with the prefix store.x/delegation/keeper/delegation_state.go (3)
112-138
: Implement test coverage for new method.
The newGetDelegatedAmountByStakerAssetOperator
method effectively retrieves the user’s delegated amount. Please add or confirm a test that verifies edge cases (e.g., store key doesn’t exist, invalid operator address).Do you want me to propose a test snippet for these edge cases?
285-290
: Check boundary behavior for negative index.
When staker is “not found,” you return immediately. That is consistent. Consider logging or at least clarifying the scenario in a comment if it’s a meaningful functional condition.
311-317
: Consider partial staker deletion vs. full wipe.
Deleting all stakers might be irreversible. If partial data is needed later, you may want a safer approach.x/operator/keeper/consensus_keys.go (2)
94-97
: Implement the TODO for key activation checks
These new TODO comments indicate a missing check for whether the previous key is actually active before recording it. Consider implementing a helper function likeConfirmKeyActivation(chainID, consAddr)
soon to avoid edge-case issues.Would you like assistance creating a utility that verifies key activation in x/dogfood or x/appchain?
298-307
: Hook vs. event order
The TODO questions whether to emit the event prior to hook completion. Consider deferring event emission until after the hook runs to avoid misleading logs if it fails.x/assets/types/asset_info.go (1)
10-12
: Consider documenting the empty string parameter.The empty string parameter in
GetStakerIDAndAssetIDFromStr
call could be confusing. Consider adding a comment explaining why it's empty or define a constant if it's a special case._, assetID := GetStakerIDAndAssetIDFromStr( - info.LayerZeroChainID, "", info.Address, + info.LayerZeroChainID, + "", // Unused in this context as we only need the assetID + info.Address, )x/dogfood/types/events.go (1)
15-20
: Consider adding epoch completion time attribute.For opt-out events, tracking when the epoch will complete could be valuable for indexers.
const ( EventTypeOptOutBegan = "opt_out_began" AttributeKeyEpoch = "epoch" AttributeKeyOperator = "operator" + AttributeKeyEpochEndTime = "epoch_end_time" EventTypeOptOutsFinished = "opt_outs_finished" )
x/assets/types/events.go (2)
13-24
: Consider adding validation status attribute for client chains.The client chain events could benefit from an attribute indicating the validation status of the chain configuration.
const ( EventTypeNewClientChain = "client_chain_added" EventTypeUpdatedClientChain = "client_chain_updated" AttributeKeyName = "name" AttributeKeyMetaInfo = "meta_info" AttributeKeyChainID = "chain_id" AttributeKeyExocoreChainIdx = "exocore_chain_index" AttributeKeyFinalizationBlocks = "finalization_blocks" AttributeKeyLZID = "layer_zero_chain_id" AttributeKeySigType = "signature_type" AttributeKeyAddrLength = "address_length" + AttributeKeyValidationStatus = "validation_status" )
41-44
: Add precision about slashing in comment.The comment about slashed quantity could be more precise about how it affects the total amount.
- // note that this amount includes lifetime slashed quantity of that token. + // note that this amount includes lifetime slashed quantity of that token, + // meaning it represents the total amount ever staked minus slashed amounts.x/epochs/client/cli/query.go (1)
112-145
: LGTM! Well-structured new CLI command.The new
epoch-info
command follows consistent patterns and includes proper error handling.Consider adding input validation for the identifier argument to ensure it matches expected epoch identifiers. Example enhancement:
func GetCmdEpochInfo() *cobra.Command { cmd := &cobra.Command{ Use: "epoch-info <identifier>", Short: "Query epoch info for the given identifier", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + // Validate identifier format + if !types.IsValidEpochIdentifier(args[0]) { + return fmt.Errorf("invalid epoch identifier format: %s", args[0]) + } clientCtx, err := client.GetClientQueryContext(cmd)x/dogfood/keeper/keeper.go (2)
113-118
: LGTM! Well-structured AVS event flag management.The implementation follows the established pattern for flag management in the keeper.
Consider adding a comment explaining when this flag should be set and what conditions trigger AVS event emission.
120-125
: LGTM! Consistent implementation of event flag checking and clearing.The methods properly handle the AVS event flag state with appropriate store operations.
Consider adding metrics or logging to track AVS event emission patterns, which could help with monitoring and debugging.
Also applies to: 127-132
x/delegation/keeper/update_native_restaking_balance.go (1)
51-57
: Consider refactoring complex slashing logic.While the error handling changes are correct, the slashing logic is spread across multiple sections with similar patterns.
Consider extracting common slashing logic into helper functions to improve maintainability:
+func (k Keeper) applySlashAndUpdateState( + ctx sdk.Context, + stakerID, assetID string, + slashAmount sdkmath.Int, + updateWithdrawable bool, +) error { + deltaAsset := types.DeltaStakerSingleAsset{ + TotalDepositAmount: slashAmount.Neg(), + } + if updateWithdrawable { + deltaAsset.WithdrawableAmount = slashAmount.Neg() + } + _, err := k.assetsKeeper.UpdateStakerAssetState( + ctx, stakerID, assetID, deltaAsset, + ) + return err +}This would simplify the main function and reduce code duplication.
Also applies to: 70-75, 113-118
x/operator/keeper/operator_slash_state.go (1)
51-59
: Consider expanding event attributes as noted in TODO.The event emission for operator slashing is well-implemented with basic attributes. However, as noted in the TODO comment, consider adding more attributes for comprehensive indexing.
Potential additional attributes to consider:
- Slash proportion
- Event height
- Submitted height
- Slash contract address
Would you like me to propose additional attributes for the operator slash event?
x/avs/keeper/multi_operator_submit_task_test.go (1)
56-56
: Consider capturing and logging the discarded return value.While error checking is maintained, the discarded return value from
PerformDepositOrWithdraw
might contain useful information for debugging test failures.-_, err := suite.App.AssetsKeeper.PerformDepositOrWithdraw(suite.Ctx, depositParam) +result, err := suite.App.AssetsKeeper.PerformDepositOrWithdraw(suite.Ctx, depositParam) +suite.T().Logf("Deposit result: %v", result)x/assets/keeper/staker_asset.go (1)
174-201
: Well-structured event emission for state tracking.The addition of detailed events with all relevant state information enables efficient indexing and state tracking. The comprehensive event attributes allow indexers to maintain accurate state without requiring historical data.
Consider adding a version attribute to the event to support future schema evolution:
+sdk.NewAttribute( + "version", "1", +),x/operator/keeper/slash.go (1)
149-150
: Track the TODO comment about checking pendingUndelegation.The TODO comment indicates a potential issue where an operator should be opted out if all assets fall to 0. This should be tracked and addressed to ensure proper operator state management.
Would you like me to create an issue to track this TODO item? I can help implement the check for pendingUndelegation and the operator opt-out logic.
x/operator/keeper/usd_value.go (1)
25-25
: Consider removing test-only code from production.The comment indicates this function is only used during tests. Consider moving test-specific code to test files to maintain a clean production codebase.
x/delegation/keeper/delegation_op_test.go (1)
46-46
: Consider explicit error handling.The error from
PerformDepositOrWithdraw
is being discarded. While the test might expect success, explicit error handling would make the test's expectations clearer.-_, err := suite.App.AssetsKeeper.PerformDepositOrWithdraw(suite.Ctx, depositEvent) +result, err := suite.App.AssetsKeeper.PerformDepositOrWithdraw(suite.Ctx, depositEvent) suite.NoError(err) +suite.NotNil(result)client/docs/swagger-ui/swagger.json (4)
76-181
: New Epoch Info Endpoint Definition
The addition of the/exocore/epochs/v1/epoch/{identifier}
endpoint is comprehensive. The detailed schema—including properties such asidentifier
,start_time
,duration
,current_epoch
,current_epoch_start_time
,epoch_counting_started
, andcurrent_epoch_start_height
—clearly documents the response for an epoch query.
- Suggestion: The description for
current_epoch_start_time
(lines 109–112) is very verbose. Consider summarizing the explanation or referencing external documentation to reduce duplication if possible.
17279-17427
: Addition of Validators List Endpoint with Pagination
The new/exocore/dogfood/v1/validators
endpoint is well defined. It returns an array of validator objects including apagination
object to help navigators of large result sets. The response schema’s inclusion of detailed properties (such as the lengthy description for thepubkey
field) ensures clarity.
- Suggestion: If the detailed descriptions for fields like
pubkey
repeat across endpoints, consider referencing a shared schema or definition to enhance maintainability.
24946-24994
: QueryEpochInfoResponse Schema Definition
The newly defined"exocore.epochs.v1.QueryEpochInfoResponse"
is detailed and mirrors the endpoint’s response structure. The replication of detailed descriptions (e.g., forcurrent_epoch_start_time
) improves clarity for API consumers but might be optimized by modularizing common field definitions in the future.
40643-40699
: QueryAllValidatorsResponse Schema Addition
The response type"exocore.dogfood.v1.QueryAllValidatorsResponse"
is comprehensive. It includes an array of validators (with detailed properties foraddress
,power
, andpubkey
) alongside a pagination object.
- Suggestion: Consider abstracting the common
pubkey
field description into a shared component if similar details are repeated across multiple response types.scripts/protoc-swagger-gen.sh (1)
38-39
: Enhanced Logging Message:
The newly added echo statement (echo "Combining swagger files"
) clearly indicates when the swagger combination process starts. This improves traceability during script execution. Consider including a timestamp (or additional context) for even better traceability in complex environments.proto/exocore/delegation/v1/query.proto (1)
173-182
: New MessageQueryDelegatedAmountRequest
:
This message captures the necessary fields (staker_id
,asset_id
, andoperator
) to query the delegated amount. One minor note: the field namedoperator
might benefit from further clarification (e.g., renaming tooperator_addr
) if there’s any risk of ambiguity or conflict with reserved keywords.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
x/delegation/types/query.pb.go
is excluded by!**/*.pb.go
x/delegation/types/query.pb.gw.go
is excluded by!**/*.pb.gw.go
x/dogfood/types/query.pb.go
is excluded by!**/*.pb.go
x/dogfood/types/query.pb.gw.go
is excluded by!**/*.pb.gw.go
x/epochs/types/query.pb.go
is excluded by!**/*.pb.go
x/epochs/types/query.pb.gw.go
is excluded by!**/*.pb.gw.go
x/operator/types/tx.pb.go
is excluded by!**/*.pb.go
📒 Files selected for processing (85)
app/ethtest_helper.go
(1 hunks)app/test_helpers.go
(1 hunks)client/docs/config.json
(1 hunks)client/docs/swagger-ui/swagger.json
(8 hunks)cmd/exocored/testnet.go
(1 hunks)local_node.sh
(1 hunks)precompiles/assets/assets_test.go
(2 hunks)precompiles/assets/tx.go
(3 hunks)precompiles/avs/avs_test.go
(2 hunks)precompiles/avs/utils_test.go
(2 hunks)precompiles/delegation/delegation_test.go
(3 hunks)precompiles/reward/reward_test.go
(1 hunks)precompiles/slash/slash_test.go
(1 hunks)proto/exocore/delegation/v1/query.proto
(2 hunks)proto/exocore/dogfood/v1/query.proto
(3 hunks)proto/exocore/epochs/v1/query.proto
(2 hunks)proto/exocore/operator/v1/tx.proto
(1 hunks)scripts/protoc-swagger-gen.sh
(1 hunks)testutil/network/util.go
(1 hunks)testutil/utils.go
(2 hunks)x/assets/keeper/bank.go
(1 hunks)x/assets/keeper/client_chain.go
(2 hunks)x/assets/keeper/client_chain_asset.go
(4 hunks)x/assets/keeper/genesis.go
(2 hunks)x/assets/keeper/keeper.go
(3 hunks)x/assets/keeper/operator_asset.go
(2 hunks)x/assets/keeper/staker_asset.go
(2 hunks)x/assets/keeper/staker_asset_test.go
(7 hunks)x/assets/types/asset_info.go
(1 hunks)x/assets/types/events.go
(1 hunks)x/assets/types/general.go
(1 hunks)x/assets/types/genesis.go
(1 hunks)x/avs/keeper/avs.go
(3 hunks)x/avs/keeper/avs_test.go
(1 hunks)x/avs/keeper/keeper.go
(1 hunks)x/avs/keeper/multi_operator_submit_task_test.go
(2 hunks)x/avs/keeper/submit_task_test.go
(2 hunks)x/avs/types/events.go
(1 hunks)x/delegation/client/cli/query.go
(2 hunks)x/delegation/keeper/abci.go
(1 hunks)x/delegation/keeper/delegation.go
(4 hunks)x/delegation/keeper/delegation_op_test.go
(4 hunks)x/delegation/keeper/delegation_state.go
(12 hunks)x/delegation/keeper/grpc_query.go
(1 hunks)x/delegation/keeper/share.go
(1 hunks)x/delegation/keeper/un_delegation_state.go
(3 hunks)x/delegation/keeper/update_native_restaking_balance.go
(4 hunks)x/delegation/module.go
(1 hunks)x/delegation/types/events.go
(1 hunks)x/delegation/types/expected_keepers.go
(1 hunks)x/delegation/types/keys.go
(1 hunks)x/dogfood/client/cli/query.go
(3 hunks)x/dogfood/keeper/abci.go
(1 hunks)x/dogfood/keeper/genesis.go
(2 hunks)x/dogfood/keeper/impl_epochs_hooks_test.go
(4 hunks)x/dogfood/keeper/keeper.go
(1 hunks)x/dogfood/keeper/msg_server.go
(3 hunks)x/dogfood/keeper/opt_out.go
(5 hunks)x/dogfood/keeper/opt_out_test.go
(6 hunks)x/dogfood/keeper/query.go
(6 hunks)x/dogfood/keeper/unbonding.go
(3 hunks)x/dogfood/keeper/unbonding_test.go
(4 hunks)x/dogfood/keeper/validators.go
(1 hunks)x/dogfood/module.go
(1 hunks)x/dogfood/types/events.go
(1 hunks)x/dogfood/types/expected_keepers.go
(1 hunks)x/dogfood/types/keys.go
(2 hunks)x/epochs/client/cli/query.go
(2 hunks)x/epochs/keeper/grpc_query.go
(1 hunks)x/exomint/keeper/impl_epochs_hooks.go
(1 hunks)x/exomint/types/events.go
(1 hunks)x/feedistribution/keeper/hooks_test.go
(3 hunks)x/operator/client/cli/tx.go
(1 hunks)x/operator/keeper/consensus_keys.go
(8 hunks)x/operator/keeper/genesis.go
(1 hunks)x/operator/keeper/operator.go
(4 hunks)x/operator/keeper/operator_info_test.go
(2 hunks)x/operator/keeper/operator_slash_state.go
(1 hunks)x/operator/keeper/opt.go
(2 hunks)x/operator/keeper/opt_test.go
(2 hunks)x/operator/keeper/slash.go
(5 hunks)x/operator/keeper/usd_value.go
(10 hunks)x/operator/types/events.go
(1 hunks)x/operator/types/tx.go
(1 hunks)x/operator/types/validator.go
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/exomint/types/events.go
🧰 Additional context used
🧠 Learnings (1)
x/avs/keeper/avs.go (1)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#149
File: x/operator/keeper/usd_value.go:228-228
Timestamp: 2024-11-12T10:03:10.791Z
Learning: The `GetAVSAddrByChainID` function in `x/avs/keeper/avs.go` does not return an error.
🪛 Buf (1.47.2)
proto/exocore/dogfood/v1/query.proto
5-5: import "cosmos/base/query/v1beta1/pagination.proto": file does not exist
(COMPILE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: goreleaser
🔇 Additional comments (139)
x/assets/keeper/bank.go (3)
34-34
: Validate consistent usage ofIsStakingAsset
.
Returning an error on unrecognized staking assets is appropriate. Confirm that any future expansions (e.g., allowing new asset IDs) also keep this logic consistent.
66-72
: Validate updated staker asset state thoroughly.
The call toUpdateStakerAssetState
can fail if the staker or asset doesn't exist. The error is properly handled here, returning ZeroInt. Ensure any usage of the returned deposit amount is robustly tested in your integration tests.
85-86
: Great job returning final deposit amount.
Returninginfo.TotalDepositAmount
is helpful to consumers. Ensure calling code uses it consistently.x/assets/keeper/client_chain.go (4)
14-14
: Signature expansion is consistent with event-based updates.
Returning(bool, error)
clarifies whether the chain was updated or newly created. Nicely done.
16-16
: Consider fallback for repeated calls.
The logic determines if the store key already exists, then changes the event type. Ensure that repeated calls with the same info do not cause spurious “updated” events if content is unchanged.Also applies to: 18-22
27-39
: Great usage of event emission.
Emitting a chain creation/update event with well-labeled attributes is beneficial for external watchers or indexers. Ideally, confirm integration tests capture these events.
41-41
: Return value approach is consistent.
Returningupdated
clarifies creation vs. modification. Good design choice.x/delegation/keeper/delegation_state.go (4)
5-5
: Check slices usage for version compatibility.
slices
is from Go 1.21. Ensure your build environment is pinned or updated to a version that supports the standard libraryslices
package.
208-217
: Event emission for delegation updates is robust.
This event helps trace changes at a granular level. Make sure any external indexers or watchers know about this new event.
257-261
: Use slices.Contains to keep code consistent.
This approach is simpler and safer than local iteration or manual searching. Good improvement.
438-444
: Ensure operator address validity before storing.
This is done well. The method emits an event associating staker with operator. Affirm in tests that re-associating a staker overwrites the old operator if repeated.x/operator/keeper/consensus_keys.go (9)
17-17
: Imported go-ethereum/common
No issues. The library is used properly for consensus address manipulations and hex conversions.
90-90
: Removed code comment clarifies key replacement timing
This comment better explains why the old key is zeroed out only once per epoch. No functional changes required.
103-103
: Invoking setOperatorConsKeyForChainIDUnchecked
Calling this internal function after preparing the key looks consistent. No issues found.
121-122
: Refactor: converting from raw bytes to WrappedConsKey
Adopting a structuredwrappedKey
parameter improves readability and type safety, while theMustMarshal
usage is appropriate.Also applies to: 125-125
144-148
: Registering chainID → consensus address → operator mapping
This data is essential for slashing and operator lookups. The approach is correct.
150-163
: Emitting SetConsKey event
Publishing the newEventTypeSetConsKey
event expands observability. Confirm the key’s public exposure is acceptable security-wise.
179-187
: Event for previous consensus key
Storing and emitting the old key underEventTypeSetPrevConsKey
aids in tracking validator set changes.
346-354
: Finalizing operator key removal
EmittingEventTypeEndRemoveConsKey
maintains consistency with the other lifecycle events.
509-518
: Verify slice indexing for operator address
The substringkey[1+len(partialKey):]
may be fragile ifpartialKey
changes in length. Ensure prefix sizing remains consistent to avoid off-by-one errors.x/operator/types/tx.go (1)
1-13
: New DecValueField type
Implementingcodec.ProtoMarshaler
and providing aString
method forDecValueField
is straightforward. No issues found.x/avs/types/events.go (1)
1-13
: AVS events and attribute keys
Defining events for AVS creation and chain-specific AVS creation is clear and aligns with the event-based architecture.x/dogfood/types/events.go (2)
5-9
: LGTM! Well-structured AVS event definition.The AVS creation event and its attributes are well-defined with clear documentation explaining the genesis context.
10-14
: LGTM! Clear power update event definition.The total power update event is well-documented and includes the necessary attribute for tracking validator set changes.
x/assets/types/events.go (1)
6-12
: LGTM! Comprehensive staker asset event attributes.The staker asset update event includes all necessary attributes for tracking deposit, withdrawal, and undelegation amounts.
x/delegation/types/events.go (1)
1-43
: Well-structured event definitions for comprehensive delegation tracking!The event types and attribute keys are logically organized and provide comprehensive coverage of the delegation lifecycle, including:
- Asset delegation events
- Delegation state updates
- Staker management (append/remove)
- Operator association
- Undelegation process
- Maturity and hold count tracking
This structure will effectively support the indexing requirements for the Callisto fork.
x/operator/types/events.go (2)
1-55
: Well-structured event definitions for comprehensive operator tracking!The event types and attribute keys provide thorough coverage of the operator lifecycle:
- Registration and opt-in/out
- Consensus key management
- USD value tracking
- Slashing events
51-54
: Consider tracking combined slashing effects in the indexer.The comment indicates that combined slashing effects are not yet tracked in the indexer. This might limit the ability to analyze the overall impact of slashing events.
Would you like me to help create an issue to track this enhancement for the indexer?
x/epochs/keeper/grpc_query.go (1)
75-95
: Well-implemented epoch info query method!The new
EpochInfo
method:
- Follows consistent error handling patterns
- Provides comprehensive epoch information
- Includes block time in response
x/exomint/keeper/impl_epochs_hooks.go (1)
72-72
: Enhanced minting event with denomination tracking!The addition of the denomination attribute provides complete information about minted coins, improving the ability to track and index minting operations.
x/delegation/types/expected_keepers.go (1)
44-47
: LGTM! Enhanced return type aligns with PR objectives.The updated
UpdateStakerAssetState
method now returns additional state information, which supports better tracking of state changes for indexers.x/assets/keeper/keeper.go (1)
73-75
: LGTM! Consistent interface changes across modules.The updated method signature matches the changes in the delegation module, maintaining consistency across the codebase.
x/operator/keeper/genesis.go (1)
27-29
: LGTM! Optimized consensus key setting.The change simplifies the code by directly passing the wrapped key object, removing unnecessary marshaling step.
x/dogfood/keeper/msg_server.go (3)
54-55
: LGTM! Clear documentation of parameter behavior.The comment effectively clarifies that changes to
EpochsUntilUnbonded
won't affect existing undelegations, which is important for understanding the system's behavior.
64-71
: LGTM! Robust parameter update handling.The code now explicitly forbids changes to the epoch identifier by overriding any attempted changes with the previous value. This is a more robust approach than validation, as it ensures the epoch identifier remains constant.
94-94
: LGTM! Clear documentation of validation logic.The comment clearly explains why epoch identifier validation is no longer needed, which helps maintain code clarity.
x/assets/keeper/genesis.go (2)
20-25
: LGTM! Enhanced genesis validation.The code now properly handles duplicate client chains during genesis initialization by checking the
updated
boolean. This prevents invalid genesis states from being initialized.
46-51
: LGTM! Improved clarity and error handling.The changes:
- Clearly document that
UpdateStakerAssetState
won't emit an event.- Properly handle the return values by using the blank identifier for unused returns.
x/delegation/keeper/grpc_query.go (1)
101-110
: LGTM! Well-implemented query method.The new
QueryDelegatedAmount
method follows best practices:
- Consistent error handling
- Proper context unwrapping
- Clean and concise implementation
x/delegation/keeper/abci.go (1)
86-94
: LGTM! Consistent error handling.The change properly handles the unused return value from
UpdateStakerAssetState
while maintaining the existing error handling logic.x/dogfood/types/expected_keepers.go (1)
101-106
:❓ Verification inconclusive
Verify implementations of the updated AVSKeeper interface.
The
RegisterAVSWithChainID
method signature has been updated to return an additional boolean value. This is a breaking change that requires updates in all implementations of theAVSKeeper
interface.Run this script to find all implementations that need updating:
🏁 Script executed:
#!/bin/bash # Description: Find all implementations of AVSKeeper interface that need updating # Search for struct types implementing AVSKeeper ast-grep --pattern 'type $_ struct { $$$ } func ($_ $_) RegisterAVSWithChainID($_, $_) (common.Address, error) {'Length of output: 118
Manual Verification Required: Confirm All Implementations Reflect the Updated Signature
The
RegisterAVSWithChainID
signature now returns a boolean along with(common.Address, error)
, marking a breaking change. Although an automated search was attempted to locate implementations, it returned no results. Please manually verify that all concrete types implementing theAVSKeeper
interface have been updated accordingly.
- Action Item: Manually search for usages of
RegisterAVSWithChainID
(for instance, usingrg "RegisterAVSWithChainID" -n .
) to confirm that every implementation now aligns with the new signature.x/dogfood/keeper/query.go (2)
16-24
: LGTM! Good separation of concerns.The introduction of a dedicated
Querier
struct that embedsKeeper
follows the standard Cosmos SDK pattern for query handling. This improves code organization by separating query-related functionality from the core keeper logic.
117-146
: LGTM! Well-implemented pagination support.The new
Validators
query method correctly implements pagination usingGenericFilteredPaginate
. The implementation includes proper error handling and type conversion.x/delegation/module.go (1)
104-105
: LGTM! Empty BeginBlock implementation.The empty
BeginBlock
implementation is appropriate as no specific logic is required at the beginning of each block for this module.x/dogfood/keeper/unbonding.go (2)
38-44
: LGTM! Good event emission for undelegation scheduling.The event emission for undelegation maturity scheduling includes appropriate attributes (epoch and record ID) and uses proper type conversion for the epoch value.
74-79
: LGTM! Good event emission for undelegation maturity.The event emission for undelegation maturity includes the epoch attribute and uses proper type conversion.
x/assets/keeper/staker_asset_test.go (1)
20-20
: LGTM! Consistent error handling updates.The changes correctly handle the updated method signature of
UpdateStakerAssetState
by discarding the first return value while maintaining proper error checking.Also applies to: 32-32, 43-43, 53-53, 62-62, 75-75, 107-107, 109-109
precompiles/slash/slash_test.go (1)
69-70
: LGTM! Proper error handling update.The change correctly handles the updated method signature while maintaining proper error checking in the test case.
x/delegation/keeper/update_native_restaking_balance.go (1)
25-31
: LGTM! Proper handling of positive balance updates.The change correctly handles the updated method signature while maintaining proper error checking.
x/dogfood/module.go (1)
132-132
: LGTM! Improved query server initialization.The change properly encapsulates the query server initialization by using
NewQueryServer
wrapper.precompiles/reward/reward_test.go (1)
69-69
: LGTM! Properly handling new return value.The change correctly handles the new tuple return value from
PerformDepositOrWithdraw
by discarding the unused deposit amount since only the error is relevant for the test.precompiles/assets/tx.go (2)
52-52
: LGTM! Improved return value handling in DepositOrWithdraw.The change simplifies the logic by directly using the final deposit amount from
PerformDepositOrWithdraw
instead of retrieving it separately from the state.Also applies to: 76-76
95-95
: LGTM! Simplified client chain registration logic.The change streamlines the control flow by directly using the updated flag from
SetClientChainInfo
.x/dogfood/keeper/genesis.go (3)
35-36
: LGTM! Added proper AVS existence tracking.The change adds proper tracking of whether an AVS already exists during initialization.
Also applies to: 37-38
56-62
: LGTM! Added event emission for new AVS registrations.The change ensures that new AVS registrations are properly tracked by deferring event emission to BeginBlock, as InitGenesis events are discarded. This is particularly important for indexer functionality.
53-53
: LGTM! Improved logging.The change simplifies logging by directly using the String() method instead of a separate variable.
x/dogfood/client/cli/query.go (2)
190-219
: LGTM! Improved address validation.The changes enhance input validation by properly converting and validating the consensus address format using
ConsAddressFromBech32
.
221-249
: LGTM! Well-implemented pagination support.The new command follows best practices by:
- Implementing pagination support using
client.ReadPageRequest
- Following consistent error handling patterns
- Maintaining consistency with other query commands
x/assets/keeper/client_chain_asset.go (3)
37-44
: LGTM! Well-structured event emission.The event emission is correctly placed after the state update and includes all necessary attributes for tracking total amount changes.
70-83
: LGTM! Comprehensive event attributes.The event emission includes all essential token information, making it valuable for indexers tracking new token registrations.
104-110
: LGTM! Appropriate event emission.The event emission correctly tracks meta information updates with relevant attributes.
x/dogfood/types/keys.go (2)
79-80
: LGTM! Well-documented constant.The constant follows the established pattern of using single-byte keys for store identification.
205-208
: LGTM! Consistent implementation.The function maintains consistency with other key generation functions in the file.
x/operator/keeper/opt.go (1)
96-105
: LGTM! Well-documented event emission.The event emission:
- Is correctly placed after all state updates
- Includes comprehensive attributes for tracking opt-in actions
- Has a helpful comment explaining why opt-out height is not emitted
x/assets/types/general.go (1)
60-82
: LGTM! Well-structured String() method implementation.The implementation provides clear string representations for all CrossChainOpType values and includes a default case for unknown values, which is good for error handling and debugging.
x/assets/keeper/operator_asset.go (2)
120-130
: LGTM! Well-structured event emission for operator asset updates.The event includes all relevant attributes for tracking operator asset state changes, which aligns with the PR's objective of enhancing state change tracking.
163-173
: LGTM! Consistent event emission in iteration updates.The event emission matches the implementation in UpdateOperatorAssetState, ensuring consistent tracking of state changes across different update paths.
x/delegation/types/keys.go (1)
122-125
: LGTM! Clean implementation of GetKey() method.The method provides a convenient way to get the key for an undelegation record while reusing existing functionality.
x/operator/keeper/operator_info_test.go (2)
19-29
: LGTM! Test case properly validates the breaking change requirement.The test correctly sets ApproveAddr equal to EarningsAddr (both set to suite.AccAddress.String()), which aligns with the PR's breaking change requiring these addresses to be equal.
41-49
: LGTM! Consistent validation in AllOperators test.The test maintains consistency with the breaking change by setting ApproveAddr equal to EarningsAddr and OperatorAddress.
x/operator/client/cli/tx.go (2)
101-103
: LGTM! Good validation for required flag.The validation check ensures that
metaInfo
is not empty after trimming whitespace, with a clear error message.
107-108
: LGTM! Enforcing breaking change for operator addresses.The implementation correctly enforces that
EarningsAddr
andApproveAddr
must be equal to the sender's address, aligning with the PR objectives.x/operator/keeper/opt_test.go (2)
35-35
: LGTM! Test updated for breaking change.The test correctly sets
ApproveAddr
to the operator's address, aligning with the breaking change.
64-64
: LGTM! Simplified error handling.The change correctly maintains error handling while discarding the unused return value.
x/dogfood/keeper/abci.go (2)
8-8
: LGTM! Required import for event types.The import is necessary for using event types and attributes from the dogfood module.
16-29
: LGTM! Event emission for AVS creation.The implementation correctly emits events for AVS creation with appropriate attributes, aligning with the PR objectives to enhance event tracking for indexers.
x/avs/keeper/avs_test.go (1)
170-170
: LGTM! Test updated for breaking change.The test correctly sets
ApproveAddr
to the operator's address, aligning with the breaking change in operator registration.x/dogfood/keeper/opt_out.go (4)
31-37
: LGTM! Event emission for opt-out tracking.The event emission for tracking operator opt-outs is well-implemented, including relevant attributes for epoch and operator address.
79-84
: LGTM! Event emission for opt-outs completion.The event emission for tracking completed opt-outs is well-implemented, including the epoch attribute.
159-165
: LGTM! Event emission for consensus address pruning schedule.The event emission for tracking scheduled consensus address pruning is well-implemented, including relevant attributes for epoch and consensus address.
192-197
: LGTM! Event emission for consensus addresses pruning completion.The event emission for tracking completed consensus address pruning is well-implemented, including the epoch attribute.
precompiles/avs/utils_test.go (2)
41-42
: LGTM! Addition of ApproveAddr field.The ApproveAddr field is correctly set to match the operator's address, aligning with the PR's breaking change requirement that FromAddress, ApproveAddress, and EarningsAddress must be equal.
65-65
: LGTM! Simplified error handling.The error handling for PerformDepositOrWithdraw has been streamlined to only capture the error, which is sufficient for test purposes.
x/avs/keeper/submit_task_test.go (2)
32-32
: LGTM! Addition of ApproveAddr field.The ApproveAddr field is correctly set to match the operator's address, aligning with the PR's breaking change requirement that FromAddress, ApproveAddress, and EarningsAddress must be equal.
56-56
: LGTM! Simplified error handling.The error handling for PerformDepositOrWithdraw has been streamlined to only capture the error, which is sufficient for test purposes.
x/avs/keeper/multi_operator_submit_task_test.go (1)
31-31
: LGTM! Addition of ApproveAddr field aligns with new requirements.The addition of
ApproveAddr
field set to the same value asEarningsAddr
andFromAddress
aligns with the breaking change requirement that all three addresses must be equal.x/feedistribution/keeper/hooks_test.go (1)
59-59
: LGTM! Consistent implementation of ApproveAddr requirement.The addition of
ApproveAddr
field with the same value asFromAddress
maintains consistency with the new operator registration requirements.app/ethtest_helper.go (1)
207-207
: LGTM! Proper genesis state setup for operator approval.The addition of
ApproveAddr
in the genesis state correctly initializes operators with the required address equality constraint.x/assets/keeper/staker_asset.go (2)
141-143
: LGTM! Enhanced method signature with improved return values.The updated signature returning
(*StakerAssetInfo, error)
provides better access to the updated state, which is useful for callers needing the new state information.
159-168
: Consistent error handling with wrapped errors.The error handling now consistently returns nil for info along with wrapped errors, providing better context for debugging.
x/assets/types/genesis.go (1)
89-94
: Great improvement to documentation!The updated comment provides clearer context about the validation process and its limitations. It effectively explains that while ideally the quantity should sum each staker's deposit, this validation isn't possible within this module alone.
x/avs/keeper/avs.go (3)
68-76
: Well-documented function with clear explanation of the naming!The comment effectively clarifies that "duration" in the function name refers to the number of epochs rather than a time duration, preventing potential confusion.
147-148
: Improved error handling with existence check.The function signature change to include an
exists
boolean return value enhances the API by providing more information about the registration state. The deferred function correctly uses this flag to determine when to update the state.Also applies to: 164-165
199-206
: Good addition of event emission for better observability.The addition of event emission when storing AVS address to chain ID mapping will help with tracking and debugging AVS creation.
app/test_helpers.go (1)
249-249
: Correctly implements the breaking change requirement.Setting
ApproveAddr
tooperator.String()
aligns with the PR's breaking change requirement thatFromAddress
,ApproveAddress
, andEarningsAddress
must be equal for operator acceptance.x/dogfood/keeper/unbonding_test.go (1)
24-24
: LGTM!The addition of
ApproveAddr
field aligns with the PR's breaking change to enforce equal addresses for operators. The error handling improvement by using blank identifier follows Go best practices.Also applies to: 161-161
x/operator/keeper/slash.go (1)
99-108
: LGTM!The addition of event emission for undelegation slashing enhances the observability of the system by providing detailed information about the slashing operation, including record ID, remaining amount, and slashed quantity.
x/dogfood/keeper/validators.go (1)
325-330
: LGTM!The addition of event emission for last total power updates enhances the observability of validator power changes in the system.
x/delegation/keeper/delegation.go (3)
54-60
: LGTM! Improved error handling.The code now correctly ignores the first return value from
UpdateStakerAssetState
since it's not needed, while still properly handling any errors.
81-91
: LGTM! Added event emission for tracking ExoAsset delegations.The event emission includes essential attributes for tracking delegation state changes:
- StakerID
- Operator
- Amount
This aligns with the PR objective of enhancing event tracking for indexers.
196-216
: LGTM! Added comprehensive event emission for tracking undelegation records.The event emission includes all necessary attributes for tracking undelegation state:
- StakerID, AssetID, Operator
- Record identifier
- Amount and completion details
- Transaction metadata
This aligns with the PR objective of enhancing event tracking for indexers.
x/dogfood/keeper/impl_epochs_hooks_test.go (3)
26-28
: LGTM! Added ApproveAddr field in operator registration test.The test now correctly sets up the operator with the required ApproveAddr field, aligning with the breaking change mentioned in the PR objectives.
54-54
: LGTM! Improved error handling.The code now correctly captures the return value from
PerformDepositOrWithdraw
while maintaining error checking.
207-207
: LGTM! Added ApproveAddr field in different epoch test.Consistently applying the ApproveAddr field requirement across different test scenarios.
x/dogfood/keeper/opt_out_test.go (2)
26-27
: LGTM! Added ApproveAddr field in operator registration test.The test now correctly sets up the operator with the required ApproveAddr field, aligning with the breaking change mentioned in the PR objectives.
68-69
: LGTM! Consistent error handling improvements.The code now consistently handles return values from
PerformDepositOrWithdraw
across multiple test cases while maintaining proper error checking.Also applies to: 98-99, 172-173, 207-208
x/operator/keeper/operator.go (4)
32-34
: LGTM! Added defensive nil check.Added validation to prevent nil pointer dereference, improving code robustness.
41-46
: LGTM! Added address validation for operator registration.The code now enforces that the operator's address matches both EarningsAddr and ApproveAddr, implementing the breaking change mentioned in the PR objectives.
79-92
: LGTM! Added comprehensive event emission for operator registration.The event emission includes essential attributes for tracking operator registration:
- Operator address
- Metadata
- Commission rates and update time
This aligns with the PR objective of enhancing event tracking for indexers.
156-166
: LGTM! Added event emission for opt info updates.The event emission includes all necessary attributes for tracking operator opt status:
- Operator and AVS addresses
- Slash contract
- Opt-in/out heights
- Jailed status
This aligns with the PR objective of enhancing event tracking for indexers.
precompiles/delegation/delegation_test.go (2)
92-92
: LGTM! Added ApproveAddr field to align with new requirements.The addition of
ApproveAddr
field set toopAccAddr
aligns with the PR's breaking change that enforcesFromAddress
,ApproveAddress
, andEarningsAddress
to be equal.
84-84
:❓ Verification inconclusive
Verify if the discarded return value is needed for test validation.
The return value from
PerformDepositOrWithdraw
is being discarded. Consider if this value needs to be validated in the test case.
🏁 Script executed:
#!/bin/bash # Description: Check if the return value is used in other test files. # Search for PerformDepositOrWithdraw usage in test files rg -A 5 'PerformDepositOrWithdraw.*:=' 'test_*.go' 'precompiles/*_test.go'Length of output: 188
Action: Verify the necessity of validating the return value in the test
The automated search using the initial file patterns didn’t produce any output (due to directory matching issues), so it’s unclear if or where the return value might be used elsewhere in tests. Please verify manually that the first return value returned by
PerformDepositOrWithdraw
isn’t essential for test validation—either by confirming that no assertions are based on it or by checking if its value carries meaningful information from the function’s implementation.
- Verify the function’s definition in the AssetsKeeper to determine what the discarded value represents.
- Ensure that the test case doesn’t miss an opportunity to catch unexpected conditions by overlooking this return value.
- Re-run the search against all Go test files (e.g., using a command like
find . -type f -name "*_test.go" -exec grep -Hn "PerformDepositOrWithdraw(" {} \;
) to confirm there’s no existing use of the return value that would suggest its importance.x/delegation/keeper/un_delegation_state.go (1)
103-114
: LGTM! Enhanced event tracking for undelegation state changes.The added events improve the ability to track:
- Undelegation maturity with completed amounts
- Hold count changes with record IDs
- Hold count decrements with record IDs
This aligns with the PR's objective to enhance event tracking for state changes.
Also applies to: 357-363, 383-389
testutil/network/util.go (1)
368-368
: LGTM! Added ApproveAddr field to test utilities.The addition of
ApproveAddr
field toOperatorInfo
in test utilities aligns with the PR's breaking change for operator address validation.testutil/utils.go (1)
263-263
: LGTM! Added ApproveAddr fields to test setup.The addition of
ApproveAddr
fields for both operators in the test setup aligns with the PR's breaking change that enforces address equality.Also applies to: 272-272
cmd/exocored/testnet.go (1)
424-424
: LGTM! TheApproveAddr
field is correctly initialized.The addition of
ApproveAddr
field aligns with the PR's breaking change to enforce equal addresses forFromAddress
,ApproveAddress
, andEarningsAddress
in the x/operator module.x/operator/keeper/usd_value.go (1)
58-68
: LGTM! Consistent event emission pattern.The added event emissions follow a consistent pattern across all USD value operations, which will help with tracking and monitoring state changes. The events include relevant attributes such as operator addresses, AVS addresses, and USD values.
Also applies to: 106-112, 127-133, 195-201, 215-221, 229-234, 381-387
x/avs/keeper/keeper.go (1)
137-149
: LGTM! Improved error handling and event emission.The changes enhance error handling for AVS info setting and add event emission for AVS creation, which aligns with the PR's objective to track state changes effectively.
x/delegation/keeper/delegation_op_test.go (1)
65-65
: LGTM! Consistent operator registration with approval address.The
ApproveAddr
field is consistently set in operator registration across all test cases, aligning with the PR's breaking change to enforce equal addresses.Also applies to: 141-141, 231-231
precompiles/avs/avs_test.go (2)
512-512
: LGTM! TheApproveAddr
field is correctly set to match the operator's address.The test correctly implements the breaking change mentioned in the PR objectives, where the operator's
ApproveAddr
must match theEarningsAddr
andFromAddress
.
678-678
: LGTM! TheApproveAddr
field is correctly set to match the operator's address.The test correctly implements the breaking change mentioned in the PR objectives, where the operator's
ApproveAddr
must match theEarningsAddr
andFromAddress
.precompiles/assets/assets_test.go (2)
313-313
: LGTM! Correctly capturing the additional return value fromPerformDepositOrWithdraw
.The test has been updated to handle the new method signature that returns additional data.
912-912
: LGTM! Correctly capturing the additional return value fromUpdateStakerAssetState
.The test has been updated to handle the new method signature that returns additional data.
client/docs/swagger-ui/swagger.json (4)
17183-17228
: Validator Query Endpoint Refinement
The modifications to the/exocore/dogfood/v1/validator/{cons_addr}
endpoint, which now documents a trimmed response with only the minimal validator fields (public key, consensus address, power), are clearly specified. The detailed description for thepubkey
property is consistent with standard protobuf Any representations.
- Note: Ensure that downstream clients expecting additional validator details (if any) are updated accordingly.
20826-20905
: Delegated Amount Query Endpoint
The endpoint/exocore/delegation/v1/delegated_amount/{staker_id}/{asset_id}/{operator}
is clearly specified. The schema is straightforward with anamount
field of type string—which is a common pattern in blockchain systems to avoid precision or overflow issues.
- Verify: Confirm that clients and business logic correctly handle the amount as a string even when numeric operations are required.
40764-40800
: QueryValidatorResponse Schema Definition
The new"exocore.dogfood.v1.QueryValidatorResponse"
schema is well structured, providing a nestedvalidator
object with the expected minimal properties. Its detailed documentation mirrors other similar sections in the API, ensuring consistency.
43934-43944
: QueryDelegatedAmountResponse Schema Addition
The response type"exocore.delegation.v1.QueryDelegatedAmountResponse"
is correctly defined with anamount
property of type string and an appropriate title. This aligns with the conventions used in other endpoints for handling numeric amounts.proto/exocore/epochs/v1/query.proto (1)
23-26
: New RPC MethodEpochInfo
:
The addition of theEpochInfo
RPC method expands the Query service by enabling clients to retrieve detailed information about a specific epoch using an identifier. Ensure that all client integrations and downstream services that require epoch details are updated to invoke this new method.proto/exocore/dogfood/v1/query.proto (5)
46-51
: NewValidator
RPC Method:
The newValidator
method now returns aQueryValidatorResponse
containing the essential validator details (e.g., public key, consensus address, power). This refined response structure simplifies the API for clients needing only the bare-minimum information. Be sure to update any consumers to handle the new response type.
53-58
: NewValidators
RPC Method:
The addition of theValidators
method enhances the API by allowing bulk retrieval of validator records with pagination support. This change is aligned with modern API practices to improve scalability.
118-123
: Definition ofQueryValidatorResponse
:
The new message typeQueryValidatorResponse
wraps theExocoreValidator
details for single validator queries. The design is concise and fits well with the updated RPC method.
124-128
: Definition ofQueryAllValidatorsRequest
:
This request message introduces an optional pagination mechanism for querying validators. It will help in efficiently managing large datasets when retrieving multiple validators.
130-136
: Definition ofQueryAllValidatorsResponse
:
The response message correctly provides a repeated list of validators along with pagination information. The use of thegogoproto.nullable
annotation ensures proper handling of empty lists.client/docs/config.json (1)
145-147
: Updated Operation ID Mappings:
The revised mappings—renamingValidator
toDogfoodValidator
andValidators
toDogfoodValidators
(while retainingDogfoodParams
)—improve clarity for consumers of the Dogfood API endpoint. Verify that these new identifiers are consistently referenced in both the API documentation and client-side implementations.proto/exocore/delegation/v1/query.proto (2)
184-192
: New MessageQueryDelegatedAmountResponse
:
The response message appropriately encapsulates the delegated amount as a string with the necessary type annotations for high-precision values (cosmos.Int
). This design choice should preserve numerical accuracy.
245-250
: New RPC MethodQueryDelegatedAmount
:
This RPC method effectively extends the delegation module by allowing queries based on the combination of staker ID, asset ID, and operator. Ensure that the defined HTTP GET endpoint (/exocore/delegation/v1/delegated_amount/{staker_id}/{asset_id}/{operator}
) is secured and that all dependent modules are updated to integrate this new functionality.proto/exocore/operator/v1/tx.proto (2)
15-17
: Disable Default Stringer for DecValueFieldThe added option
(gogoproto.goproto_stringer) = false;
clearly disables the default stringer. Ensure you have a custom implementation (likely viaamount.String()
) that meets your output precision and formatting requirements.
125-133
: Add New 'approve_addr' Field in OperatorInfoThe new field
approve_addr
is introduced in theOperatorInfo
message, aligning with the updated requirement that an operator’sFromAddress
,EarningsAddr
, andApproveAddr
must be identical. Please verify that the downstream validation logic enforces this invariant consistently across the operator registration and update flows.local_node.sh (1)
137-139
: Set 'approve_addr' in Genesis ConfigurationThe new jq command on line 138 sets the
approve_addr
field to the same value as the operator’s address ($LOCAL_ADDRESS_EXO
). This is in line with the breaking change that requires the operator’s addresses (from, earnings, and approve) to be consistent. Ensure that other genesis configuration updates (and corresponding validations later in the runtime) adhere to this invariant.
and add operator slashing event for indexer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the whole looks good.
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
x/avs/keeper/keeper.go (1)
295-299
:⚠️ Potential issueHandle error from PublicKeyFromBytes.
The error from
PublicKeyFromBytes
is ignored. This could mask potential issues with malformed public keys.- pubKey, _ := bls.PublicKeyFromBytes(params.PubKey) + pubKey, err := bls.PublicKeyFromBytes(params.PubKey) + if err != nil { + return errorsmod.Wrap(types.ErrSigNotMatchPubKey, fmt.Sprintf("invalid public key for operator: %s, error: %v", params.OperatorAddress, err)) + }
🧹 Nitpick comments (7)
client/docs/swagger-ui/swagger.json (4)
73-181
: New Epoch Endpoint Addition: Validate Data Types and Consistency.
The new endpoint/exocore/epochs/v1/epoch/{identifier}
is introduced with a detailed schema that carefully documents all epoch-related properties. Ensure that the use of"string"
with a"format": "int64"
(e.g. for"current_epoch"
) is intentional and consistent with other API definitions—this pattern is common when handling large numbers in JSON, but it must be supported by the backend logic.
24868-24920
: QueryEpochInfoResponse Schema: Detailed and Extensive.
The response schema forexocore.epochs.v1.QueryEpochInfoResponse
is very detailed and the descriptions provide clarity on each field’s purpose. However, verify that properties like"current_epoch"
(defined as a"string"
with"format": "int64"
) are intended to be used in this manner. If possible, consider referencing common definitions to reduce duplication and simplify future maintenance.
40565-40625
: QueryAllValidatorsResponse Schema: Consistency and Modularity.
The schema forexocore.dogfood.v1.QueryAllValidatorsResponse
is comprehensive, including the validators list and pagination details. Note that the repeated, lengthy descriptions for fields (especially those related to theAny
type) might be streamlined by referencing a shared component (e.g. using$ref
if your tooling supports it) to ensure consistency and simplify future updates.
40686-40726
: QueryValidatorResponse Schema: Consistency with Minimal Validator Fields.
This schema mirrors the minimal validator details from the earlier endpoint and provides extensive documentation of nested objects (likepubkey
). While the level of detail is beneficial, consider abstracting common components (such as thepubkey
definition) to avoid redundancy across endpoints.x/avs/keeper/keeper.go (2)
254-257
: Improve error handling in GetAVSUSDValue.Consider including the AVS address in the error wrap to preserve context when the error occurs.
- return types.InvalidTaskID, errorsmod.Wrap(err, "failed to get AVS USD value") + return types.InvalidTaskID, errorsmod.Wrap(err, fmt.Sprintf("failed to get AVS USD value for AVS: %s", avsInfo.AvsAddress))
318-324
: Maintain consistent error handling patterns.The error handling is inconsistent in this section. Some errors use
errorsmod.Wrap
while others usefmt.Errorf
. Consider usingerrorsmod.Wrap
consistently.- return fmt.Errorf("avs does not exist,this avs address: %s", params.AvsAddress) + return errorsmod.Wrap(types.ErrUnregisterNonExistent, fmt.Sprintf("avs does not exist, avs address: %s", params.AvsAddress))x/delegation/keeper/delegation.go (1)
80-90
: IncludeassetID
in the event for improved visibility.
While you emit the delegation event with staker ID, operator, and amount, addingassetID
would provide uniform logging across both native and non-native asset delegations.Proposed patch:
ctx.EventManager().EmitEvent( sdk.NewEvent( delegationtype.EventTypeExoAssetDelegation, + sdk.NewAttribute(delegationtype.AttributeKeyAssetID, assetID), sdk.NewAttribute(delegationtype.AttributeKeyStakerID, sdk.AccAddress(params.StakerAddress).String()), sdk.NewAttribute(delegationtype.AttributeKeyOperator, params.OperatorAddress.String()), sdk.NewAttribute(delegationtype.AttributeKeyAmount, params.OpAmount.String()), ), )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/operator/types/tx.pb.go
is excluded by!**/*.pb.go
📒 Files selected for processing (7)
client/docs/swagger-ui/swagger.json
(6 hunks)proto/exocore/operator/v1/tx.proto
(1 hunks)testutil/network/util.go
(1 hunks)x/avs/keeper/keeper.go
(1 hunks)x/delegation/keeper/delegation.go
(4 hunks)x/operator/keeper/slash.go
(5 hunks)x/operator/keeper/usd_value.go
(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- proto/exocore/operator/v1/tx.proto
- testutil/network/util.go
- x/operator/keeper/slash.go
- x/operator/keeper/usd_value.go
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: goreleaser
🔇 Additional comments (7)
client/docs/swagger-ui/swagger.json (2)
17182-17228
: Minimal Validator Endpoint: Schema and Documentation Update.
The revised endpoint for/exocore/dogfood/v1/validator/{cons_addr}
now clearly specifies that only the bare-minimum validator details (public key, consensus address, and power) will be returned. The field definitions (e.g."format": "byte"
for the address and"format": "int64"
for power) appear consistent, but please ensure these formats align with overall API conventions across modules.
17276-17426
: All Validators Endpoint: Comprehensive Schema with Pagination.
The addition of the/exocore/dogfood/v1/validators
endpoint is well executed with an extensive schema that includes pagination parameters. Double-check that the data types (such as using"string"
with numeric formats like"uint64"
for pagination fields) meet client expectations and are consistent with existing endpoints.x/avs/keeper/keeper.go (2)
137-148
: LGTM! Improved error handling and event emission.The changes properly handle errors from
SetAVSInfo
and add event emission for tracking AVS creation, which enhances observability.
274-274
:❓ Verification inconclusive
Verify GetTaskID error handling.
The return value from
GetTaskID
is used without error checking. Please verify if this method can return an error that needs to be handled.
🏁 Script executed:
#!/bin/bash # Search for GetTaskID implementation to verify error handling ast-grep --pattern 'GetTaskID($$$) { $$$ }'Length of output: 47
🏁 Script executed:
#!/bin/bash # Search for the definition of GetTaskID in the repository, # including several lines before and after the definition to inspect its signature. rg -A 10 -B 5 "func (k *Keeper) GetTaskID" .Length of output: 46
Action Required: Confirm GetTaskID Error Handling
Automated searches did not reveal the definition ofGetTaskID
, so its error behavior remains unclear. Please manually verify whether this method is capable of returning an error. If an error can be returned, add explicit handling to ensure robustness.x/delegation/keeper/delegation.go (3)
11-11
: Good usage ofhexutil
.
The imported package is correctly utilized for encoding the undelegation record key, so no issues here.
54-56
: Validate potential negative amounts.
Although subtractingparams.OpAmount
from theWithdrawableAmount
is correct and guarded by a prior check, it’s worth reconfirming that this step won’t result in unexpected negative amounts in edge cases. It looks safe given the existing checks.
195-219
: Ensure error propagation from hooks is intended.
Emitting an event for undelegation adds valuable transparency. Consider whether reverting the entire transaction is desired ifAfterUndelegationStarted
fails. Also, including the staker’s raw address (in addition to the staker ID) may enhance debugging.
Many of our modules don't have sufficient mechanisms for indexers to track the changes to state (as a result of transactions / block / epoch).
This PR adds events to many modules, which are used by our fork of Callisto to index and prepare for the front-end of the Cosmos block explorer.
BREAKING: The x/operator module will not accept an operator unless the
FromAddress == ApproveAddress == EarningsAddress
.Summary by CodeRabbit
New Features
Enhancements