Introduce bitcoinUriScheme
ClosedPublic
Actions

Authored by dagurval on Dec 12 2017, 11:05.

Details

Reviewers

deadalnix
schancel
kyuupichan

Group Reviewers

Restricted Project

Commits

rSTAGINGa38bff37c337: Introduce bitcoinUriScheme
rABCa38bff37c337: Introduce bitcoinUriScheme

Summary

Adds support for the new uri schemes introduced by cashaddr.

Test Plan

Added tests

Diff Detail

Repository

rABC Bitcoin ABC

Branch

8-12-uriScheme-only

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 1351
Build 1351: arc lint + arc unit

Event Timeline

dagurval created this revision.Dec 12 2017, 11:05

dagurval mentioned this in D583: Support parsing cashaddr from command line.Dec 12 2017, 12:13

dagurval added a child revision: D583: Support parsing cashaddr from command line.Dec 12 2017, 12:14

Mengerian added reviewers: deadalnix, schancel, kyuupichan.Dec 12 2017, 17:33

Mengerian added a reviewer: Restricted Project.

I was under the impression that we would be gracefully degrading in the case of base58 addresses. This looks like there's some toggle flag that's required for the user?

src/qt/sendcoinsentry.cpp
39	It seemed like you were threading the config through the GUI. Should we pass it in here also?

dagurval added inline comments.Dec 13 2017, 08:26

src/qt/sendcoinsentry.cpp
39	You're right. How about doing that in a separate diff after this one?

deadalnix requested changes to this revision.Dec 13 2017, 15:38

deadalnix added inline comments.

src/qt/paymentserver.cpp
205	This sinkhole all the configs. It looks like what you want is a ProxyConfig class that wrap an existing config and forward all its method to it, and then override a method from it. Or maybe a way to clone a config and then change some of its parameter. Either way, this is not good design in its current shape.
230	Really, `cfg` is not significantly shorter than `config`. Also, what tells us we are supposed to be on main net here ?
411	dito

This revision now requires changes to proceed.Dec 13 2017, 15:38

dagurval requested review of this revision.Dec 13 2017, 16:26

dagurval added inline comments.

src/qt/paymentserver.cpp
205	There is no existing config to wrap at this point, so I’m not sure what you mean. Will you be working on new this config architecture (ProxyConfig)? Should this wait for it?
230	Also, what tells us we are supposed to be on main net here ? This change does nothing but remove the URI_SCHEME constant. The functionality (or lack of) is the same. The fix to support multiple schemes here is in D582.

dagurval added inline comments.Dec 13 2017, 16:28

src/qt/paymentserver.cpp
230	I ment D583

deadalnix added inline comments.Dec 13 2017, 17:45

src/qt/paymentserver.cpp
205	The problem here is that thing thing is clobbering all the other configs, not just the use of cashaddr. The only reason this work is because you know that the code down the road is only using that, but this is breaking the whole abstraction.
230	No, it is not the same. The network in the old system is determined by a prefix coming before the hash in the base58 encoded data. In this case it comes from the prefix. This is breaking properly functioning code. It sounds like D583 should come before this one. I guess this is ok if you this exclusively to fetch the prefix, but in this case, I don't see how it is an improvement over using a constant, and if this is used for anything else, this is quite dangerous.

dagurval added inline comments.Dec 14 2017, 07:27

src/qt/paymentserver.cpp
205	The only reason this work is because you know that the code down the road is only using that, but this is breaking the whole abstraction. This is why I used Params.CashAddrPrefix() to get the scheme instead of a dummy object in my previous diff revision (rejected because it didn't use bitcoinURIScheme()). How about creating a second `bitcoinURIScheme` function with signature `(const CChainParams&, bool useCashAddr)`? Then we don't need a dummy object. The `(const Config&)` one can still exist for convenience.