Page MenuHomePhabricator

Introduce wrappers around CBitcoinAddress

Authored by dagurval on Sep 13 2017, 13:35.



This patch removes the need for the intermediary Base58 type
CBitcoinAddress, by providing {Encode,Decode,IsValid}Destination
function that directly operate on the conversion between strings
and CTxDestination.

This is a port of

Depends on D539

Test Plan

make check, rpc tests, manual QT testing

Diff Detail

rABC Bitcoin ABC
Lint Not Applicable
Tests Not Applicable

Event Timeline

I have not manually tested the QT changes yet, as I need to fix the Ubuntu system I have for GUI testing - it does not want to boot.

deadalnix requested changes to this revision.Sep 14 2017, 17:14
deadalnix added a subscriber: deadalnix.
deadalnix added inline comments.
211 ↗(On Diff #1369)

While we are at it, could you put the /** on its own line ?

340 ↗(On Diff #1369)

Use braces or a ternary.

449 ↗(On Diff #1369)

Looks like this is a repeating pattern, this screams refactoring.

233 ↗(On Diff #1369)

Keep comments on their own lines.

749 ↗(On Diff #1369)

Keep braces.

118 ↗(On Diff #1369)

use uint8_t

230 ↗(On Diff #1369)

The else only contains that if, so using else if seems more appropriate.

557 ↗(On Diff #1369)

Put the whole comment on one line and let clang-format do its magic.

254 ↗(On Diff #1369)

Use std::shared_ptr

171 ↗(On Diff #1369)

Put comment on its own line.

This revision now requires changes to proceed.Sep 14 2017, 17:14
dagurval edited edge metadata.
dagurval marked 9 inline comments as done.

Addressed comments and rebased on top of D539

Thanks for the review. I rebased and addressed all comments except the one pattern screaming for refactoring, I consider it out of scope for this diff.

deadalnix requested changes to this revision.Sep 21 2017, 19:34

Modulo a few nits, LGTM.

836 ↗(On Diff #1390)

Put the comment withing the braces.

840 ↗(On Diff #1390)


This revision now requires changes to proceed.Sep 21 2017, 19:34
dagurval edited edge metadata.
dagurval marked an inline comment as done.


This revision is now accepted and ready to land.Sep 23 2017, 19:28
Closed by commit rABC831a13bf63c3: Introduce wrappers around CBitcoinAddress (authored by Pieter Wuille <>, committed by dagurval). · Explain WhySep 23 2017, 21:13
This revision was automatically updated to reflect the committed changes.