Skip to content
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

Vault wallet migration between currencies #484

Closed
sander2 opened this issue Jul 3, 2023 · 5 comments · Fixed by #494
Closed

Vault wallet migration between currencies #484

sander2 opened this issue Jul 3, 2023 · 5 comments · Fixed by #494
Assignees
Labels
enhancement New feature or request

Comments

@sander2
Copy link
Member

sander2 commented Jul 3, 2023

We'll need a companion to interlay/interbtc#1116 that migrates bitcoin wallets

@gregdhill
Copy link
Member

In the case of interlay/interbtc#1116 since we migrate the entire Vault I'm wondering if it is sufficient to simply rename the wallets? We will have a problem with re-scanning however since the issue keys will always be re-imported for the old_vault on restart. What is the advantage again for having separate wallets? I'm wondering if that was only relevant when we had theft reporting enabled. Going back to a single wallet would solve this issue and reduce some complexity.

@sander2
Copy link
Member Author

sander2 commented Jul 4, 2023

I think the primary reason we went with separate wallets was the theft checking indeed. But I guess it also helps with the (prometheus) monitoring. I think we also have a lock per wallet to ensure that tx creation is atomic with the sending, but that could be refactored. Rescanning logic would also need to be refactored to fetch issues for all vaults.

Having said that, I'm not necessarily against merging the wallets, but I'd probably ask Dom to sign off on it

@nud3l
Copy link
Member

nud3l commented Jul 5, 2023

IMO it would be good to merge wallets for the following reasons:

  1. One wallet should help with averaging out BTC fees. Technically, users pay on the upper end for redeem fees, but in extreme cases, the vault might have insufficient due to fee fluctuations. Averaging out over multiple wallets should help with that.
  2. Reducing complexity is much needed. Hopefully, this might also help move to descriptor wallets soon (see Switch to descriptor wallets #457).

Technically the downside is that vaults will need to ensure that they have enough BTC left once they withdraw some in case of liquidations: if the same account has a DOT and USDT vault with the DOT portion being liquidated, then they can withdraw parts but not all of the BTC. But I don't see this as a blocker. Withdrawing BTC is anyway a manual process.

@sander2
Copy link
Member Author

sander2 commented Jul 5, 2023

What about the monitoring though?

@nud3l nud3l moved this from New 🆕 to Todo ⏳ in Backlog Jul 10, 2023
@nud3l
Copy link
Member

nud3l commented Jul 13, 2023

I think it's fine to adjust the monitoring to sum the entire iBTC issued against the vault against the BTC holdings.

@gregdhill gregdhill self-assigned this Jul 18, 2023
@gregdhill gregdhill moved this from Todo ⏳ to Development 🏗️ in Backlog Jul 18, 2023
@github-project-automation github-project-automation bot moved this from Development 🏗️ to Done ✅ in Backlog Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants