-
Notifications
You must be signed in to change notification settings - Fork 121
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
[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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
BeginBlocker
is concise and follows the expected pattern for Cosmos SDK modules. It's crucial to ensure that theDecommissionVaults
method 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
GetVaultEquity
method 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
DecommissionVault
method 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
TestDecomissionVaults
effectively 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
TestDecomissionVault
toTestDecommissionVault
. 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
BeginBlock
method 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it positive for totalShares to be negative? How could that happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
negative shouldn't happen, but just added this check here to be future-proof
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep! correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what are you testing with this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-breaking
label.indexer-postgres-breaking
label.PrepareProposal
orProcessProposal
, manually add the labelproposal-breaking
.feature:[feature-name]
.backport/[branch-name]
.refactor
,chore
,bug
.