[tra-87] add x/vault protos #1169
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates focus on enhancing the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files ignored due to path filters (2)
protocol/x/vault/types/tx.pb.gois excluded by:!**/*.pb.goprotocol/x/vault/types/vault.pb.gois excluded by:!**/*.pb.go
Files selected for processing (14)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (4 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.tx.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.rpc.msg.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/vault.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1 hunks)
- indexer/packages/v4-protos/src/codegen/google/bundle.ts (1 hunks)
- proto/dydxprotocol/vault/tx.proto (1 hunks)
- proto/dydxprotocol/vault/vault.proto (1 hunks)
- protocol/app/basic_manager/basic_manager.go (2 hunks)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/testutil/app/app.go (3 hunks)
- protocol/x/vault/keeper/msg_server_deposit_to_vault.go (1 hunks)
- protocol/x/vault/types/msg_deposit_to_vault.go (1 hunks)
Check Runs (11)
install-runsim completed (5)
v4-proto-py-gen completed (1)
v4-proto-js-gen completed (1)
protocol-gen completed (1)
lint completed (1)
indexer-gen completed (1)
check-bc-breaking completed (1)
check-sample-pregenesis-up-to-date completed (1)
build completed (1)
validate-yaml completed (1)
format completed (1)
Additional comments: 19
indexer/packages/v4-protos/src/codegen/gogoproto/bundle.ts (1)
- 1-2: The update from
_101to_102in the import statement seems appropriate. Ensure that the new version of the module is fully compatible and does not introduce any breaking changes.indexer/packages/v4-protos/src/codegen/google/bundle.ts (1)
- 1-14: The reordering and addition of imports in this file, along with the updates to the
proto/dydxprotocol/vault/vault.proto (1)
- 8-34: The definitions in
vault.proto, including theVaultTypeenum,VaultId, andNumSharesmessages, are well-structured. Ensure that the custom type used fornum_sharesis correctly implemented and thoroughly tested to maintain data integrity.proto/dydxprotocol/vault/tx.proto (1)
- 16-33: The message definitions in
tx.proto, includingMsgDepositToVaultandMsgDepositToVaultResponse, are appropriately structured for handling deposit operations. Ensure that the custom types used, especially forquote_quantums, are correctly implemented and thoroughly tested to ensure data integrity.indexer/packages/v4-protos/src/codegen/dydxprotocol/rpc.tx.ts (1)
- 20-20: The addition of the
vaultentity to thecreateRPCMsgClientfunction is a logical step to enable RPC calls for vault operations. Ensure that thevault/tx.rpc.msgmodule is correctly implemented and tested to support these operations effectively.protocol/app/basic_manager/basic_manager.go (1)
- 39-39: Adding
vaultmoduleto the list ofAppModuleBasicinstances is a crucial step in integrating the new vault functionality into the application. Ensure that thevaultmoduleis properly implemented and tested to maintain the integrity of the application.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/tx.ts (3)
- 7-15: The
MsgDepositToVaultinterface is well-defined, with clear comments and appropriate field types. Ensure thatquoteQuantumsis correctly handled in all relevant operations, especially considering its typeUint8Array.- 36-97: The encoding and decoding functions for
MsgDepositToVaultare correctly implemented, following the protobufjs conventions. It's important to ensure comprehensive testing, particularly for edge cases in the encoding and decoding processes.- 101-131: The
MsgDepositToVaultResponseinterface and its associated functions are minimal, as expected for a response type without specific fields. This simplicity is appropriate unless future requirements necessitate additional response data.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/vault.ts (5)
- 5-11: The
VaultTypeenum is correctly defined with appropriate comments. Ensure that all potential vault types are covered, and consider future extensibility for new vault types.- 54-59: The
VaultIdinterface is well-structured, providing a clear way to uniquely identify vaults. Ensure that thenumberfield is sufficient for identifying vaults within a type and consider any potential for collisions.- 72-74: The
NumSharesinterface correctly represents the number of shares in a vault. Given its use ofUint8ArrayfornumShares, ensure that there's a consistent approach to interpreting this data across the application.- 83-134: The encoding and decoding functions for
VaultIdare implemented following the protobufjs conventions. It's important to ensure these functions are thoroughly tested, especially for edge cases.- 138-179: Similarly, the encoding and decoding functions for
NumSharesfollow the correct conventions. Ensure that the handling ofUint8ArrayfornumSharesis consistent and well-documented throughout the application.indexer/packages/v4-protos/src/codegen/dydxprotocol/bundle.ts (1)
- 90-174: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [93-324]
The reordering of imports and updates to references in
bundle.tsappear to be correctly implemented, enhancing the organization within thedydxprotocolnamespace. Ensure that all references are correctly updated across the codebase to reflect these changes, avoiding any broken imports or references.protocol/app/msgs/all_msgs.go (1)
- 237-239: The addition of
"/dydxprotocol.vault.MsgDepositToVault": {},correctly aligns with the PR's objectives to introduce new functionalities related to vault operations. This change is well-placed within theAllTypeMessagesmap, ensuring that the new vault-related message type is registered and recognized by the application.However, considering the potential for future expansions in the
vaultfunctionality or other areas, it might be beneficial to consider a strategy for organizing these message types. For instance, grouping related message types or introducing comments to delineate sections could enhance readability and maintainability as the list grows.protocol/testutil/app/app.go (3)
- 66-66: The addition of
vaulttypesimport is necessary for incorporating vault functionality into the test utilities. Looks good.- 204-205: The inclusion of
vaulttypes.GenesisStatein theGenesisStatesinterface is a necessary update to handle vault-related genesis state. This change aligns well with the PR's objectives.- 259-260: Adding a case for
vaulttypes.GenesisStatein theUpdateGenesisDocWithAppStateForModulefunction is essential for updating the genesis document with vault-related state. This change is consistent and necessary for the introduction of vault functionality.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/normal_msgs.go (2 hunks)
Check Runs (18)
install-runsim completed (5)
call-build-ecs-service-vulcan / (vulcan) Check docker image build completed (1)
v4-proto-py-gen completed (1)
call-build-ecs-service-socks / (socks) Check docker image build completed (1)
v4-proto-js-gen completed (1)
call-build-ecs-service-comlink / (comlink) Check docker image build completed (1)
protocol-gen completed (1)
call-build-ecs-service-roundtable / (roundtable) Check docker image build completed (1)
lint completed (1)
call-build-ecs-service-ender / (ender) Check docker image build completed (1)
validate-yaml completed (1)
format completed (1)
golangci-lint completed (1)
check-build-bazooka completed (1)
check-bc-breaking completed (1)
check-sample-pregenesis-up-to-date completed (1)
build completed (1)
indexer-gen completed (1)
Files skipped from review as they are similar to previous changes (1)
- protocol/app/msgs/all_msgs.go
Additional comments: 2
protocol/app/msgs/normal_msgs.go (2)
- 22-22: The import statement for
vault "github.com/dydxprotocol/v4-chain/protocol/x/vault/types"has been correctly added to include the new vault types. This is necessary for the subsequent use ofvault.MsgDepositToVaultandvault.MsgDepositToVaultResponsein theNormalMsgsDydxCustommap.- 239-240: The addition of
"/dydxprotocol.vault.MsgDepositToVault": &vault.MsgDepositToVault{}and"/dydxprotocol.vault.MsgDepositToVaultResponse": nilto theNormalMsgsDydxCustommap correctly registers the new vault messages. This is essential for enabling the handling of these messages within the application. However, it's important to ensure that the corresponding message handlers are implemented and tested thoroughly to handle these new message types effectively.Please confirm that the message handlers for
MsgDepositToVaultandMsgDepositToVaultResponsehave been implemented and covered by unit tests to ensure correct behavior.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/app/msgs/all_msgs.go (1 hunks)
- protocol/app/msgs/normal_msgs.go (2 hunks)
- protocol/app/msgs/normal_msgs_test.go (1 hunks)
Check Runs (18)
install-runsim completed (5)
call-build-ecs-service-vulcan / (vulcan) Check docker image build completed (1)
call-build-ecs-service-socks / (socks) Check docker image build completed (1)
v4-proto-py-gen completed (1)
call-build-ecs-service-roundtable / (roundtable) Check docker image build completed (1)
protocol-gen completed (1)
call-build-ecs-service-comlink / (comlink) Check docker image build completed (1)
indexer-gen completed (1)
call-build-ecs-service-ender / (ender) Check docker image build completed (1)
v4-proto-js-gen completed (1)
lint completed (1)
check-build-bazooka completed (1)
validate-yaml completed (1)
format completed (1)
build completed (1)
check-bc-breaking completed (1)
golangci-lint completed (1)
check-sample-pregenesis-up-to-date completed (1)
Files skipped from review as they are similar to previous changes (2)
- protocol/app/msgs/all_msgs.go
- protocol/app/msgs/normal_msgs.go
Additional comments: 1
protocol/app/msgs/normal_msgs_test.go (1)
- 141-143: The addition of vault messages
"/dydxprotocol.vault.MsgDepositToVault"and"/dydxprotocol.vault.MsgDepositToVaultResponse"to theexpectedMsgsarray inTestNormalMsgs_Keyis correctly implemented and logically categorized.Ensure that the
NormalMsgsmap is updated to include these new messages, as this test assumes their presence.
vincentwschau
approved these changes
Mar 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changelist
add x/vault protos
Test Plan
N/A
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.