-
Notifications
You must be signed in to change notification settings - Fork 716
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
[Wallet] Address book encapsulation. #1819
[Wallet] Address book encapsulation. #1819
Conversation
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.
Pretty nice. utACK f97ed1c
Needs rebase |
f97ed1c
to
264d331
Compare
rebased |
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.
re-utACK 264d331
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.
Found a bug with the addressbook iterator-based while loops.
264d331
to
17b57b9
Compare
updated following the feedback 👌 |
c659294
to
735c169
Compare
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.
Sorry for multiple reviews... but have found something else, about the recursive locking being introduced here. See inline comments (there could be other instances too).
735c169
to
8b5eaf7
Compare
updated solving all of the comments 👍 |
8b5eaf7
to
01bf9ff
Compare
01bf9ff
to
6fb8f1f
Compare
…meForAddressBookEntry(dest)
… loop to use it. refactor: move remaining mapAddressBook direct access to GetAddressBookXXX encapsulation methods.
6fb8f1f
to
e0ffc98
Compare
rebased on master due conflicts with recently merged #1853 plus cleaned a previous refactor that wasn't needed anymore in Ready for review. |
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.
There is still some recursive locking (e.g. HasAddressBook
or GetNameForAddressBookEntry
), but those can be fixed later.
Same as a couple comments left inline.
ACK e0ffc98
e0ffc98
to
f7e997f
Compare
Done, updated the two lines changes. A quick git diff and can be re acked. |
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.
re-utACK f7e997f
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.
utACK f7e997f
Essentially, have restricted
mapAddressbook
access to the wallet class only. Migrating every direct access to the correspondent getter, setter and iterator wrapper. Plus, solved a todo inAddressTableModel::getAddressToShow
(df262e4).It's a preparation for the shielded addresses addressbook support.