Page MenuHomePhabricator

Support parsing cashaddr from command line
ClosedPublic

Authored by deadalnix on Oct 4 2017, 21:50.

Details

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 1380
Build 1380: arc lint + arc unit

Event Timeline

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)

fix rebase issue in tests

deadalnix requested changes to this revision.Dec 5 2017, 15:10
deadalnix added a subscriber: deadalnix.

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.

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

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

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
src/qt/paymentserver.cpp
264 ↗(On Diff #2145)

here

275 ↗(On Diff #2145)

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 edited reviewers, added: dagurval; removed: deadalnix.
deadalnix edited the test plan for this revision. (Show Details)

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

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?

This revision is now accepted and ready to land.Dec 17 2017, 21:45
src/qt/paymentserver.cpp
505 ↗(On Diff #2163)

The prefix is not the same when not on mainnet.

This revision was automatically updated to reflect the committed changes.