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
Branch
arcpatch-D797
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 1379
Build 1379: arc lint + arc unit

Event Timeline

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?

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

src/qt/paymentserver.cpp
230 ↗(On Diff #2084)

I ment D583

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.

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.

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

removed dummy object, cfg -> config

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