Details

Reviewers

Group Reviewers

Restricted Project

Commits

rSTAGING831a13bf63c3: Introduce wrappers around CBitcoinAddress
rABC831a13bf63c3: Introduce wrappers around CBitcoinAddress

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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

dagurval created this revision.Sep 13 2017, 13:35

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 13 2017, 13:35

Harbormaster completed remote builds in B860: Diff 1368.Sep 13 2017, 13:35

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.

add missing commit

Harbormaster completed remote builds in B861: Diff 1369.Sep 13 2017, 13:40

dagurval edited the summary of this revision. (Show Details)Sep 13 2017, 13:47

dagurval added a parent revision: D539: qt: Use IsMine to validate custom change address.

dagurval added a child revision: D541: Remove duplicate destination decoding.Sep 13 2017, 17:04

deadalnix mentioned this in D539: qt: Use IsMine to validate custom change address.Sep 14 2017, 16:53

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 added a child revision: D544: Use the new cashaddr format.Sep 16 2017, 21:05

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.

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

nits

deadalnix accepted this revision.Sep 23 2017, 19:28

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 <[email protected]>, committed by dagurval). · Explain WhySep 23 2017, 21:13

This revision was automatically updated to reflect the committed changes.

nakihito mentioned this in D3278: Merge #10536: Remove unreachable or otherwise redundant code.Jun 10 2019, 22:39

Introduce wrappers around CBitcoinAddress
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1405

src/base58.h

src/base58.cpp

src/bitcoin-tx.cpp

src/core_write.cpp

src/qt/addresstablemodel.cpp

src/qt/bitcoinaddressvalidator.cpp

src/qt/coincontroldialog.cpp

src/qt/guiutil.cpp

src/qt/paymentserver.cpp

src/qt/sendcoinsdialog.cpp

src/qt/signverifymessagedialog.cpp

src/qt/transactiondesc.cpp

src/qt/transactionrecord.cpp

src/qt/walletmodel.cpp

src/rpc/mining.cpp

src/rpc/misc.cpp

src/rpc/rawtransaction.cpp

src/script/standard.h

src/script/standard.cpp

src/test/base58_tests.cpp

src/test/key_tests.cpp

src/wallet/rpcdump.cpp

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

src/wallet/walletdb.cpp

Introduce wrappers around CBitcoinAddressClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1405

src/base58.h

src/base58.cpp

src/bitcoin-tx.cpp

src/core_write.cpp

src/qt/addresstablemodel.cpp

src/qt/bitcoinaddressvalidator.cpp

src/qt/coincontroldialog.cpp

src/qt/guiutil.cpp

src/qt/paymentserver.cpp

src/qt/sendcoinsdialog.cpp

src/qt/signverifymessagedialog.cpp

src/qt/transactiondesc.cpp

src/qt/transactionrecord.cpp

src/qt/walletmodel.cpp

src/rpc/mining.cpp

src/rpc/misc.cpp

src/rpc/rawtransaction.cpp

src/script/standard.h

src/script/standard.cpp

src/test/base58_tests.cpp

src/test/key_tests.cpp

src/wallet/rpcdump.cpp

src/wallet/rpcwallet.cpp

src/wallet/wallet.cpp

src/wallet/walletdb.cpp

Introduce wrappers around CBitcoinAddress
ClosedPublic
Actions

Revision Contents
Changeset List