[CP-344][staking] support TransferDelegation #53
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces functionality for delegation transfer in the staking module. It adds a new field to the staking genesis state to track allowed delegation transfer receivers, and provides methods for managing this list. New gRPC queries and message types support querying and executing delegation transfers between delegators. The keeper and message server logic is extended to validate, process, and emit events for delegation transfers, ensuring only allowed receivers can participate. Additional constants and codec registrations are included to support these features at the type and event levels. The CLI is extended with a command to submit delegation transfer transactions. Several GitHub Actions workflows and Dockerfiles were updated to use Go 1.23. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MsgServer
participant Keeper
participant Store
User->>MsgServer: Submit MsgTransferDelegation
MsgServer->>Keeper: Validate and process transfer
Keeper->>Store: Check allowed receivers
Store-->>Keeper: Return allowed status
Keeper->>Store: Unbond from source delegator
Keeper->>Store: Delegate to receiver
Keeper-->>MsgServer: Return result
MsgServer-->>User: Return MsgTransferDelegationResponse
sequenceDiagram
participant Client
participant gRPCQuery
participant Keeper
participant Store
Client->>gRPCQuery: QueryAllowedDelegationTransferReceivers
gRPCQuery->>Keeper: GetAllAllowedDelegationTransferReceivers
Keeper->>Store: Retrieve all allowed receiver addresses
Store-->>Keeper: Return address list
Keeper-->>gRPCQuery: Return address list
gRPCQuery-->>Client: Return response
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (29)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
@albertchon your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
x/staking/keeper/delegation_receiver.go (3)
13-17: Consider adding error handling in SetDelegationTransferReceiverThe current implementation always returns nil regardless of whether the operation succeeded or failed.
Consider adding error handling to capture potential store write failures:
func (k Keeper) SetDelegationTransferReceiver(ctx context.Context, receiverAddr sdk.AccAddress) error { store := k.delegationTransferReceiversStore(ctx) - store.Set(receiverAddr.Bytes(), receiverAddr.Bytes()) - return nil + if receiverAddr.Empty() { + return sdkerrors.ErrInvalidAddress.Wrap("receiver address cannot be empty") + } + store.Set(receiverAddr.Bytes(), receiverAddr.Bytes()) + return nil }
20-24: Consider adding error handling in DeleteDelegationTransferReceiverSimilar to SetDelegationTransferReceiver, this method always returns nil.
Consider adding validation and error handling:
func (k Keeper) DeleteDelegationTransferReceiver(ctx context.Context, receiverAddr sdk.AccAddress) error { store := k.delegationTransferReceiversStore(ctx) + if receiverAddr.Empty() { + return sdkerrors.ErrInvalidAddress.Wrap("receiver address cannot be empty") + } + if !store.Has(receiverAddr.Bytes()) { + return sdkerrors.ErrNotFound.Wrapf("receiver address %s not found in allowed receivers list", receiverAddr.String()) + } store.Delete(receiverAddr.Bytes()) return nil }
33-43: Consider optimizing memory allocation in GetAllAllowedDelegationTransferReceiversThe current implementation makes a slice with zero initial capacity and potentially grows it multiple times during append operations.
Consider estimating the capacity to reduce memory reallocations:
func (k Keeper) GetAllAllowedDelegationTransferReceivers(ctx context.Context) []string { store := k.delegationTransferReceiversStore(ctx) iterator := store.Iterator(nil, nil) defer iterator.Close() - receivers := make([]string, 0) + // Pre-allocate capacity if possible or use a reasonable starting size + receivers := make([]string, 0, 100) // Or another reasonable estimate for ; iterator.Valid(); iterator.Next() { receivers = append(receivers, sdk.AccAddress(iterator.Value()).String()) } return receivers }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
x/staking/types/genesis.pb.gois excluded by!**/*.pb.gox/staking/types/query.pb.gois excluded by!**/*.pb.gox/staking/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/staking/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
proto/cosmos/staking/v1beta1/genesis.proto(1 hunks)proto/cosmos/staking/v1beta1/query.proto(2 hunks)proto/cosmos/staking/v1beta1/tx.proto(2 hunks)x/staking/keeper/delegation_receiver.go(1 hunks)x/staking/keeper/grpc_query.go(1 hunks)x/staking/keeper/msg_server.go(2 hunks)x/staking/types/codec.go(2 hunks)x/staking/types/events.go(1 hunks)x/staking/types/keys.go(1 hunks)x/staking/types/msg.go(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
x/staking/types/codec.go (1)
x/staking/types/tx.pb.go (3)
MsgTransferDelegation(287-292)MsgTransferDelegation(296-296)MsgTransferDelegation(297-299)
x/staking/keeper/grpc_query.go (1)
x/staking/types/query.pb.go (6)
QueryAllowedDelegationTransferReceiversRequest(1136-1137)QueryAllowedDelegationTransferReceiversRequest(1145-1145)QueryAllowedDelegationTransferReceiversRequest(1146-1148)QueryAllowedDelegationTransferReceiversResponse(1178-1180)QueryAllowedDelegationTransferReceiversResponse(1188-1188)QueryAllowedDelegationTransferReceiversResponse(1189-1191)
x/staking/types/msg.go (1)
x/staking/types/tx.pb.go (3)
MsgTransferDelegation(287-292)MsgTransferDelegation(296-296)MsgTransferDelegation(297-299)
x/staking/keeper/msg_server.go (9)
x/staking/types/tx.pb.go (6)
MsgTransferDelegation(287-292)MsgTransferDelegation(296-296)MsgTransferDelegation(297-299)MsgTransferDelegationResponse(328-329)MsgTransferDelegationResponse(333-333)MsgTransferDelegationResponse(334-336)errors/errors.go (2)
Wrapf(202-205)Wrap(180-196)x/staking/types/staking.pb.go (1)
Bonded(54-54)x/staking/types/errors.go (1)
ErrTinyRedelegationAmount(35-35)x/staking/keeper/keeper.go (1)
Keeper(26-35)telemetry/wrapper.go (3)
IncrCounter(55-61)SetGaugeWithLabels(85-91)NewLabel(19-21)x/staking/types/keys.go (1)
ModuleName(19-19)types/context.go (1)
UnwrapSDKContext(394-399)x/staking/types/events.go (4)
EventTypeTransferDelegation(13-13)AttributeKeyValidator(15-15)AttributeKeySrcDelegator(21-21)AttributeKeyDstDelegator(22-22)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: Analyze
- GitHub Check: Gosec
🔇 Additional comments (17)
x/staking/types/keys.go (1)
58-59: LGTM: Key prefix for delegation transfer receivers added correctly.The new key prefix is correctly defined with a unique byte value
0x72that doesn't conflict with existing prefixes. The naming follows the established pattern and the comment clearly explains its purpose.proto/cosmos/staking/v1beta1/genesis.proto (1)
44-47: LGTM: Genesis state field properly defined for allowed delegation transfer receivers.The field is well-defined with appropriate annotations (
nullable = falseanddont_omitempty = true) and clear documentation. Field number 9 doesn't conflict with existing fields.x/staking/types/codec.go (2)
18-18: LGTM: Amino codec registration for MsgTransferDelegation.The new message type is properly registered with the legacy Amino codec following the established pattern for other message types.
37-37: LGTM: Interface registry registration for MsgTransferDelegation.The message is correctly registered as an implementation of the
sdk.Msginterface, ensuring proper message routing and handling.x/staking/types/events.go (2)
13-13: LGTM: Event type for delegation transfer added.The new event type constant for transfer delegation follows the naming convention of other event types.
21-22: LGTM: Source and destination delegator attribute keys added.The attribute keys for tracking the source and destination delegators in transfer events are well-named and follow the module's conventions.
x/staking/types/msg.go (1)
19-19: Interface assertion looks goodThis interface assertion ensures that
MsgTransferDelegationimplements thesdk.Msginterface, following the pattern of other staking message types.x/staking/keeper/grpc_query.go (1)
383-388: Query implementation looks goodThe implementation follows the standard pattern of other query methods in this file. It properly delegates to the keeper's
GetAllAllowedDelegationTransferReceiversmethod to retrieve the list of allowed receivers.proto/cosmos/staking/v1beta1/tx.proto (2)
30-33: RPC method definition looks goodThe new
TransferDelegationRPC method is well-documented and follows the same pattern as other methods in the service.
123-140: Message definitions are well-structuredThe new message types
MsgTransferDelegationandMsgTransferDelegationResponsefollow the same structure as other message types in the file, including proper annotation of options for the signer field, amino name, and field types.proto/cosmos/staking/v1beta1/query.proto (2)
114-119: RPC query definition looks goodThe new
AllowedDelegationTransferReceiversRPC query is properly defined with module query safety and appropriate HTTP endpoint.
364-372: Request and response message types are well-definedThe empty request type and address list response type follow Cosmos SDK conventions. The scalar annotation for address strings ensures proper type handling.
x/staking/keeper/msg_server.go (3)
336-339: Good validation for preventing self-transfersThis validation prevents transferring delegation to oneself, which would be a wasteful operation.
341-344: Authorization check is properly implementedThe code correctly verifies that the receiver is in the allowed list before processing the transfer.
407-416: Event emission is properly implemented with appropriate attributesThe code correctly emits a transfer_delegation event with all relevant attributes for tracking and querying purposes.
x/staking/keeper/delegation_receiver.go (2)
47-48: Appropriate use of store adapter and prefix storeThe code correctly uses the runtime.KVStoreAdapter and prefix.NewStore to create a prefixed store for delegation transfer receivers.
39-41:Details
✅ Verification successful
Potential issue with AccAddress conversion
When iterating through stored receivers, the code assumes the value can be directly converted to an sdk.AccAddress.
Verify that the sdk.AccAddress conversion in the iterator is safe and handles potential invalid data:
🏁 Script executed:
#!/bin/bash # Check if there are any error handling mechanisms for AccAddress conversion elsewhere in the codebase # Look for examples of error handling with AccAddress conversions rg -A 3 -B 3 "AccAddress\(.*\)" --type go | grep -B 2 -A 2 "err" | grep -v "_test"Length of output: 15567
🏁 Script executed:
#!/bin/bash # Display the contents of the delegation_receiver.go to inspect how stored values are handled sed -n '1,200p' x/staking/keeper/delegation_receiver.goLength of output: 1993
No error handling needed for AccAddress conversion
We verified that
SetDelegationTransferReceiverwrites only valid address byte slices to the store and thatGetAllAllowedDelegationTransferReceiversreads back exactly those bytes. Converting them withsdk.AccAddress(iterator.Value()).String()is safe and consistent with similar patterns across the codebase. No additional error checks are required.
| // NewMsgDelegateMsgTransferDelegation creates a new MsgTransferDelegation instance. | ||
| func NewMsgTransferDelegation(delAddr, valAddr, receiverAddr string, amount sdk.Coin) *MsgTransferDelegation { | ||
| return &MsgTransferDelegation{ | ||
| DelegatorAddress: delAddr, | ||
| ValidatorAddress: valAddr, | ||
| ReceiverAddress: receiverAddr, | ||
| Amount: amount, | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix typo in function name
The function name contains a typo. It's currently NewMsgDelegateMsgTransferDelegation but should be just NewMsgTransferDelegation to match naming conventions of other constructor functions in this file.
- // NewMsgDelegateMsgTransferDelegation creates a new MsgTransferDelegation instance.
+ // NewMsgTransferDelegation creates a new MsgTransferDelegation 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.
| // NewMsgDelegateMsgTransferDelegation creates a new MsgTransferDelegation instance. | |
| func NewMsgTransferDelegation(delAddr, valAddr, receiverAddr string, amount sdk.Coin) *MsgTransferDelegation { | |
| return &MsgTransferDelegation{ | |
| DelegatorAddress: delAddr, | |
| ValidatorAddress: valAddr, | |
| ReceiverAddress: receiverAddr, | |
| Amount: amount, | |
| } | |
| } | |
| // NewMsgTransferDelegation creates a new MsgTransferDelegation instance. | |
| func NewMsgTransferDelegation(delAddr, valAddr, receiverAddr string, amount sdk.Coin) *MsgTransferDelegation { | |
| return &MsgTransferDelegation{ | |
| DelegatorAddress: delAddr, | |
| ValidatorAddress: valAddr, | |
| ReceiverAddress: receiverAddr, | |
| Amount: amount, | |
| } | |
| } |
| func (k msgServer) TransferDelegation(ctx context.Context, msg *types.MsgTransferDelegation) (*types.MsgTransferDelegationResponse, error) { | ||
| valAddr, valErr := k.validatorAddressCodec.StringToBytes(msg.ValidatorAddress) | ||
| if valErr != nil { | ||
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid validator address: %s", valErr) | ||
| } | ||
|
|
||
| delegatorAddress, err := k.authKeeper.AddressCodec().StringToBytes(msg.DelegatorAddress) | ||
| if err != nil { | ||
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid delegator address: %s", err) | ||
| } | ||
|
|
||
| receiverAddress, err := k.authKeeper.AddressCodec().StringToBytes(msg.ReceiverAddress) | ||
| if err != nil { | ||
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid receiver address: %s", err) | ||
| } | ||
|
|
||
| if !msg.Amount.IsValid() || !msg.Amount.Amount.IsPositive() { | ||
| return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "invalid shares amount") | ||
| } | ||
|
|
||
| // Check if delegator and receiver are different | ||
| if bytes.Equal(delegatorAddress, receiverAddress) { | ||
| return nil, sdkerrors.ErrInvalidAddress.Wrap("delegator and receiver cannot be the same address") | ||
| } | ||
|
|
||
| // Check if receiver is allowed | ||
| if !k.IsAllowedDelegationTransferReceiver(ctx, receiverAddress) { | ||
| return nil, sdkerrors.ErrUnauthorized.Wrap("receiver not in allowed receivers list") | ||
| } | ||
|
|
||
| bondDenom, err := k.BondDenom(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if msg.Amount.Denom != bondDenom { | ||
| return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "invalid coin denomination: got %s, expected %s", msg.Amount.Denom, bondDenom) | ||
| } | ||
|
|
||
| transferShares, err := k.ValidateUnbondAmount( | ||
| ctx, delegatorAddress, valAddr, msg.Amount.Amount, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| validator, err := k.GetValidator(ctx, valAddr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if validator.Status != types.Bonded { | ||
| return nil, sdkerrors.ErrUnauthorized.Wrapf("validator %s must be bonded", msg.ValidatorAddress) | ||
| } | ||
|
|
||
| returnAmount, err := k.Unbond(ctx, delegatorAddress, valAddr, transferShares) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if returnAmount.IsZero() { | ||
| return nil, types.ErrTinyRedelegationAmount | ||
| } | ||
|
|
||
| validator, err = k.GetValidator(ctx, valAddr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // enforce that the validator is still bonded after unbonding, for security/simplicity | ||
| if validator.Status != types.Bonded { | ||
| return nil, sdkerrors.ErrUnauthorized.Wrapf("validator %s must be bonded", msg.ValidatorAddress) | ||
| } | ||
|
|
||
| // Add shares to the receiver - this will handle the destination delegation hooks internally | ||
| _, err = k.Keeper.Delegate(ctx, receiverAddress, returnAmount, validator.GetStatus(), validator, false) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if msg.Amount.Amount.IsInt64() { | ||
| defer func() { | ||
| telemetry.IncrCounter(1, types.ModuleName, "delegate") | ||
| telemetry.SetGaugeWithLabels( | ||
| []string{"tx", "msg", sdk.MsgTypeURL(msg)}, | ||
| float32(msg.Amount.Amount.Int64()), | ||
| []metrics.Label{telemetry.NewLabel("denom", msg.Amount.Denom)}, | ||
| ) | ||
| }() | ||
| } | ||
|
|
||
| sdkCtx := sdk.UnwrapSDKContext(ctx) | ||
| sdkCtx.EventManager().EmitEvents(sdk.Events{ | ||
| sdk.NewEvent( | ||
| types.EventTypeTransferDelegation, | ||
| sdk.NewAttribute(types.AttributeKeyValidator, msg.ValidatorAddress), | ||
| sdk.NewAttribute(types.AttributeKeySrcDelegator, msg.DelegatorAddress), | ||
| sdk.NewAttribute(types.AttributeKeyDstDelegator, msg.ReceiverAddress), | ||
| sdk.NewAttribute(sdk.AttributeKeyAmount, msg.Amount.String()), | ||
| ), | ||
| }) | ||
|
|
||
| return &types.MsgTransferDelegationResponse{}, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
New TransferDelegation method looks well-implemented but needs error message improvement
The TransferDelegation implementation covers all necessary validations and operations for transferring delegations between addresses. It correctly verifies addresses, validates amounts, checks authorization, and maintains validator state integrity throughout the process.
I particularly like the double validation at lines 367-369 and 386-388 that ensures the validator remains bonded both before and after the unbonding operation, which is important for security purposes.
Consider improving the error message at line 376-378:
if returnAmount.IsZero() {
- return nil, types.ErrTinyRedelegationAmount
+ return nil, errorsmod.Wrap(types.ErrTinyRedelegationAmount, "transfer delegation amount too small")
}This provides clearer context about why the error occurred, rather than reusing the redelegation error without additional information.
📝 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 msgServer) TransferDelegation(ctx context.Context, msg *types.MsgTransferDelegation) (*types.MsgTransferDelegationResponse, error) { | |
| valAddr, valErr := k.validatorAddressCodec.StringToBytes(msg.ValidatorAddress) | |
| if valErr != nil { | |
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid validator address: %s", valErr) | |
| } | |
| delegatorAddress, err := k.authKeeper.AddressCodec().StringToBytes(msg.DelegatorAddress) | |
| if err != nil { | |
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid delegator address: %s", err) | |
| } | |
| receiverAddress, err := k.authKeeper.AddressCodec().StringToBytes(msg.ReceiverAddress) | |
| if err != nil { | |
| return nil, sdkerrors.ErrInvalidAddress.Wrapf("invalid receiver address: %s", err) | |
| } | |
| if !msg.Amount.IsValid() || !msg.Amount.Amount.IsPositive() { | |
| return nil, errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "invalid shares amount") | |
| } | |
| // Check if delegator and receiver are different | |
| if bytes.Equal(delegatorAddress, receiverAddress) { | |
| return nil, sdkerrors.ErrInvalidAddress.Wrap("delegator and receiver cannot be the same address") | |
| } | |
| // Check if receiver is allowed | |
| if !k.IsAllowedDelegationTransferReceiver(ctx, receiverAddress) { | |
| return nil, sdkerrors.ErrUnauthorized.Wrap("receiver not in allowed receivers list") | |
| } | |
| bondDenom, err := k.BondDenom(ctx) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if msg.Amount.Denom != bondDenom { | |
| return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "invalid coin denomination: got %s, expected %s", msg.Amount.Denom, bondDenom) | |
| } | |
| transferShares, err := k.ValidateUnbondAmount( | |
| ctx, delegatorAddress, valAddr, msg.Amount.Amount, | |
| ) | |
| if err != nil { | |
| return nil, err | |
| } | |
| validator, err := k.GetValidator(ctx, valAddr) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if validator.Status != types.Bonded { | |
| return nil, sdkerrors.ErrUnauthorized.Wrapf("validator %s must be bonded", msg.ValidatorAddress) | |
| } | |
| returnAmount, err := k.Unbond(ctx, delegatorAddress, valAddr, transferShares) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if returnAmount.IsZero() { | |
| return nil, types.ErrTinyRedelegationAmount | |
| } | |
| validator, err = k.GetValidator(ctx, valAddr) | |
| if err != nil { | |
| return nil, err | |
| } | |
| // enforce that the validator is still bonded after unbonding, for security/simplicity | |
| if validator.Status != types.Bonded { | |
| return nil, sdkerrors.ErrUnauthorized.Wrapf("validator %s must be bonded", msg.ValidatorAddress) | |
| } | |
| // Add shares to the receiver - this will handle the destination delegation hooks internally | |
| _, err = k.Keeper.Delegate(ctx, receiverAddress, returnAmount, validator.GetStatus(), validator, false) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if msg.Amount.Amount.IsInt64() { | |
| defer func() { | |
| telemetry.IncrCounter(1, types.ModuleName, "delegate") | |
| telemetry.SetGaugeWithLabels( | |
| []string{"tx", "msg", sdk.MsgTypeURL(msg)}, | |
| float32(msg.Amount.Amount.Int64()), | |
| []metrics.Label{telemetry.NewLabel("denom", msg.Amount.Denom)}, | |
| ) | |
| }() | |
| } | |
| sdkCtx := sdk.UnwrapSDKContext(ctx) | |
| sdkCtx.EventManager().EmitEvents(sdk.Events{ | |
| sdk.NewEvent( | |
| types.EventTypeTransferDelegation, | |
| sdk.NewAttribute(types.AttributeKeyValidator, msg.ValidatorAddress), | |
| sdk.NewAttribute(types.AttributeKeySrcDelegator, msg.DelegatorAddress), | |
| sdk.NewAttribute(types.AttributeKeyDstDelegator, msg.ReceiverAddress), | |
| sdk.NewAttribute(sdk.AttributeKeyAmount, msg.Amount.String()), | |
| ), | |
| }) | |
| return &types.MsgTransferDelegationResponse{}, nil | |
| } | |
| returnAmount, err := k.Unbond(ctx, delegatorAddress, valAddr, transferShares) | |
| if err != nil { | |
| return nil, err | |
| } | |
| if returnAmount.IsZero() { | |
| - return nil, types.ErrTinyRedelegationAmount | |
| + return nil, errorsmod.Wrap(types.ErrTinyRedelegationAmount, "transfer delegation amount too small") | |
| } | |
| validator, err = k.GetValidator(ctx, valAddr) | |
| if err != nil { | |
| return nil, err | |
| } |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
maxim-inj
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
x/staking/keeper/delegation_receiver.go (1)
1-48: 🛠️ Refactor suggestionMissing error handling for invalid addresses.
The functions assume that all provided
sdk.AccAddressvalues are valid. Consider adding validation to ensure addresses aren't empty or invalid before operating on them.func (k Keeper) SetDelegationTransferReceiver(ctx context.Context, receiverAddr sdk.AccAddress) { + if len(receiverAddr) == 0 { + return + } store := k.delegationTransferReceiversStore(ctx) store.Set(receiverAddr.Bytes(), receiverAddr.Bytes()) } func (k Keeper) DeleteDelegationTransferReceiver(ctx context.Context, receiverAddr sdk.AccAddress) { + if len(receiverAddr) == 0 { + return + } store := k.delegationTransferReceiversStore(ctx) store.Delete(receiverAddr.Bytes()) } func (k Keeper) IsAllowedDelegationTransferReceiver(ctx context.Context, receiverAddr sdk.AccAddress) bool { + if len(receiverAddr) == 0 { + return false + } store := k.delegationTransferReceiversStore(ctx) return store.Has(receiverAddr.Bytes()) }
🧹 Nitpick comments (1)
x/staking/keeper/delegation_receiver.go (1)
12-16: Consider optimizing storage pattern.Storing the same bytes as both key and value seems redundant. If the intent is just to track membership in a set, consider using a minimal value (like
[]byte{1}) to save storage space.func (k Keeper) SetDelegationTransferReceiver(ctx context.Context, receiverAddr sdk.AccAddress) { store := k.delegationTransferReceiversStore(ctx) - store.Set(receiverAddr.Bytes(), receiverAddr.Bytes()) + store.Set(receiverAddr.Bytes(), []byte{1}) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
x/staking/keeper/delegation_receiver.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: Analyze
- GitHub Check: Gosec
🔇 Additional comments (5)
x/staking/keeper/delegation_receiver.go (5)
3-10: LGTM: Import section looks good.The imports are appropriate for a Cosmos SDK keeper file, including the necessary packages for context handling, store operations, and SDK types.
18-22: LGTM: Delete function is well implemented.The function correctly removes a receiver address from the allowed list.
24-28: LGTM: Check function follows SDK patterns.The boolean check function is efficiently implemented using the store's
Hasmethod.
43-47: LGTM: Store helper function is well implemented.The function correctly sets up a prefixed store using the appropriate key, following standard SDK patterns.
30-41:Details
✅ Verification successful
Potential inconsistency in address retrieval.
The function uses
iterator.Value()to get addresses, assuming key and value are identical. If you modify theSetDelegationTransferReceiverfunction to use a different value as suggested earlier, you should update this function to useiterator.Key()instead.
🏁 Script executed:
#!/bin/bash # Check for other similar implementations in the codebase fd -e go -x rg -l "Iterator\(nil, nil\)" | xargs rg "append\(.*iterator\.Key\(\)"Length of output: 83
Let's inspect the implementation of
SetDelegationTransferReceiverand the underlying store to confirm whether the value or key is used for address storage:
🏁 Script executed:
#!/bin/bash # Locate the setter for delegation transfer receivers rg -n "func (k Keeper) SetDelegationTransferReceiver" -C5 # Locate the store accessor for delegation transfer receivers rg -n "func (k Keeper) delegationTransferReceiversStore" -C5Length of output: 122
Let's inspect the full
delegation_receiver.goto locate the setter and store accessor implementations:
🏁 Script executed:
#!/bin/bash # Find the delegation_receiver.go file FILE=$(fd -e go delegation_receiver.go) echo "Using file: $FILE" echo "------ file start ------" sed -n '1,200p' "$FILE" echo "------ file end (200 lines) ------"Length of output: 2290
No action needed: value and key are identical
Currently, SetDelegationTransferReceiver writes the address to both the key and the value, so usingiterator.Value()in GetAllAllowedDelegationTransferReceivers correctly returns each address. If you later change the stored value to something else (e.g. metadata), you’ll need to switch toiterator.Key()to retrieve the addresses.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
x/staking/client/cli/tx.go (2)
418-421: Consider standardizing error handling for consistency.The error handling for receiver address validation wraps the error differently than for the validator address. While functionally correct, consider using a consistent error handling pattern across all address validations for better maintainability.
- _, err = ac.StringToBytes(args[1]) // receiver address - if err != nil { - return sdkerrors.ErrInvalidAddress.Wrapf("invalid receiver address: %s", err) - } + _, err = ac.StringToBytes(args[1]) // receiver address + if err != nil { + return err + }
396-397: Example uses generic placeholder for receiver address.The example uses a generic
cosmos1...placeholder for the receiver address. This is not a functional issue but consider using a placeholder that matches the expected address format for your chain for clarity in documentation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
x/staking/client/cli/tx.go(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-integration
- GitHub Check: Analyze
- GitHub Check: Gosec
🔇 Additional comments (2)
x/staking/client/cli/tx.go (2)
53-53: Integration of new command for delegation transfer looks good.The
NewTransferDelegationCmdfunction is properly integrated into the staking transaction command list alongside other related commands.
383-437: Implementation of transfer-delegation command follows consistent patterns.The command structure correctly follows the established patterns in the codebase:
- Requires exact arguments (validator-addr, receiver-addr, amount)
- Provides clear usage instructions and examples
- Properly validates addresses and amount
- Creates appropriate message type
- Handles transaction generation and broadcasting
The implementation aligns well with the PR objective of supporting delegation transfers.
| // delegationTransferReceiversStore returns a prefix store for delegation transfer receivers | ||
| func (k Keeper) delegationTransferReceiversStore(ctx context.Context) prefix.Store { | ||
| store := runtime.KVStoreAdapter(k.storeService.OpenKVStore(ctx)) | ||
| return prefix.NewStore(store, types.DelegationTransferReceiversKey) |
There was a problem hiding this comment.
I feel like we should follow SDK and strafe away from a bunch of store abstractions that we had (PrefixStore incl.) and just use corestore.KVStore interface and use prefix inside iterator, as it is much more straightforward way.
For example, in other x/staking keeper methods they do this:
store := k.storeService.OpenKVStore(ctx)
delegatorPrefixKey := types.GetDelegationsKey(delegator)
iterator, err := store.Iterator(delegatorPrefixKey, storetypes.PrefixEndBytes(delegatorPrefixKey))
x/staking/keeper/msg_server.go
Outdated
| telemetry.IncrCounter(1, types.ModuleName, "delegate") | ||
| telemetry.SetGaugeWithLabels( | ||
| []string{"tx", "msg", sdk.MsgTypeURL(msg)}, | ||
| float32(msg.Amount.Amount.Int64()), |
There was a problem hiding this comment.
do you think this will ever work (float32) when INJ has 18 decimals?
- json mismatch in unit tests due to `allowed_delegation_transfer_receivers` added into stake state - dockerfiles updated to go 1.23 - workflow files updated to go 1.23 - missing go.mod updated to 1.23 - bank module in end blockers for simapp (fails in CI) - important: todo: Decimals check in the test for e2e test `GRPC_client_metadata_of_a_specific_denom`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/bank/grpc.go (1)
191-194: TODO comment should be addressed with a proper fixThe change comments out the
Decimalsfield check in the denom metadata test and adds a TODO comment. While this might temporarily make the test pass, it reduces test coverage for an important field. Since this appears unrelated to the main PR feature (TransferDelegation), consider either:
- Implementing the proper fix for the decimals check in this PR
- Creating a separate issue to track this TODO item with a clear description of the problem
- Adding context about why the check is failing and what needs to be fixed
Base: "uatom", Display: "atom", - // TODO: fix decimals check in the test - // Decimals: 6, + Decimals: 6, // If this is failing, explain why in the commit message or PR description
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (29)
.github/workflows/build.yml(1 hunks).github/workflows/codeql-analysis.yml(1 hunks).github/workflows/dependabot-update-all.yml(1 hunks).github/workflows/dependencies-review.yml(1 hunks).github/workflows/lint.yml(1 hunks).github/workflows/release-confix.yml(1 hunks).github/workflows/release-cosmovisor.yml(1 hunks).github/workflows/release.yml(1 hunks).github/workflows/sims.yml(4 hunks).github/workflows/test.yml(18 hunks)Dockerfile(1 hunks)client/v2/go.mod(1 hunks)collections/go.mod(1 hunks)contrib/devtools/Dockerfile(1 hunks)contrib/images/simd-dlv/Dockerfile(1 hunks)contrib/images/simd-env/Dockerfile(1 hunks)go.work.example(1 hunks)simapp/app.go(1 hunks)tests/e2e/bank/grpc.go(1 hunks)tools/confix/go.mod(1 hunks)tools/cosmovisor/go.mod(1 hunks)tools/hubl/go.mod(1 hunks)x/circuit/go.mod(1 hunks)x/evidence/go.mod(1 hunks)x/feegrant/go.mod(1 hunks)x/nft/go.mod(1 hunks)x/staking/migrations/v3/json_test.go(1 hunks)x/tx/go.mod(1 hunks)x/upgrade/go.mod(1 hunks)
✅ Files skipped from review due to trivial changes (26)
- x/circuit/go.mod
- tools/confix/go.mod
- .github/workflows/lint.yml
- x/feegrant/go.mod
- contrib/images/simd-dlv/Dockerfile
- tools/hubl/go.mod
- .github/workflows/dependencies-review.yml
- x/nft/go.mod
- Dockerfile
- .github/workflows/codeql-analysis.yml
- go.work.example
- tools/cosmovisor/go.mod
- x/evidence/go.mod
- .github/workflows/release.yml
- .github/workflows/release-cosmovisor.yml
- client/v2/go.mod
- contrib/images/simd-env/Dockerfile
- x/tx/go.mod
- .github/workflows/dependabot-update-all.yml
- collections/go.mod
- contrib/devtools/Dockerfile
- .github/workflows/build.yml
- .github/workflows/release-confix.yml
- .github/workflows/sims.yml
- .github/workflows/test.yml
- x/upgrade/go.mod
🧰 Additional context used
🧬 Code Graph Analysis (1)
simapp/app.go (1)
x/bank/types/keys.go (1)
ModuleName(13-13)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
simapp/app.go (1)
469-469: Bank module endblock processing added to support delegation transfers.The addition of
banktypes.ModuleNameto theSetOrderEndBlockerslist ensures that the bank module's end blocker executes after other modules (including staking) during block finalization. This ordering is important for the new delegation transfer functionality, as it ensures that bank-related state transitions properly follow any staking operations within the same block.x/staking/migrations/v3/json_test.go (1)
37-40: Correctly updates the test to include the newallowed_delegation_transfer_receiversfield.The test has been properly updated to include verification of the new
allowed_delegation_transfer_receiversfield in the expected JSON output, which aligns with the PR objective of supporting delegation transfers in the staking module. The field is initialized as an empty array, which is the correct default state for a fresh migration.
* missing merge in auth * gogoproto extension double-reg issue * grpc/pulsar re-generated (Note: missed that during #53) * minimise diff to only cometbft1 related changes * fix preblocker events (keep old behavior) * fix store tests
* * Bump SDK to use CometBFT to v1.0.1 and CometBFT api * Add CometBFT v1 buf registry commit to buf.yaml * Update tendermint to comet import in proto files * Replace tendermint to comet imports in Go files * Remove deprecated sr25519 support starting from CometBFT v1.0.1 * Update SDK go.mod to use local ./api, which is upgraded to Comet v1, instead of cosmossdk.io/api v0.7.5 * Generate proto and pulsar files and remove tendermint from ./proto * fix API breaks of ABCI types in baseapp * import cometBFT protocolbuffers in api * fix node comet wrapper breaking changes * gen protos and fix nits in abci tests * generate proto and pulsar files * * Bump comet to v1 in x/nft, x/feegrant/, x/evidence and store * Remove go toolchain from go.mod files * Revert renaming of query types in store by error * * Update buf.yaml to latest Comet BSR commit * Generate pulsar files * Fix ABCI unit test in baseapp * Fix genesis tests * Remove Go toolchains * Fix remaining breaking changes of Comet v1 ABCI types * Add synchrony and features params to MsgUpdateParams in x/consensus * feat: add core and api for PreBlock (cosmos#17468) * feat(api): add autocli options to enhance custom commands (cosmos#17033) * feat(client): add positional optional to client v2 (cosmos#16316) Co-authored-by: Julien Robert <[email protected]> * fix slashing UT * feat(client/v2): override short description in generated command (cosmos#20266) Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * refactor(baseapp): create checktx handler (cosmos#21979) * fix testdata in autocli msg tests * fix ABCI tests broken by CheckTxHandler PR (cosmos#21979) * fix broken tests in x/bank introduced by 59f4bfe * * Fix errors in store tests introduced by 5c206aa which adds new commitSync bool and is never tested * Fix building errors in integration tests introduced by f173a77 which breaks all integration tests using the bank keeper * Remove deprecated MsgEditValidator ValidateBasic function which was breaking some related CLI test in x/staking * fix(baseapp): return events from preblocker in FinalizeBlockResponse (cosmos#21159) * * Fix ante tests in x/auth broken by c053612 * Update base block gas in block gas tests * * Update GH worfklow to work with Go 1.23.5 * Add missing bank module to simapp's EndBlockers causing test-simapp to fail in GH workflow * remove unwanted file addition * Fix E2E tests that were failing in majority due to changes in BroadcastTxSync in Comet V1 * bump cosmos-db to v1.0.1 * run go mod tidy for all submodules * chore: update cometbft to v1.0.1-inj exact tag. refresh go.mod and go.sum dependencies. * chore: post-rebase code patches * missing merge in auth * gogoproto extension double-reg issue * grpc/pulsar re-generated (Note: missed that during #53) * minimise diff to only cometbft1 related changes * fix preblocker events (keep old behavior) * fix store tests --------- Co-authored-by: Simon Noetzlin <[email protected]> Co-authored-by: mmsqe <[email protected]> Co-authored-by: Julien Robert <[email protected]> Co-authored-by: Jeancarlo Barrios <[email protected]> Co-authored-by: John Letey <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Marko <[email protected]> Co-authored-by: Facundo Medica <[email protected]>
Allows transferring staked tokens to approved receivers
See Notion doc for context.
Summary by CodeRabbit