Skip to content

[Refactor] Wallet db keys enum + old CWalletKey removal. #2253

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.

Sign up for GitHub

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

Jump to bottom
Merged
random-zebra merged 3 commits into from
Apr 10, 2021
Merged

[Refactor] Wallet db keys enum + old CWalletKey removal. #2253

random-zebra merged 3 commits into from
Apr 10, 2021

Conversation

@furszy
Copy link

@furszy furszy commented Mar 18, 2021

Making the wallet db nicer to work with, grouping all the keys in a single spot.
Also, removed CWalletKey that was never supported in the whole project history.

@furszy furszy self-assigned this Mar 18, 2021
@random-zebra random-zebra added this to the 6.0.0 milestone Mar 25, 2021
random-zebra
random-zebra reviewed Mar 26, 2021
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

concept/code ACK. Would be nice to script this change, as checking the lines one by one is a bit tedious.

@furszy furszy force-pushed the 2020_walletdb_keys_enum branch from 32f9f82 to 995fe6c Compare March 30, 2021 13:00
random-zebra
random-zebra previously approved these changes Mar 30, 2021
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

ACK 995fe6c9645ffd216cd29640b39f2576ba124549

@random-zebra random-zebra requested a review from Fuzzbawls April 8, 2021 13:32
@random-zebra
Copy link

random-zebra commented Apr 8, 2021

Can we merge this one?
Quite simple, and it has been submitted 3 weeks ago.

Fuzzbawls
Fuzzbawls reviewed Apr 9, 2021
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

overall good. a minor nit commented inline.

Also, while we're currently never calling WriteMultiSig/EraseMultiSig from anywhere, could add it to DBKeys anyways for completeness.

furszy added 2 commits April 9, 2021 15:06
@furszy
wallet: Enumerate walletdb keys
1c1bcb4
@furszy
wallet: remove, inherited, never used CWalletKey class.
583be69 
Initially used for upstream's wallet keys and got deprecated in 2013, only leaving the db deserialization. PIVX never created this type of keys in db.
@furszy furszy force-pushed the 2020_walletdb_keys_enum branch from 995fe6c to cccc13d Compare April 9, 2021 18:11
@furszy furszy force-pushed the 2020_walletdb_keys_enum branch from cccc13d to f7e8e1e Compare April 9, 2021 18:15
@furszy
Copy link
Author

furszy commented Apr 9, 2021

Updated per feedback.
...

Also, while we're currently never calling WriteMultiSig/EraseMultiSig from anywhere, could add it to DBKeys anyways for completeness.

Removed the two WriteMultiSig and EraseMultiSig functions.
Those were just remnants of the previous bad implementation that we missed to remove in #1662. Plus well, the client already has a process to store multisig scripts using CWallet::AddCScript.
So.. good, extra unneeded stuff cleaned.

Fuzzbawls
Fuzzbawls approved these changes Apr 10, 2021
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK f7e8e1e

random-zebra
random-zebra approved these changes Apr 10, 2021
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

utACK f7e8e1e and merging...

@random-zebra random-zebra merged commit 630ec7a into PIVX-Project:master Apr 10, 2021
@random-zebra random-zebra mentioned this pull request Apr 11, 2021
Merged
@furszy furszy deleted the 2020_walletdb_keys_enum branch November 29, 2022 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

+1 more reviewer

@random-zebra random-zebra random-zebra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@furszy furszy

Labels

Refactoring Wallet

Projects

None yet

Milestone

5.3.0

Development

Successfully merging this pull request may close these issues.

3 participants

@furszy @random-zebra @Fuzzbawls