fix(accounts): lockup check sender in context not in message #23621
Conversation
📝 WalkthroughWalkthroughThe changes remove the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Unauthorized Sender ("hacker")
participant Context as Account Context
participant Server as Lockup Account Server
Client->>Context: Create mock context with "hacker"
Client->>Server: Initiate SendCoins operation
Server->>Server: Retrieve sender via accountstd.Sender(ctx)
alt Sender unauthorized
Server-->>Client: Return error response
else
Server-->>Client: Process transaction (not applicable here)
end
Possibly related PRs
Suggested reviewers
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@tac0turtle 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 (2)
x/accounts/defaults/lockup/periodic_locking_account_test.go (1)
255-275: Optimize the query handler in newMockContext2.The query handler contains a switch statement with only a default case, which can be simplified.
Consider simplifying the query handler:
- }, func(ctx context.Context, req transaction.Msg) (transaction.Msg, error) { - typeUrl := sdk.MsgTypeURL(req) - switch typeUrl { - default: - return nil, errors.New("unrecognized request type") - } + }, func(ctx context.Context, req transaction.Msg) (transaction.Msg, error) { + return nil, errors.New("unrecognized request type")🧰 Tools
🪛 golangci-lint (1.62.2)
269-269: singleCaseSwitch: found switch with default case only
(gocritic)
x/accounts/defaults/lockup/lockup.go (1)
333-333: Remove redundant Error() call in error wrapping.The Error() call is redundant when wrapping an error, as the error message is already included.
-return sdkerrors.ErrInvalidAddress.Wrapf("invalid owner address: %s", err.Error()) +return sdkerrors.ErrInvalidAddress.Wrapf("invalid owner address: %s", err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/accounts/defaults/lockup/v1/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go(1 hunks)x/accounts/defaults/lockup/continuous_locking_account_test.go(0 hunks)x/accounts/defaults/lockup/delayed_locking_account_test.go(0 hunks)x/accounts/defaults/lockup/lockup.go(6 hunks)x/accounts/defaults/lockup/periodic_locking_account_test.go(2 hunks)x/accounts/defaults/lockup/permanent_locking_account_test.go(0 hunks)x/accounts/defaults/lockup/utils_test.go(1 hunks)x/accounts/msg_server.go(1 hunks)x/accounts/msg_server_test.go(1 hunks)x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto(0 hunks)
💤 Files with no reviewable changes (4)
- x/accounts/defaults/lockup/delayed_locking_account_test.go
- x/accounts/defaults/lockup/permanent_locking_account_test.go
- x/accounts/defaults/lockup/continuous_locking_account_test.go
- x/accounts/proto/cosmos/accounts/defaults/lockup/v1/tx.proto
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/defaults/lockup/utils_test.gox/accounts/msg_server_test.gox/accounts/defaults/lockup/periodic_locking_account_test.gox/accounts/msg_server.gox/accounts/defaults/lockup/lockup.goapi/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go
`**/*_test.go`: "Assess the unit test code assessing suffici...
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/accounts/defaults/lockup/utils_test.gox/accounts/msg_server_test.gox/accounts/defaults/lockup/periodic_locking_account_test.go
🪛 golangci-lint (1.62.2)
x/accounts/defaults/lockup/periodic_locking_account_test.go
269-269: singleCaseSwitch: found switch with default case only
(gocritic)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-system-v2
- GitHub Check: test-sim-nondeterminism
- GitHub Check: Analyze
- GitHub Check: Summary
🔇 Additional comments (7)
api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go (1)
4815-4818: Confirm usage of newly introduced proto imports.These added imports (e.g.,
cosmos_proto/cosmos.proto,gogoproto/gogo.proto,google/protobuf/any.proto,google/protobuf/timestamp.proto) appear consistent with the usage oftimestamppb.Timestampandanypb.Anythroughout the file. No issues found.x/accounts/msg_server.go (1)
85-89: LGTM! Return statement added for proper method completion.The added return statement correctly returns the encoded response from the execution.
x/accounts/msg_server_test.go (1)
82-116: LGTM! Well-structured test for unauthorized execution.The test effectively validates that unauthorized users cannot execute transactions on accounts they don't own. Good security boundary testing with clear setup and assertions.
x/accounts/defaults/lockup/utils_test.go (1)
73-73: LGTM! Consistent sender parameter update.The change from "sender" to "owner" aligns with the broader refactor of sender handling across the codebase.
x/accounts/defaults/lockup/periodic_locking_account_test.go (1)
218-252: LGTM! Comprehensive test for unauthorized coin sending.The test effectively validates that unauthorized users cannot send coins even when tokens are fully unlocked. Good test structure with clear setup, time manipulation, and assertions.
x/accounts/defaults/lockup/lockup.go (2)
132-133: LGTM! Improved sender validation.The change to use
accountstd.Sender(ctx)and pass bytes directly tocheckSenderis more efficient and aligns with the PR objectives.
330-340: LGTM! Improved efficiency in sender validation.The change to use byte comparison directly instead of string conversion is more efficient and reduces unnecessary allocations.
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
x/accounts/defaults/lockup/lockup.go (1)
429-489: Consider breaking down complex methods into smaller functions.The
TrackDelegationandTrackUndelegationmethods, while well-documented, are quite complex. Consider extracting the computation logic (e.g., the X and Y calculations) into separate helper functions to improve readability and maintainability.Example refactor for TrackDelegation:
+func computeDelegationAmounts(lockedAmt, delLockingAmt, delAmt math.Int) (x, y math.Int) { + // compute x and y per the specification, where: + // X := min(max(V - DV, 0), D) + // Y := D - X + x = math.MinInt(math.MaxInt(lockedAmt.Sub(delLockingAmt), math.ZeroInt()), delAmt) + y = delAmt.Sub(x) + return x, y +} func (bva *BaseLockup) TrackDelegation( ctx context.Context, balance, lockedCoins, amount sdk.Coins, ) error { // ... existing validation code ... lockedAmt := lockedCoins.AmountOf(bondDenom) delLockingAmt, err := bva.DelegatedLocking.Get(ctx, bondDenom) if err != nil { return err } delFreeAmt, err := bva.DelegatedFree.Get(ctx, bondDenom) if err != nil { return err } - // compute x and y per the specification, where: - // X := min(max(V - DV, 0), D) - // Y := D - X - x := math.MinInt(math.MaxInt(lockedAmt.Sub(delLockingAmt), math.ZeroInt()), delAmt) - y := delAmt.Sub(x) + x, y := computeDelegationAmounts(lockedAmt, delLockingAmt, delAmt) // ... rest of the method }Also applies to: 491-547
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tests/integration/v2/accounts/lockup/continous_lockup_test_suite.go(0 hunks)tests/integration/v2/accounts/lockup/delayed_lockup_test_suite.go(0 hunks)tests/integration/v2/accounts/lockup/periodic_lockup_test_suite.go(0 hunks)tests/integration/v2/accounts/lockup/permanent_lockup_test_suite.go(0 hunks)x/accounts/defaults/lockup/lockup.go(5 hunks)
💤 Files with no reviewable changes (4)
- tests/integration/v2/accounts/lockup/continous_lockup_test_suite.go
- tests/integration/v2/accounts/lockup/permanent_lockup_test_suite.go
- tests/integration/v2/accounts/lockup/delayed_lockup_test_suite.go
- tests/integration/v2/accounts/lockup/periodic_lockup_test_suite.go
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.go`: Review the Golang code for conformity with the Ub...
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/accounts/defaults/lockup/lockup.go
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Analyze
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: test-system-v2
- GitHub Check: build (arm64)
- GitHub Check: Summary
🔇 Additional comments (2)
x/accounts/defaults/lockup/lockup.go (2)
131-132: LGTM! Consistent sender retrieval from context.The changes consistently retrieve the sender from context instead of message across all methods, which aligns with the module's design principles.
Also applies to: 187-188, 260-261, 288-289
328-338: LGTM! Optimized sender verification.The method now efficiently compares byte slices directly without string conversion, which is a performance improvement.
Co-authored-by: Alex | Interchain Labs <[email protected]> (cherry picked from commit 5cedd50) # Conflicts: # api/cosmos/accounts/defaults/lockup/v1/tx.pulsar.go # x/accounts/defaults/lockup/continuous_locking_account_test.go # x/accounts/defaults/lockup/periodic_locking_account_test.go # x/accounts/defaults/lockup/permanent_locking_account_test.go
Description
The lockup accounts were not following the design of the accounts module instead like the sdk.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit