-
Notifications
You must be signed in to change notification settings - Fork 725
Prepare for non-Base58 addresses [Step 3] #1679
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
Prepare for non-Base58 addresses [Step 3] #1679
Conversation
|
Needs rebase. |
fcf29b2 to
2126cf0
Compare
|
rebased. |
|
There is still a conflict in wallet.cpp |
Coming from btc@8d0041e607a0e5bff52152f3d8af504f3703baa7
2126cf0 to
97bf8d0
Compare
|
because #1663 was merged. Rebased again. |
random-zebra
left a comment
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.
Code review ACK.
Just a minor concern about the implementation of IsValidDestination(const std::string& str) and a couple nits for the comments.
Anyway, since the version of IsValidDestination with one argument is used only twice (in paymentserver.cpp and settingsmultisendwidget.cpp, where only regular addresses should be supported), we might just get rid of it, and use directly the more readable IsValidDestinationString(str, false).
Coming from btc@1e46ebdf8618e585568ffc1b093c79cc9be07b57
97bf8d0 to
fa7e63d
Compare
|
Updated per @random-zebra's feedback |
random-zebra
left a comment
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.
about fa7e63d660b55b5a42e00d97e6ae6dec437d4f58: why force it to a particular network (MAIN), and not use the current params with Params()?
|
good catch. no real explanation for that, just me being tired. None of those flows are used anyway. |
fa7e63d to
e07286d
Compare
|
Updated with last feedback solved. |
random-zebra
left a comment
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 e07286d
Fuzzbawls
left a comment
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 e07286d
|
merging... |
Finished the complete removal of the base58 address class from the sources, migrated to the destination wrapper.
Plus includes bitcoin#11117 - last commit - and bitcoin#11259 as an extra.