Page MenuHomePhabricator

WIP: Remove GetConfig/Params call from dstencode
AbandonedPublic

Authored by matiu on Jun 19 2018, 17:24.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

As per title

Test Plan

make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
master
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2755
Build 3621: Bitcoin ABC Buildbot (legacy)
Build 3620: arc lint + arc unit

Event Timeline

matiu retitled this revision from Remove GetConfig/Params call from dstencode to WIP: Remove GetConfig/Params call from dstencode.Jun 19 2018, 18:02
deadalnix requested changes to this revision.Jun 19 2018, 22:19
deadalnix added a subscriber: deadalnix.

It looks like this create an inordinate amount of churn because of the API change. It is unclear what changes in there are really useful. For instance, is there really a place where the wallet actually need the config ?

src/core_write.cpp
185

This looks like you only need the chainparams in there.

src/dstencode.h
16

Who do these need to take a config ? This seems like it si creating a ton of code churn for no good reason.

src/rpc/misc.cpp
139

If you add the config to the wallet, this is redundant.

289

Redundant.

src/rpc/misc.h
14

dito

src/wallet/rpcdump.cpp
801

You added a config to the wallet so adding this parameter is redundant (and open the door for inconsistencies).

src/wallet/wallet.h
678

private

713

This is taking ownership of the config, so it needs to pass it by pointer.

This revision now requires changes to proceed.Jun 19 2018, 22:19
jasonbcox added inline comments.
src/dstencode.h
15

End of this line looks very inconsistent without a variable name. Please add one even though there wasn't before. :)

@deadalnix @jasonbcox adding config param to many method's signatures doesn't feel right. If we include gArgs + ChainParams + ChainBaseParams to config it will lose meaning, you will end up not understanding what the method needs.

What would you think about doing a global singleton ConfigProvider, that join Config + gArgs (+configfile) + ChainParams?

It could implement argument parsing, config file parsing, and the ChainParams constants. It could be by default immutable, but it could be constructed mutable for testing.

Do you think it worth to try to implement this? What do you think would be the drawbacks of this approach?

src/rpc/misc.cpp
139

I seems to me that wallet shouldn't be the provider of 'config' to other classes. If we modify wallet in the future we could break classes that depend on that. Even more, don't you think 'config' on wallet should be private?

src/wallet/wallet.h
713

thanks, will change.