Skip to content
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

add share unlocking mechanism to megavault #2169

Merged
merged 3 commits into from
Aug 29, 2024
Merged

add share unlocking mechanism to megavault #2169

merged 3 commits into from
Aug 29, 2024

Conversation

tqin7
Copy link
Contributor

@tqin7 tqin7 commented Aug 28, 2024

Changelist

  • add msg and keeper method on shares unlocking
  • delay a MsgUnlockShares when locking shares

Test Plan

unit tests

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

Summary by CodeRabbit

  • New Features

    • Introduced a new method for unlocking shares, allowing users to manage their shares based on transaction block height.
    • Added support for delayed message processing to enhance message handling capabilities within the vault module.
    • Expanded internal message processing to recognize the new MsgUnlockShares type.
    • Implemented comprehensive testing for share locking and unlocking functionalities, improving reliability.
  • Bug Fixes

    • Enhanced validation checks during the unlocking process to ensure integrity.

@tqin7 tqin7 requested a review from a team as a code owner August 28, 2024 21:44
Copy link

linear bot commented Aug 28, 2024

Copy link
Contributor

coderabbitai bot commented Aug 28, 2024

Walkthrough

The changes introduce a new feature for unlocking shares in a blockchain protocol, encompassing new message types, RPC methods, and modifications to existing structures. This includes the implementation of the UnlockShares functionality, updates to the Keeper class, and enhancements to testing utilities. The modifications facilitate delayed message processing and improve the management of share unlock operations.

Changes

Files Change Summary
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts Added unlockShares method to Msg interface and MsgClientImpl class for unlocking shares.
indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts Introduced MsgUnlockShares and MsgUnlockSharesResponse interfaces with encoding/decoding logic.
proto/dydxprotocol/vault/tx.proto Added UnlockShares RPC method to the Msg service with corresponding message types.
protocol/app/app.go Modified New function to include app.DelayMsgKeeper parameter for delayed message processing.
protocol/testutil/delaymsg/delaymsg_helpers.go Introduced FilterDelayedMsgsByType function for filtering delayed messages by type.
protocol/testutil/keeper/vault.go Updated createVaultKeeper function to include &mocks.DelayMsgKeeper{} for testing delayed message handling.
protocol/x/vault/keeper/keeper.go Added delayMsgKeeper field to Keeper struct and updated NewKeeper function to accept this parameter.
protocol/x/vault/keeper/msg_server_unlock_shares.go Implemented UnlockShares function for unlocking shares in the keeper package.
protocol/x/vault/keeper/shares.go Added UnlockShares method to Keeper for managing share unlocks and modified LockShares to integrate delayed messaging.
protocol/x/vault/keeper/shares_test.go Enhanced test cases for share locking and unlocking functionalities, including new scenarios and assertions.
protocol/x/vault/types/expected_keepers.go Introduced DelayMsgKeeper interface with DelayMessageByBlocks method for delayed message processing.
protocol/lib/ante/internal_msg.go Modified IsInternalMsg function to recognize *vault.MsgUnlockShares as an internal message type.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant MsgServer
    participant Keeper
    participant DelayMsgKeeper

    User->>MsgServer: UnlockShares(MsgUnlockShares)
    MsgServer->>Keeper: Validate authority and ownerAddress
    Keeper-->>MsgServer: Validation result
    MsgServer->>Keeper: UnlockShares(ownerAddress)
    Keeper->>DelayMsgKeeper: DelayMessageByBlocks(ctx, msg, blockDelay)
    DelayMsgKeeper-->>Keeper: Message ID
    Keeper-->>MsgServer: Unlocked shares count
    MsgServer-->>User: MsgUnlockSharesResponse
Loading

🐰 In the meadow where bunnies play,
New features hop in, brightening the day!
Unlocking shares with a flick of a paw,
Delayed messages bring joy, oh what a flaw!
With each little change, our code's a delight,
Hopping along, everything feels just right! 🌼


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Outside diff range, codebase verification and nitpick comments (1)
protocol/x/vault/keeper/keeper.go (1)

Missing delayMsgKeeper Parameter in NewKeeper Call

The delayMsgKeeper parameter is not being passed in the NewKeeper call for the vault module in protocol/app/app.go. Ensure that this parameter is correctly integrated to avoid potential issues.

  • File: protocol/app/app.go
  • Line: app.VaultKeeper = *vaultmodulekeeper.NewKeeper(
Analysis chain

Line range hint 32-43: LGTM! But verify the new parameter usage.

The updated function signature appears to be well-integrated. However, ensure that the new parameter delayMsgKeeper is passed correctly when initializing the Keeper.

The code changes are approved.

Run the following script to verify the usage of the new parameter:

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the new parameter `delayMsgKeeper`.

# Test: Search for the function calls. Expect: Correct parameter usage.
rg --type go 'NewKeeper'

Length of output: 5525


Script:

#!/bin/bash
# Description: Verify the usage of the `delayMsgKeeper` parameter in the `NewKeeper` function calls.

# Test: Search for calls to `NewKeeper` in the `protocol/x/vault/keeper/keeper.go` file.
rg --type go 'NewKeeper' protocol/x/vault/keeper/keeper.go -A 3

Length of output: 164


Script:

#!/bin/bash
# Description: Verify the usage of the `delayMsgKeeper` parameter in the `NewKeeper` function calls across the codebase.

# Test: Search for calls to `NewKeeper` across the entire codebase.
rg --type go 'NewKeeper' -A 3

Length of output: 18697


Script:

#!/bin/bash
# Description: Verify the usage of the `delayMsgKeeper` parameter in the `NewKeeper` function calls related to the `vault` module.

# Test: Search for calls to `NewKeeper` in the context of the `vault` module.
rg --type go 'vaultmodulekeeper.NewKeeper' -A 3

Length of output: 241

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between fbda06f and ed8de19.

Files ignored due to path filters (1)
  • protocol/x/vault/types/tx.pb.go is excluded by !**/*.pb.go
Files selected for processing (12)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2 hunks)
  • proto/dydxprotocol/vault/tx.proto (2 hunks)
  • protocol/app/app.go (1 hunks)
  • protocol/testutil/delaymsg/delaymsg_helpers.go (2 hunks)
  • protocol/testutil/keeper/vault.go (1 hunks)
  • protocol/x/vault/keeper/keeper.go (3 hunks)
  • protocol/x/vault/keeper/msg_server_unlock_shares.go (1 hunks)
  • protocol/x/vault/keeper/msg_server_unlock_shares_test.go (1 hunks)
  • protocol/x/vault/keeper/shares.go (2 hunks)
  • protocol/x/vault/keeper/shares_test.go (3 hunks)
  • protocol/x/vault/types/expected_keepers.go (1 hunks)
Additional comments not posted (23)
protocol/x/vault/keeper/keeper.go (1)

18-19: LGTM! But verify the new field usage.

The addition of the new field delayMsgKeeper appears to be well-integrated. However, ensure that the new field is used appropriately in the rest of the codebase.

The code changes are approved.

Run the following script to verify the usage of the new field:

Verification successful

Field delayMsgKeeper is well-integrated and used appropriately.

The delayMsgKeeper field is actively used across different modules, including vault and bridge, and is referenced in test files, indicating proper integration and testing. No issues found.

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of the new field `delayMsgKeeper`.

# Test: Search for the field usage. Expect: Appropriate usage in the codebase.
rg --type go 'delayMsgKeeper'

Length of output: 1822

protocol/testutil/keeper/vault.go (1)

57-57: LGTM!

The addition of &mocks.DelayMsgKeeper{} enhances the function's capability to include a new mock keeper for delayed message handling.

The code changes are approved.

protocol/x/vault/types/expected_keepers.go (1)

33-42: LGTM!

The DelayMsgKeeper interface is correctly defined and enhances the functionality by allowing delayed message processing.

The code changes are approved.

indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (2)

51-55: LGTM!

The unlockShares method is correctly implemented and enhances the functionality by allowing share unlocking operations.

The code changes are approved.


30-30: LGTM!

The constructor correctly binds the unlockShares method to the class instance.

The code changes are approved.

protocol/x/vault/keeper/msg_server_unlock_shares_test.go (4)

3-16: LGTM!

The imports are appropriate and necessary for the functionality being tested.

The code changes are approved.


18-51: LGTM!

The test cases are well-defined, covering both positive and negative scenarios.

The code changes are approved.


53-75: LGTM!

The test setup and execution are appropriate for the scenarios being tested.

The code changes are approved.


77-91: LGTM!

The test assertions are comprehensive and ensure the correctness of the functionality.

The code changes are approved.

proto/dydxprotocol/vault/tx.proto (3)

27-29: LGTM!

The new RPC method UnlockShares is appropriately defined and follows the existing pattern.

The code changes are approved.


88-96: LGTM!

The new message type MsgUnlockShares is appropriately defined and follows the existing pattern.

The code changes are approved.


98-102: LGTM!

The new response message type MsgUnlockSharesResponse is appropriately defined and follows the existing pattern.

The code changes are approved.

protocol/x/vault/keeper/shares.go (4)

5-11: LGTM!

The imports are appropriate and necessary for the functionality being implemented.

The code changes are approved.


202-213: LGTM!

The modification to include a call to DelayMessageByBlocks enhances the control flow by integrating delayed messaging functionality.

The code changes are approved.


223-264: LGTM!

The new method UnlockShares is appropriately defined and follows the existing pattern. It retrieves the owner's share unlocks, processes them, and updates the state accordingly.

The code changes are approved.


230-231: LGTM!

The error handling in the UnlockShares method is comprehensive and ensures robustness.

The code changes are approved.

Also applies to: 258-260

indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (4)

107-112: LGTM!

The MsgUnlockShares interface is correctly defined with the required properties.

The code changes are approved.


115-120: LGTM!

The MsgUnlockSharesSDKType interface is correctly defined with the required properties and follows the SDK type naming conventions.

The code changes are approved.


123-126: LGTM!

The MsgUnlockSharesResponse interface is correctly defined with the optional property unlockedShares.

The code changes are approved.


129-132: LGTM!

The MsgUnlockSharesResponseSDKType interface is correctly defined with the optional property unlocked_shares and follows the SDK type naming conventions.

The code changes are approved.

protocol/x/vault/keeper/shares_test.go (2)

Line range hint 385-415: LGTM!

The enhancements to the TestLockShares function are correctly implemented and improve the test coverage by including assertions for delayed messages.

The code changes are approved.


420-578: LGTM!

The TestUnlockShares function is well-structured and covers various scenarios for unlocking shares. The assertions ensure the expected outcomes are validated.

The code changes are approved.

protocol/app/app.go (1)

1161-1161: LGTM!

The addition of app.DelayMsgKeeper to the New function is correctly implemented.

The code changes are approved.

Comment on lines +1 to +42
package keeper

import (
"context"
"fmt"

errorsmod "cosmossdk.io/errors"

govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
"github.com/dydxprotocol/v4-chain/protocol/lib"
"github.com/dydxprotocol/v4-chain/protocol/x/vault/types"
)

// UnlockShares unlocks an owner's shares.
func (k msgServer) UnlockShares(
goCtx context.Context,
msg *types.MsgUnlockShares,
) (*types.MsgUnlockSharesResponse, error) {
// Check if authority is valid.
if !k.HasAuthority(msg.Authority) {
return nil, errorsmod.Wrapf(
govtypes.ErrInvalidSigner,
"invalid authority %s",
msg.Authority,
)
}

ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName)

if msg.OwnerAddress == "" {
return nil, fmt.Errorf("owner address cannot be empty")
}

unlockedShares, err := k.Keeper.UnlockShares(ctx, msg.OwnerAddress)
if err != nil {
return nil, err
}

return &types.MsgUnlockSharesResponse{
UnlockedShares: unlockedShares,
}, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure comprehensive error handling and validation.

  1. The error message for an empty owner address could be more descriptive.
  2. Consider logging the error returned by k.Keeper.UnlockShares for better debugging.
  3. Ensure that the UnlockedShares response is correctly populated.

Apply this diff to improve error handling and validation:

 if msg.OwnerAddress == "" {
-  return nil, fmt.Errorf("owner address cannot be empty")
+  return nil, errorsmod.Wrapf(
+    govtypes.ErrInvalidAddress,
+    "owner address cannot be empty",
+  )
 }

 unlockedShares, err := k.Keeper.UnlockShares(ctx, msg.OwnerAddress)
 if err != nil {
-  return nil, err
+  k.Logger(ctx).Error("failed to unlock shares", "error", err)
+  return nil, errorsmod.Wrapf(
+    govtypes.ErrInternal,
+    "failed to unlock shares for owner %s: %v",
+    msg.OwnerAddress,
+    err,
+  )
 }

 return &types.MsgUnlockSharesResponse{
   UnlockedShares: unlockedShares,
 }, nil
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
package keeper
import (
"context"
"fmt"
errorsmod "cosmossdk.io/errors"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
"github.com/dydxprotocol/v4-chain/protocol/lib"
"github.com/dydxprotocol/v4-chain/protocol/x/vault/types"
)
// UnlockShares unlocks an owner's shares.
func (k msgServer) UnlockShares(
goCtx context.Context,
msg *types.MsgUnlockShares,
) (*types.MsgUnlockSharesResponse, error) {
// Check if authority is valid.
if !k.HasAuthority(msg.Authority) {
return nil, errorsmod.Wrapf(
govtypes.ErrInvalidSigner,
"invalid authority %s",
msg.Authority,
)
}
ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName)
if msg.OwnerAddress == "" {
return nil, fmt.Errorf("owner address cannot be empty")
}
unlockedShares, err := k.Keeper.UnlockShares(ctx, msg.OwnerAddress)
if err != nil {
return nil, err
}
return &types.MsgUnlockSharesResponse{
UnlockedShares: unlockedShares,
}, nil
}
package keeper
import (
"context"
"fmt"
errorsmod "cosmossdk.io/errors"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
"github.com/dydxprotocol/v4-chain/protocol/lib"
"github.com/dydxprotocol/v4-chain/protocol/x/vault/types"
)
// UnlockShares unlocks an owner's shares.
func (k msgServer) UnlockShares(
goCtx context.Context,
msg *types.MsgUnlockShares,
) (*types.MsgUnlockSharesResponse, error) {
// Check if authority is valid.
if !k.HasAuthority(msg.Authority) {
return nil, errorsmod.Wrapf(
govtypes.ErrInvalidSigner,
"invalid authority %s",
msg.Authority,
)
}
ctx := lib.UnwrapSDKContext(goCtx, types.ModuleName)
if msg.OwnerAddress == "" {
return nil, errorsmod.Wrapf(
govtypes.ErrInvalidAddress,
"owner address cannot be empty",
)
}
unlockedShares, err := k.Keeper.UnlockShares(ctx, msg.OwnerAddress)
if err != nil {
k.Logger(ctx).Error("failed to unlock shares", "error", err)
return nil, errorsmod.Wrapf(
govtypes.ErrInternal,
"failed to unlock shares for owner %s: %v",
msg.OwnerAddress,
err,
)
}
return &types.MsgUnlockSharesResponse{
UnlockedShares: unlockedShares,
}, nil
}

protocol/testutil/delaymsg/delaymsg_helpers.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ed8de19 and eadf66f.

Files ignored due to path filters (1)
  • protocol/x/vault/types/tx.pb.go is excluded by !**/*.pb.go
Files selected for processing (12)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2 hunks)
  • proto/dydxprotocol/vault/tx.proto (2 hunks)
  • protocol/app/app.go (1 hunks)
  • protocol/testutil/delaymsg/delaymsg_helpers.go (2 hunks)
  • protocol/testutil/keeper/vault.go (1 hunks)
  • protocol/x/vault/keeper/keeper.go (3 hunks)
  • protocol/x/vault/keeper/msg_server_unlock_shares.go (1 hunks)
  • protocol/x/vault/keeper/msg_server_unlock_shares_test.go (1 hunks)
  • protocol/x/vault/keeper/shares.go (2 hunks)
  • protocol/x/vault/keeper/shares_test.go (3 hunks)
  • protocol/x/vault/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (11)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts
  • proto/dydxprotocol/vault/tx.proto
  • protocol/app/app.go
  • protocol/testutil/delaymsg/delaymsg_helpers.go
  • protocol/testutil/keeper/vault.go
  • protocol/x/vault/keeper/keeper.go
  • protocol/x/vault/keeper/msg_server_unlock_shares.go
  • protocol/x/vault/keeper/msg_server_unlock_shares_test.go
  • protocol/x/vault/keeper/shares_test.go
  • protocol/x/vault/types/expected_keepers.go
Additional comments not posted (2)
protocol/x/vault/keeper/shares.go (2)

202-213: LGTM!

The code correctly schedules an unlock message at the specified block height.

The code changes are approved.


223-264: LGTM!

The UnlockShares method is correctly implemented, handling the retrieval, processing, and updating of share unlocks.

The code changes are approved.

@@ -2,11 +2,13 @@ package keeper

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use the E2E testing framework to add an E2E test of the flow of:

  • lock shares til block X
  • advance to block X
  • check that shares are unlocked

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this will be possible once there's a tx that calls LockShares. I was thinking once PML is integrated, we can add this e2e test. (before we have a tx that calls LockShares, e2e won't work)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added a task here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure there's a need for a tx that calls LockShares? Isn't there an instance of the VaultsKeeper that's accessible on the test app in the E2E test framework?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can call VaultsKeeper.LockShares keeper method but i don't think the state changes from that call persists once you advance to next block (as it's not part of a tx)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, possibly an E2E test with a delayed message w/ unlock shares could be useful (just to check authorities/everything else works) but not necessary.

}

res, err := ms.UnlockShares(ctx, tc.msg)
newLockedShares, exists := k.GetOwnerShareUnlocks(ctx, tc.msg.OwnerAddress)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Check newLockedShares is what's expected as well.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between eadf66f and d80a547.

Files ignored due to path filters (1)
  • protocol/x/vault/types/tx.pb.go is excluded by !**/*.pb.go
Files selected for processing (15)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (4 hunks)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (2 hunks)
  • proto/dydxprotocol/vault/tx.proto (2 hunks)
  • protocol/app/app.go (1 hunks)
  • protocol/app/msgs/all_msgs.go (1 hunks)
  • protocol/app/msgs/internal_msgs.go (1 hunks)
  • protocol/app/msgs/internal_msgs_test.go (1 hunks)
  • protocol/testutil/delaymsg/delaymsg_helpers.go (2 hunks)
  • protocol/testutil/keeper/vault.go (1 hunks)
  • protocol/x/vault/keeper/keeper.go (3 hunks)
  • protocol/x/vault/keeper/msg_server_unlock_shares.go (1 hunks)
  • protocol/x/vault/keeper/msg_server_unlock_shares_test.go (1 hunks)
  • protocol/x/vault/keeper/shares.go (2 hunks)
  • protocol/x/vault/keeper/shares_test.go (3 hunks)
  • protocol/x/vault/types/expected_keepers.go (1 hunks)
Files skipped from review as they are similar to previous changes (9)
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts
  • indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts
  • proto/dydxprotocol/vault/tx.proto
  • protocol/testutil/keeper/vault.go
  • protocol/x/vault/keeper/keeper.go
  • protocol/x/vault/keeper/msg_server_unlock_shares.go
  • protocol/x/vault/keeper/msg_server_unlock_shares_test.go
  • protocol/x/vault/keeper/shares_test.go
  • protocol/x/vault/types/expected_keepers.go
Additional comments not posted (9)
protocol/testutil/delaymsg/delaymsg_helpers.go (1)

21-35: Improve parameter naming and error handling.

  1. Consider using a more descriptive name for the delayedmsgs parameter, such as delayedMessages.
  2. Ensure that the function handles cases where GetMessage returns an error, even though require.NoError is used.

Apply this diff to improve parameter naming and error handling:

 func FilterDelayedMsgsByType[T sdk.Msg](
 	t testing.TB,
-	delayedmsgs []*delaymsgtypes.DelayedMessage,
+	delayedMessages []*delaymsgtypes.DelayedMessage,
 ) []*delaymsgtypes.DelayedMessage {
-	var filtered []*delaymsgtypes.DelayedMessage
-	for _, delayedmsg := range delayedmsgs {
-		msg, err := delayedmsg.GetMessage()
+	var filteredMessages []*delaymsgtypes.DelayedMessage
+	for _, delayedMessage := range delayedMessages {
+		msg, err := delayedMessage.GetMessage()
 		require.NoError(t, err)
 		if _, ok := msg.(T); ok {
-			filtered = append(filtered, delayedmsg)
+			filteredMessages = append(filteredMessages, delayedMessage)
 		}
 	}
-	return filtered
+	return filteredMessages
 }
protocol/app/msgs/internal_msgs_test.go (1)

155-156: LGTM!

The new message paths for unlocking shares are appropriately added.

protocol/x/vault/keeper/shares.go (2)

202-213: LGTM! But add E2E tests.

The function is correctly implemented and the new code schedules an unlock message at the specified block height. However, consider adding E2E tests to ensure the functionality.

Refer to the existing comments for more details.


223-264: LGTM!

The function is correctly implemented and handles the unlocking process efficiently.

protocol/app/msgs/internal_msgs.go (2)

199-199: LGTM!

The addition of the "/dydxprotocol.vault.MsgUnlockShares" message type is consistent with the PR objective of adding share unlocking functionality.


200-200: LGTM!

The addition of the "/dydxprotocol.vault.MsgUnlockSharesResponse" message type is consistent with the PR objective of adding share unlocking functionality.

protocol/app/msgs/all_msgs.go (2)

256-256: LGTM!

The addition of the "/dydxprotocol.vault.MsgUnlockShares" message type is consistent with the PR objective of adding share unlocking functionality.


257-257: LGTM!

The addition of the "/dydxprotocol.vault.MsgUnlockSharesResponse" message type is consistent with the PR objective of adding share unlocking functionality.

protocol/app/app.go (1)

1161-1161: LGTM! But verify the initialization and usage of app.DelayMsgKeeper.

The addition of app.DelayMsgKeeper to the New function is correctly integrated. However, ensure that this new keeper is properly initialized and used within the application.

The code changes are approved.

Run the following script to verify the initialization and usage of app.DelayMsgKeeper:

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between d80a547 and 451a07c.

Files selected for processing (1)
  • protocol/lib/ante/internal_msg.go (1 hunks)
Additional comments not posted (1)
protocol/lib/ante/internal_msg.go (1)

132-132: LGTM! But verify the usage of MsgUnlockShares in the codebase.

The addition of *vault.MsgUnlockShares to the list of recognized internal messages is correct. However, ensure that the new message type is correctly handled in other parts of the codebase.

The code changes are approved.

Run the following script to verify the usage of MsgUnlockShares:

Verification successful

MsgUnlockShares is correctly integrated into the codebase.

The MsgUnlockShares message type is well-handled across the codebase, including its definition, usage in server logic, registration in message handlers, and associated tests. This confirms that the addition of *vault.MsgUnlockShares to the list of internal messages is appropriate and should function correctly.

  • File: protocol/x/vault/types/tx.pb.go
  • File: protocol/x/vault/keeper/msg_server_unlock_shares.go
  • File: protocol/x/vault/keeper/msg_server_unlock_shares_test.go
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the usage of `MsgUnlockShares` in the codebase.

# Test: Search for the usage of `MsgUnlockShares`. Expect: Correct handling in other parts of the codebase.
rg --type go -A 5 $'MsgUnlockShares'

Length of output: 22074

@tqin7 tqin7 merged commit 6d86d0b into main Aug 29, 2024
39 checks passed
@tqin7 tqin7 deleted the tq/tra-565 branch August 29, 2024 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants