Details

Reviewers

dagurval
schancel

Group Reviewers

Restricted Project

Commits

rSTAGING2966060caa70: Support parsing cashaddr from command line
rABC2966060caa70: Support parsing cashaddr from command line

Summary

This makes the client support parsing cashaddr encoded addresses for all networks.

Depends on D797

Test Plan

./bitcoin-qt "bitcoincash:mz8DUu5atAfNEBNeQHrpdGkfWZ9vmk3Q13?amount=42.00000000&label=test1&message=a%20message"

Ensure that the client boot in testnet mode and the proper sent is prefilled.

./bitcoin-qt "bitcoincash:1F6MqpCw3f8JRErcZMYjkFLBzpFvvjobZZ?amount=42.00000000&label=mainnet1&message=a%20message"

Ensure the client boots in mainnet and the sent form is prefilled.

./bitcoin-qt "bchtest:qp5dzwdct8xcpec62z799rdfpylxftsl6v0trfz2m9?amount=42.00000000&label=cashtest2&message=a%20message" "bitcoincash:mz8DUu5atAfNEBNeQHrpdGkfWZ9vmk3Q13?amount=42.00000000&label=test1&message=a%20message"

Ensure that the client start on testnet and fill up the sent form properly.

./bitcoin-qt "bitcoincash:qrc5xvnqwv3esj8hs5w85gh3xjwgnnjfvv3ll6wmdu?amount=42.00000000&label=test3&message=a%20message" "bitcoincash:1F6MqpCw3f8JRErcZMYjkFLBzpFvvjobZZ?amount=42.00000000&label=mainnet1&message=a%20message"

dito

./bitcoin-qt "bchreg:qrxvv8fpv42wvlls6z4mp9qvrs3kar2pau5tssl8xd?amount=42.00000000&label=cashtest1&message=a%20message"

Start in regtest mode.

Diff Detail

Repository

rABC Bitcoin ABC

Branch

arcpatch-D583

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 1387
Build 1387: arc lint + arc unit

Event Timeline

dagurval created this revision.Oct 4 2017, 21:50

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 4 2017, 21:50

dagurval planned changes to this revision.Nov 7 2017, 08:21

Rebased on top of D709

dagurval retitled this revision from [qt] Cash address support to Support multiple URI schemes in Qt.Nov 27 2017, 13:31

dagurval edited the summary of this revision. (Show Details)

dagurval edited the test plan for this revision. (Show Details)

dagurval added a parent revision: D709: Support cashaddr in parse/format URI.

fix rebase issue in tests

Harbormaster completed remote builds in B1198: Diff 1870.Nov 27 2017, 14:21

Test plan is not adequate. Manual is not something I can reproduce if I wanted/needed to.

It also looks like this diff does several things at once. The GUIUtil::parseBitcoinURI a brain dead change that could be in by now, and what's not in there could be reviewed much more easily.

src/qt/guiutil.cpp
169 ↗	(On Diff #1870)	Braces
src/qt/paymentserver.cpp
203 ↗	(On Diff #1870)	remove
210 ↗	(On Diff #1870)	You have created an helper function to do this.
227 ↗	(On Diff #1870)	This is validating that it is a valid destination.
234 ↗	(On Diff #1870)	This is not.
395 ↗	(On Diff #1870)	You have an helper function for this one as well.

This revision now requires changes to proceed.Dec 5 2017, 15:10

Test plan is not adequate. Manual is not something I can reproduce if I wanted/needed to.

How can the test plan be improved? Should it include the manual test steps? I'm afraid this cannot be automated in any sane way.

src/qt/paymentserver.cpp
210 ↗	(On Diff #1870)	The helper function requires a config object, which we don't have. The alternative is to create a dummy config object.
227 ↗	(On Diff #1870)	My interpretation of this code is that it's a hack to check if it's a mainnet address or a testnet address. Validation is a side effect of that. We can do that now by looking at the URI scheme. Validation is done in handleURIOrFile. I can add validation though before using the chain params.
395 ↗	(On Diff #1870)	Hard to tell if the GetConfig() object is ready here (or will be after some change in the future).

Parts of this were split out into D797 and D769. Rebased on top.

dagurval retitled this revision from Support multiple URI schemes in Qt to Support parsing cashaddr from command line.Dec 12 2017, 12:14

dagurval edited the summary of this revision. (Show Details)

dagurval edited the test plan for this revision. (Show Details)

dagurval added a parent revision: D797: Introduce bitcoinUriScheme.

dagurval mentioned this in D797: Introduce bitcoinUriScheme.Dec 13 2017, 16:28

This is 99% there.

src/qt/paymentserver.cpp
242 ↗	(On Diff #2085)	Putting that here makes it is very dangerous function to use in general. I would extract the loop and the SelectParams call in ipcParseCommandLine and keep the inner working of the loop in that function, which would now take networks as an argument.

deadalnix requested changes to this revision.Dec 15 2017, 00:02

This revision now requires changes to proceed.Dec 15 2017, 00:02

Moved network selection to bitcoin.cpp

Harbormaster completed remote builds in B1380: Diff 2136.Dec 15 2017, 13:13

Added a warning if two (or more) addresses from incompatible networks are parsed.

Harbormaster completed remote builds in B1381: Diff 2137.Dec 15 2017, 13:26

deadalnix requested changes to this revision.Dec 15 2017, 18:04

deadalnix added inline comments.

src/qt/paymentserver.cpp
229 ↗	(On Diff #2137)	It doesn't seem like you need to return a pair.
266 ↗	(On Diff #2137)	I guess this guy should check if the network provided int he address match or not. There are clearly different source of network here and they should all be consistent. We don't want to be sending coins on the wrong network.

This revision now requires changes to proceed.Dec 15 2017, 18:04

Don't use pair

Harbormaster completed remote builds in B1387: Diff 2145.Dec 15 2017, 20:02

deadalnix added inline comments.Dec 16 2017, 00:04

src/qt/paymentserver.cpp
264	here
275	This is still selecting network that may not correspond to the addresses. Plus this is checked nowhere so I'm not sure what's the point of returning it.

deadalnix commandeered this revision.Dec 17 2017, 15:59

deadalnix edited reviewers, added: dagurval; removed: deadalnix.

deadalnix edited the summary of this revision. (Show Details)Dec 17 2017, 16:24

deadalnix edited the test plan for this revision. (Show Details)

Unbreak support for testnet.
Do set the network in one place.

Harbormaster completed remote builds in B1397: Diff 2163.Dec 17 2017, 16:26

schancel added a subscriber: schancel.Dec 17 2017, 19:41

schancel added inline comments.

src/qt/paymentserver.cpp
219 ↗	(On Diff #2163)	If this is just checking validity, should be called `CanParse` or something to that effect.
226 ↗	(On Diff #2163)	Ditto
505 ↗	(On Diff #2163)	Isn't the scheme the same between these two calls unless we're in regtest/testnet mode? How will this return a different value the second time if it is false the first?