Page MenuHomePhabricator

Introduce wrappers around CBitcoinAddress
ClosedPublic

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

Details

Summary

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 https://github.com/bitcoin/bitcoin/pull/11117

Depends on D539

Test Plan

make check, rpc tests, manual QT testing

Diff Detail

Repository
rABC Bitcoin ABC
Branch
non-base58-wrappers
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 883
Build 883: arc lint + arc unit

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.
src/base58.cpp
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.

src/bitcoin-tx.cpp
449 ↗(On Diff #1369)

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

src/qt/addresstablemodel.cpp
233 ↗(On Diff #1369)

Keep comments on their own lines.

src/qt/coincontroldialog.cpp
749 ↗(On Diff #1369)

Keep braces.

src/qt/guiutil.cpp
118 ↗(On Diff #1369)

use uint8_t

src/qt/paymentserver.cpp
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.

src/rpc/mining.cpp
254 ↗(On Diff #1369)

Use std::shared_ptr

src/test/base58_tests.cpp
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.

src/qt/sendcoinsdialog.cpp
836 ↗(On Diff #1390)

Put the comment withing the braces.

840 ↗(On Diff #1390)

dito

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

nits

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 <pieter.wuille@gmail.com>, committed by dagurval). · Explain WhySep 23 2017, 21:13
This revision was automatically updated to reflect the committed changes.