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

Branch

non-base58-wrappers

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 875
Build 875: arc lint + arc unit

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	Put the comment withing the braces.
840	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 <pieter.wuille@gmail.com>, 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 1390

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 1390

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