[TRA-115] decommission vaults at the beginning of a block #1264
Conversation
|
Warning Rate Limit Exceeded@tqin7 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 55 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent updates streamline vault management by automating vault decommissioning at each block's start using the new 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 selected for processing (4)
- protocol/x/vault/abci.go (1 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)
Additional comments: 7
protocol/x/vault/abci.go (1)
- 8-13: The implementation of
BeginBlockeris concise and follows the expected pattern for Cosmos SDK modules. It's crucial to ensure that theDecommissionVaultsmethod in the keeper is thoroughly tested, given its impact on the system state at the beginning of each block.protocol/x/vault/keeper/vault.go (3)
- 14-28: Consider adding documentation to the
GetVaultEquitymethod to explain its purpose and the meaning of the returned equity value. This can improve code readability and maintainability.- 31-64: While the logic for decommissioning vaults based on their equity is sound, consider refining the error handling strategy. Instead of logging errors directly within the loop, it might be beneficial to aggregate errors and handle them after the loop completes. Additionally, assess the performance implications of iterating through all vaults, especially in scenarios with a large number of vaults.
- 67-83: The
DecommissionVaultmethod is implemented correctly. Consider adding documentation to explain its purpose, especially the implications of deleting total and owner shares for a vault. This can aid in understanding the method's impact on the system state.protocol/x/vault/keeper/vault_test.go (2)
- 17-90: The test
TestDecomissionVaultseffectively covers the decommissioning logic. However, consider correcting the typo in the test name toTestDecommissionVaults. Additionally, adding more detailed comments explaining the test setup and the expected outcomes would enhance readability and maintainability.- 92-134: Similar to the previous comment, correct the typo in
TestDecomissionVaulttoTestDecommissionVault. Enhancing the comments to explain the test setup and assertions in more detail would also be beneficial for future maintainability.protocol/x/vault/module.go (1)
- 151-158: The implementation of the
BeginBlockmethod is correct and follows the expected pattern for integrating ABCI BeginBlock logic in Cosmos SDK modules. Ensure that telemetry is correctly configured to capture the metrics as intended.
tqin7
changed the title
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (4)
- protocol/x/vault/abci.go (1 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)
Files skipped from review as they are similar to previous changes (4)
- protocol/x/vault/abci.go
- protocol/x/vault/keeper/vault.go
- protocol/x/vault/keeper/vault_test.go
- protocol/x/vault/module.go
protocol/x/vault/keeper/vault.go
Outdated
| } | ||
|
|
||
| // DecommissionVaults decommissions all vaults with positive shares and non-positive equity. | ||
| func (k Keeper) DecommissionVaults( |
There was a problem hiding this comment.
nit: can we clarify this function name? It sounds like DecomissionVaults would decomission all of the vaults, maybe DecommissionInvalidVaults
protocol/x/vault/keeper/vault.go
Outdated
| } | ||
| } | ||
|
|
||
| // DecommissionVault decommissions a vault by deleting its tota shares and owner shares. |
There was a problem hiding this comment.
| // DecommissionVault decommissions a vault by deleting its tota shares and owner shares. | |
| // DecommissionVault decommissions a vault by deleting its total shares and owner shares. |
| // Skip if TotalShares is non-positive. | ||
| totalSharesRat, err := totalShares.ToBigRat() | ||
| if err != nil || totalSharesRat.Sign() <= 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
is it positive for totalShares to be negative? How could that happen?
There was a problem hiding this comment.
negative shouldn't happen, but just added this check here to be future-proof
There was a problem hiding this comment.
Curious, will this have to be updated when withdrawal logic is added? It's possible for a vault to have 0 shares and 0 equity in that case right? Non-blocking as that isn't part of Phase 1.
There was a problem hiding this comment.
yeah 0 shares is possible. will think about that
| } | ||
|
|
||
| // DecommissionVault decommissions a vault by deleting its tota shares and owner shares. | ||
| func (k Keeper) DecommissionVault( |
There was a problem hiding this comment.
iiuc, if a user attempts to deposit in this vault before it's decommissioned, it will be recreated in the same block its decommissioned and a new vault will be created right?
There was a problem hiding this comment.
Is that the case? If a user deposits to the vault before begin block (when decomissioning occurs), then it would have happened in the previous block. What re-creates it in the new block?
There was a problem hiding this comment.
deposit tx is processed after begin block. so if a vault is ready to be decommissioned, begin block would decommission it and then when deposit tx is processed, it will be created again.
if a user deposits before begin block, then that deposit tx is processed in the previous block
| // Check that total shares and owner shares are not deleted for vault 0. | ||
| got, exists := k.GetTotalShares(ctx, vault0) | ||
| require.Equal(t, true, exists) | ||
| require.Equal(t, shares, got) | ||
| got, exists = k.GetOwnerShares(ctx, vault0, constants.Alice_Num0.Owner) | ||
| require.Equal(t, true, exists) | ||
| require.Equal(t, shares, got) | ||
| // Check that total shares and owner shares are deleted for vault 1. | ||
| _, exists = k.GetTotalShares(ctx, vault1) | ||
| require.Equal(t, false, exists) | ||
| _, exists = k.GetOwnerShares(ctx, vault1, constants.Bob_Num0.Owner) | ||
| require.Equal(t, false, exists) |
There was a problem hiding this comment.
nit: shouldn't this be multiple separate test cases?
| // Decomission a non-existent vault. | ||
| k.DecommissionVault(ctx, constants.Vault_Clob_0) |
There was a problem hiding this comment.
what are you testing with this?
There was a problem hiding this comment.
making sure that the function doesn't panic on non-existent vaults
| @@ -0,0 +1,135 @@ | |||
| package keeper_test | |||
There was a problem hiding this comment.
non-blocking nit: Both tests here can be broken up into separate test cases w/ descriptions which would be more helpful for someone running the test if anything fails.
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/abci.go (1 hunks)
- protocol/x/vault/keeper/vault.go (1 hunks)
- protocol/x/vault/keeper/vault_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (3)
- protocol/x/vault/abci.go
- protocol/x/vault/keeper/vault.go
- protocol/x/vault/keeper/vault_test.go
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/vault_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- protocol/x/vault/keeper/vault_test.go
Changelist
in beginblock, decommission a vault if its total shares is positive and equity is non-positive
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.