-
Notifications
You must be signed in to change notification settings - Fork 7
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
Support liquidated vaults in metrics #559
Support liquidated vaults in metrics #559
Conversation
pub async fn get_vault(&self, vault_id: &VaultId) -> Option<VaultData> { | ||
self.vault_data.read().await.get(vault_id).cloned() | ||
} | ||
|
||
// Get all ACTIVE vaults |
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.
I didn't want to change the name of this method to get_active_entries
since it is used in other places and wanted to minimize the modifications to the rest of the code, but we could of course.
@@ -150,11 +156,32 @@ impl VaultIdManager { | |||
self.vault_data.read().await.get(vault_id).map(|x| x.stellar_wallet.clone()) | |||
} | |||
|
|||
pub async fn get_active_vault(&self, vault_id: &VaultId) -> Option<VaultData> { |
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.
The reason I added this method is to have it into account for the future. Now, get_vault
will bring the data even if the vault is liquidated. This impacts a few processes, namely:
I think these are all processes that the liquidated vault should be able to perform anyway. This is the most critical difference about this change, simply replacing in those instances with get_active_vault
will not modify the functionality, in case this assumptions are wrong.
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.
Looks good to me 👍 nice job @gianfra-t.
So you are saying that you did not test this yet, right? Or did you test this somehow? I would like to confirm that all metrics export at least some value in both cases, liquidated or not.
Furthermore, we will not detect if the vault is liquidated while the client is running. Although it was observed that this leads to a restart of the client in production.
Regarding the new liquidated field, I think it's fine if we don't keep that updated all the time and only for each restart.
Yes I tested it with the local spacewalk testnet, registering one of the example vaults but commenting here (to avoid simulating a liquidation). With that, none of the vaults metrics were exported which makes sense since there is nothing to iterate form With this new change the metrics should show, all of them. We can later decide to filter some of them based on the I would've liked to test the restart that happens when the client runs a liquidated vault, but I have no way of reproducing it. |
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.
I tested this with the liquidated XLM vault on Amplitude. I recalled that we actually still do have a liquidated vault available, namely the old XLM vault. This is because we were not able to call the recover() extrinsic because of some very old non-resolvable redeem request so we created a separate XLM vault instead.
I can confirm that it works as expected. The metrics are properly exported also for the liquidated vault. And the 'liquidated' gauge also works. Great job @gianfra-t 🙏
That's great @ebma ! Thanks for testing, I kinda ignored Amplitude since I couldn't connect to polkadot.js and then forgot to check there. I'll merge this then! |
Issue: #548.
Description of the issue.
Right now, when we start the client pointing to a liquidated vault, the vault will never be saved into the
VaultIdManager
created on initiation (see here).The issue is that many of the metrics code relies on iterating this map to obtain the metrics from on-chain data (eg.: here or here).
Proposed workaround
Add a new field to
VaultData
to keep track of a liquidated vault, and add the vault even if the status isLiquidated
when the systems starts.Modifying the
get
methods likeget_entries
to filter by default non-liquidated vaults allows to maintain compatibility, while in metrics we fetch all vaults on theVaultIdManager
.Testing
To verify that the missing exports, I ran a new vault against the
testnet
, but momentarily patching the code such that it was not added toVaultIdManager
. When exploring the metrics, I could see that most of them are missing.Extra metric
Added a
Liquidated
metric that displays if the vault is liquidated.This takes the
liquidated
status fromVaultData
, this is currently NOT updated and is only updated upon restart. I believe the vault will not be set back to regular status since therecover
extrinsic does not emit. Also, we are not polling to check the status of it from the state which means that a liquidated vault will stay in this state (in the client) until restart.Furthermore, we will not detect if the vault is liquidated while the client is running. Although it was observed that this leads to a restart of the client in production.