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

Merge currency specific bitcoin wallets #494

Merged
merged 6 commits into from
Jul 21, 2023

Conversation

gregdhill
Copy link
Member

@gregdhill gregdhill commented Jul 18, 2023

Closes #484

Adds a new bitcoin wallet with the suffix "shared" and migrates all keys from the currency specific wallets. I went back and forth a bit on the metrics but I added a second commit which merges those that make sense. If we revert that commit the metrics are duplicated for each vault label.

Copy link
Member

@sander2 sander2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little bit worried about the fact that all vaults will now use the same rpc connection - specifically of concurrent access. I think that it'll be mostly ok, since we already were using the connection from multiple tasks, and we have the locking in place for tx creation. But please also have a think about it

vault/src/system.rs Show resolved Hide resolved
vault/src/issue.rs Outdated Show resolved Hide resolved
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Signed-off-by: Gregory Hill <gregorydhill@outlook.com>
Copy link
Member

@sander2 sander2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but it's a relatively high-risk pr, so we should definitely test this on testnet

tracing::info!("initial status: = {scanning_status:?}");
let account_id = btc_parachain.get_account_id();
let mut scanning_status = RescanStatus::get(&account_id, db)?;
tracing::info!("Scanning: {scanning_status:?}");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite accurate since scanning range is updated below based on issues

@gregdhill gregdhill merged commit 5fdc15d into interlay:master Jul 21, 2023
6 checks passed
@gregdhill gregdhill deleted the refactor/merge-wallets branch July 21, 2023 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vault wallet migration between currencies
2 participants