[TRA-116] add x/vault total shares store #1179
Conversation
WalkthroughThe recent update introduces and enhances vault management functionalities within a blockchain protocol. It defines constants for vault types, adds and tests methods for managing and tracking the total shares of vaults, and improves documentation. Key developments include the introduction of a new constant for share management, the ability to convert vault IDs to state keys for easier access, and a systematic enhancement of the protocol's vault handling capabilities. 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: 1
Configuration used: CodeRabbit UI
Files selected for processing (7)
- protocol/testutil/constants/vault.go (1 hunks)
- protocol/x/vault/keeper/shares.go (1 hunks)
- protocol/x/vault/keeper/shares_test.go (1 hunks)
- protocol/x/vault/types/keys.go (1 hunks)
- protocol/x/vault/types/keys_test.go (1 hunks)
- protocol/x/vault/types/vault_id.go (1 hunks)
- protocol/x/vault/types/vault_id_test.go (1 hunks)
Additional comments: 6
protocol/x/vault/types/keys.go (1)
- 3-16: The addition of
TotalSharesKeyPrefixand the comments for clarity are well-implemented. These changes enhance the readability and maintainability of the code.protocol/testutil/constants/vault.go (1)
- 1-16: The constants
Vault_Clob_0andVault_Clob_1are defined correctly, using appropriate types and following a consistent naming convention. These changes facilitate testing and implementation of vault-related features.protocol/x/vault/types/keys_test.go (1)
- 1-17: The tests introduced for verifying the correctness of module and state keys are well-structured and follow best practices for unit testing in Go. These tests ensure the reliability and correctness of the constants defined in the code.
protocol/x/vault/types/vault_id_test.go (1)
- 1-16: The tests for the
ToStateKeymethod are correctly implemented, ensuring that the method behaves as expected. However, if theToStateKeymethod is updated to return an error (as suggested in the review ofvault_id.go), these tests will need to be updated accordingly to handle the error.protocol/x/vault/keeper/shares.go (1)
- 9-34: The
GetTotalSharesandSetTotalSharesmethods are correctly implemented, providing a robust mechanism for managing total shares of a vault. The use ofprefix.NewStoreand the codec for marshaling and unmarshaling data are appropriate and align with best practices for interacting with the store in Cosmos SDK modules.protocol/x/vault/keeper/shares_test.go (1)
- 1-45: The tests for the getter and setter methods for total shares of a vault are well-structured and comprehensive, covering a variety of scenarios to ensure the correctness and reliability of the implementation. These tests are crucial for maintaining the robustness of the vault functionality within the protocol.
| var ( | ||
| Vault_Clob_0 = vaulttypes.VaultId{ | ||
| Type: vaulttypes.VaultType_VAULT_TYPE_CLOB, | ||
| Number: 0, |
There was a problem hiding this comment.
why is this field named Number instead of like Id?
There was a problem hiding this comment.
I think Id is a bit confusing as VaultId already has Id in the name. For example, see subaccountId
|
|
||
| b := store.Get(vaultId.ToStateKey()) | ||
| if b == nil { | ||
| return val, false |
There was a problem hiding this comment.
how is val being returned here without being initialized? Is val being in the return of this function also initialize it? Does that also mean exists is initialized?
There was a problem hiding this comment.
Yes, that's correct. See here for more details.
| ctx sdk.Context, | ||
| vaultId types.VaultId, | ||
| ) (val types.NumShares, exists bool) { | ||
| store := prefix.NewStore(ctx.KVStore(k.storeKey), []byte(types.TotalSharesKeyPrefix)) |
There was a problem hiding this comment.
This is finding the existing keyvalue store that is associated with this keeper? Why do we use this instead of just storing this KV store directly in the keeper in x/vault/keeper.go?
There was a problem hiding this comment.
We usually create a separate store (within KVStore) for each "functionality"
| } | ||
|
|
||
| // SetTotalShares sets TotalShares for a vault. | ||
| func (k Keeper) SetTotalShares( |
There was a problem hiding this comment.
I wonder if instead of having SetTotalShares, we should have incrementTotalShares, decrementTotalShares, or resetTotalShares(setting total shares to 0). It seems like we will never set totalShares to an arbitrary number, so SetTotalShares would require additional arithmetic.
nit: Should we panic on totalShares being a negative number?
There was a problem hiding this comment.
Yeah we can build these increment decrement on top of Get and Set for sure.
As for verifying whether totalShares is non-negative, perhaps we can add that later when building out MsgDepositToVault?
There was a problem hiding this comment.
chatted with Vincent offline about this check on negative. will change this to return error if negative
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- protocol/x/vault/keeper/shares.go (1 hunks)
- protocol/x/vault/keeper/shares_test.go (1 hunks)
- protocol/x/vault/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- protocol/x/vault/keeper/shares.go
- protocol/x/vault/keeper/shares_test.go
Additional comments: 1
protocol/x/vault/types/errors.go (1)
- 7-12: The addition of
ErrNegativeSharesis well-defined and follows best practices for error handling within Cosmos SDK modules. The error message is clear and appropriately describes the condition being checked. Ensure that the error code1aligns with the existing error code sequence within the module to avoid conflicts.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- protocol/x/vault/keeper/shares_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/shares_test.go
Changelist
Add
TotalSharesstore (and getter and setter methods)Test Plan
added unit tests in this PR
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.