[TRA-185] keep track of vault shares as integers #1267
Conversation
WalkthroughThe recent updates aim to streamline vault operations by 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: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/vault/types/vault.pb.gois excluded by!**/*.pb.go
Files selected for processing (11)
- protocol/x/vault/abci.go (1 hunks)
- protocol/x/vault/keeper/msg_server_deposit_to_vault_test.go (10 hunks)
- protocol/x/vault/keeper/orders.go (1 hunks)
- protocol/x/vault/keeper/orders_test.go (2 hunks)
- protocol/x/vault/keeper/shares.go (6 hunks)
- protocol/x/vault/keeper/shares_test.go (12 hunks)
- protocol/x/vault/keeper/vault.go (1 hunks)
- protocol/x/vault/keeper/vault_test.go (1 hunks)
- protocol/x/vault/module.go (2 hunks)
- protocol/x/vault/types/num_shares.go (1 hunks)
- protocol/x/vault/types/num_shares_test.go (1 hunks)
Path instructions used (11)
protocol/x/vault/types/num_shares.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/abci.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/types/num_shares_test.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/vault.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/vault_test.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/shares.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/module.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/orders.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/msg_server_deposit_to_vault_test.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/shares_test.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.protocol/x/vault/keeper/orders_test.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Additional comments (23)
protocol/x/vault/types/num_shares.go (1)
- 9-15: The implementation of
BigIntToNumSharesfunction is correct and handles the conversion frombig.InttoNumSharesefficiently, including the nil check for the input parameter.protocol/x/vault/abci.go (1)
- 8-13: The
BeginBlockerfunction is correctly implemented to trigger the decommissioning of vaults at the start of a block using theDecommissionVaultsmethod on the keeper.protocol/x/vault/types/num_shares_test.go (1)
- 12-31: The test function
TestBigIntToNumSharesis correctly updated to cover the functionality of theBigIntToNumSharesfunction, including the handling of nil inputs.protocol/x/vault/keeper/vault.go (3)
- 14-28: The
GetVaultEquityfunction is correctly implemented to calculate the equity of a vault using thesubaccountsKeeper. Error handling and the logic for returning the net collateral are appropriately done.- 31-63: The
DecommissionVaultsfunction is correctly implemented to decommission vaults with positive shares and non-positive equity. The logic for iterating through vaults, checking conditions, and handling errors is appropriately done.- 66-82: The
DecommissionVaultfunction is correctly implemented to decommission a specific vault by deleting its total shares and all owner shares. The logic for accessing stores and iterating through owner shares for deletion is appropriately done.protocol/x/vault/keeper/vault_test.go (2)
- 17-90: The
TestDecomissionVaultsfunction correctly tests the decommissioning logic for vaults with positive equity and those without. The setup, assertions, and verification of outcomes are appropriately implemented.- 92-134: The
TestDecomissionVaultfunction correctly tests the decommissioning of a single vault, ensuring that its total shares and owner shares are deleted as expected. The setup and verification of outcomes are appropriately done.protocol/x/vault/keeper/shares.go (3)
- 42-50: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [36-47]
The
SetTotalSharesfunction correctly implements the logic for setting total shares for a vault, including the check for negative shares. Note that convertingbig.Inttofloat32for metrics might lead to precision loss for very large numbers, but this is likely acceptable in the context of metrics.
- 84-84: The
SetOwnerSharesfunction is correctly implemented to set owner shares for an owner in a vault, including the check for negative shares.- 111-137: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [114-180]
The
MintSharesfunction correctly implements the logic for minting shares based on quantums to deposit, including checks for positive deposit amounts and non-positive equity. The calculations for shares to mint and updates to total and owner shares are appropriately done.protocol/x/vault/module.go (1)
- 151-158: The
BeginBlockfunction is correctly implemented to execute ABCI BeginBlock logic for the vault module, including the unwrapping of the SDK context and the call toBeginBlocker.protocol/x/vault/keeper/orders.go (1)
- 50-50: The modification in the
RefreshAllVaultOrdersfunction to directly access theNumSharesfield for checking ifTotalSharesis non-positive simplifies the logic and is correctly implemented.protocol/x/vault/keeper/msg_server_deposit_to_vault_test.go (4)
- 35-35: The transition from
big.Rattobig.IntforexpectedOwnerSharesinDepositInstancealigns with the PR's objective of using integers for vault shares. This change simplifies the representation and operations on shares.- 58-58: Switching
totalSharesHistoryto usebig.Intinstead ofbig.Ratis consistent with the PR's goal. This modification ensures that the test cases accurately reflect the new integer-based representation of vault shares.- 332-332: The use of
vaulttypes.BigIntToNumSharesfor convertingbig.Intto the internal shares representation is correct and necessary due to the transition to integer-based shares. This ensures that the test validations are performed against the expected types.- 343-343: Correctly using
vaulttypes.BigIntToNumSharesfor convertingexpectedOwnerSharesto the internal representation for comparison in tests. This change is essential for maintaining the integrity of the test cases after the transition to integer-based shares.protocol/x/vault/keeper/shares_test.go (5)
- 26-27: Switching to
big.Intfor representing shares inTestGetSetTotalSharesis consistent with the PR's goal of using integers for vault shares. This change simplifies the representation and operations on shares.- 67-68: Attempting to set negative shares should result in an error, as shares cannot be negative. This test case correctly validates the error handling for such a scenario, ensuring the robustness of the shares management logic.
- 94-95: The transition to
big.IntfornumSharesinTestGetSetOwnerSharesaligns with the PR's objective. This ensures that the test cases accurately reflect the new integer-based representation of vault shares.- 125-126: Validating the error handling for setting negative owner shares is crucial. This test case ensures that the system correctly prevents negative shares, aligning with the logical constraints of share management.
- 143-155: The update to use
big.Intfor representing shares in theTestMintSharessetup and expectations is consistent with the PR's objectives. This change simplifies the representation and operations on shares, making the test cases more straightforward.protocol/x/vault/keeper/orders_test.go (1)
- 33-33: Switching to
big.Intfor representingtotalSharesin the test setup forTestRefreshAllVaultOrdersaligns with the PR's objective of using integers for vault shares. This ensures that the test cases accurately reflect the new integer-based representation of vault shares.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/vault/types/vault.pb.gois excluded by!**/*.pb.go
Files selected for processing (13)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/vault.ts (4 hunks)
- proto/dydxprotocol/vault/vault.proto (1 hunks)
- protocol/x/vault/abci.go (1 hunks)
- protocol/x/vault/keeper/msg_server_deposit_to_vault_test.go (10 hunks)
- protocol/x/vault/keeper/orders.go (1 hunks)
- protocol/x/vault/keeper/orders_test.go (2 hunks)
- protocol/x/vault/keeper/shares.go (6 hunks)
- protocol/x/vault/keeper/shares_test.go (12 hunks)
- protocol/x/vault/keeper/vault.go (1 hunks)
- protocol/x/vault/keeper/vault_test.go (1 hunks)
- protocol/x/vault/module.go (2 hunks)
- protocol/x/vault/types/num_shares.go (1 hunks)
- protocol/x/vault/types/num_shares_test.go (1 hunks)
Path instructions used (2)
proto/dydxprotocol/vault/vault.proto (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/vault.ts (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Files skipped from review as they are similar to previous changes (11)
- protocol/x/vault/abci.go
- protocol/x/vault/keeper/msg_server_deposit_to_vault_test.go
- protocol/x/vault/keeper/orders.go
- protocol/x/vault/keeper/orders_test.go
- protocol/x/vault/keeper/shares.go
- protocol/x/vault/keeper/shares_test.go
- protocol/x/vault/keeper/vault.go
- protocol/x/vault/keeper/vault_test.go
- protocol/x/vault/module.go
- protocol/x/vault/types/num_shares.go
- protocol/x/vault/types/num_shares_test.go
Additional comments (3)
proto/dydxprotocol/vault/vault.proto (1)
- 26-29: The transition to using a single
bytesfield for representing shares in theNumSharesmessage aligns well with the PR's objectives of moving from rational numbers to integers. It's a good practice to document the expected format and limitations (if any) of thenum_sharesfield, especially since it uses a custom typeSerializableInt. This will help developers understand how to interact with this field correctly.indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/vault.ts (2)
- 70-80: The simplification of the
NumSharesandNumSharesSDKTypeinterfaces to use a singlenumSharesfield is consistent with the changes in the protobuf definition and the PR's objectives. It would be beneficial to add documentation explaining the format and usage of thenumSharesfield, especially for developers unfamiliar with handlingUint8Arrayfor representing large integers.- 137-150: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [140-177]
The adjustments made to the encoding and decoding functions for the
NumSharesinterface are appropriate and necessary for handling thenumSharesfield as aUint8Array. These changes ensure that the binary serialization and deserialization processes align with the updated interface definition and the overall objectives of the PR.
protocol/x/vault/abci.go
Outdated
| @@ -5,6 +5,13 @@ import ( | |||
| "github.com/dydxprotocol/v4-chain/protocol/x/vault/keeper" | |||
| ) | |||
|
|
|||
| func BeginBlocker( | |||
There was a problem hiding this comment.
i dont think this belongs
protocol/x/vault/keeper/vault.go
Outdated
| return netCollateral, nil | ||
| } | ||
|
|
||
| // DecommissionVaults decommissions all vaults with positive shares and non-positive equity. |
There was a problem hiding this comment.
this shouldnt be here
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
protocol/x/vault/types/vault.pb.gois excluded by!**/*.pb.go
Files selected for processing (11)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/vault.ts (4 hunks)
- proto/dydxprotocol/vault/vault.proto (1 hunks)
- protocol/x/vault/keeper/msg_server_deposit_to_vault_test.go (10 hunks)
- protocol/x/vault/keeper/orders.go (1 hunks)
- protocol/x/vault/keeper/orders_test.go (2 hunks)
- protocol/x/vault/keeper/shares.go (6 hunks)
- protocol/x/vault/keeper/shares_test.go (12 hunks)
- protocol/x/vault/keeper/vault.go (1 hunks)
- protocol/x/vault/keeper/vault_test.go (6 hunks)
- protocol/x/vault/types/num_shares.go (1 hunks)
- protocol/x/vault/types/num_shares_test.go (1 hunks)
Path instructions used (1)
protocol/x/vault/keeper/vault_test.go (1)
/
Do not include a walkthrough. For all comments on line changes that are longer than 15 lines, add a prefix to the comment of what lines you are commenting on (for example: "The changes from lines 100 to 200...". Add your comment to be on the last 15 lines of the chunk rather than adding your comment to the entire chunk.
Files skipped from review as they are similar to previous changes (10)
- indexer/packages/v4-protos/src/codegen/dydxprotocol/vault/vault.ts
- proto/dydxprotocol/vault/vault.proto
- protocol/x/vault/keeper/msg_server_deposit_to_vault_test.go
- protocol/x/vault/keeper/orders.go
- protocol/x/vault/keeper/orders_test.go
- protocol/x/vault/keeper/shares.go
- protocol/x/vault/keeper/shares_test.go
- protocol/x/vault/keeper/vault.go
- protocol/x/vault/types/num_shares.go
- protocol/x/vault/types/num_shares_test.go
Additional comments (4)
protocol/x/vault/keeper/vault_test.go (4)
- 23-23: The change from
*big.Ratto*big.IntfortotalSharesaligns with the PR's objective to represent vault shares as integers. This is a crucial change for consistency across the protocol.- 51-59: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [36-74]
The initialization of
totalShareswith*big.Intin the test cases ensures that the tests are now compatible with the updated data type for vault shares. This change is necessary for the tests to reflect the new implementation accurately.
- 125-132: The usage of
vaulttypes.BigIntToNumSharesfor converting*big.Intto the protocol's share type is correct and ensures that the tests are aligned with the new integer representation of shares. This is an essential part of integrating the changes into the existing test suite.- 189-190: The creation of shares using
vaulttypes.BigIntToNumSharesinTestDecommissionVaultis consistent with the changes made inTestDecommissionNonPositiveEquityVaults. This consistency is key to ensuring that all parts of the test suite are updated to work with the new integer representation of shares.
Changelist
keep track of vault shares as integers (instead of rational numbers)
Test Plan
unit tests
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.