Page MenuHomePhabricator

Introduce bitcoinUriScheme
ClosedPublic

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

Details

Summary

Adds support for the new uri schemes introduced by cashaddr.

Test Plan

Added tests

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.Dec 12 2017, 11:05
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 ↗(On Diff #2084)

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 ↗(On Diff #2084)

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 ↗(On Diff #2084)

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 ↗(On Diff #2084)

Really, cfg is not significantly shorter than config.

Also, what tells us we are supposed to be on main net here ?

411 ↗(On Diff #2084)

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 ↗(On Diff #2084)

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 ↗(On Diff #2084)

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 ↗(On Diff #2084)

I ment D583

deadalnix added inline comments.Dec 13 2017, 17:45
src/qt/paymentserver.cpp
205 ↗(On Diff #2084)

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 ↗(On Diff #2084)

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 ↗(On Diff #2084)

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.

deadalnix added inline comments.Dec 14 2017, 22:06
src/qt/paymentserver.cpp
205 ↗(On Diff #2084)

I'm good with that.

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 2135.Dec 15 2017, 12:07

removed dummy object, cfg -> config

deadalnix accepted this revision.Dec 15 2017, 16:54
This revision is now accepted and ready to land.Dec 15 2017, 16:54
This revision was automatically updated to reflect the committed changes.