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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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
dagurval updated this revision to Diff 1869.Nov 27 2017, 13:28

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 updated this revision to Diff 1870.Nov 27 2017, 14:21

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
dagurval added a comment.Dec 5 2017, 20:43

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).

dagurval updated this revision to Diff 2085.Dec 12 2017, 12:13

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
dagurval updated this revision to Diff 2136.Dec 15 2017, 13:13

Moved network selection to bitcoin.cpp

dagurval updated this revision to Diff 2137.Dec 15 2017, 13:26

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
dagurval updated this revision to Diff 2145.Dec 15 2017, 20:02

Don't use pair

deadalnix added inline comments.Dec 16 2017, 00: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.Dec 17 2017, 15:59
deadalnix commandeered this revision.
deadalnix edited the summary of this revision. (Show Details)Dec 17 2017, 16:24
deadalnix edited the test plan for this revision. (Show Details)
deadalnix updated this revision to Diff 2163.Dec 17 2017, 16:26

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?

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

The prefix is not the same when not on mainnet.

deadalnix updated this revision to Diff 2177.Dec 17 2017, 21:55

can parse

This revision was automatically updated to reflect the committed changes.