Differential D1509

WIP: Remove GetConfig/Params call from dstencode
AbandonedPublic
Actions

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 created this revision.Jun 19 2018, 17:24

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 19 2018, 17:24

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC :: Diffs :: Bitcoin-ABC Diff Testing failed.

Harbormaster failed remote builds in B2755: Diff 4147!Jun 19 2018, 17:46

matiu retitled this revision from Remove GetConfig/Params call from dstencode to WIP: Remove GetConfig/Params call from dstencode.Jun 19 2018, 18:02

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 a subscriber: jasonbcox.Jun 25 2018, 16:23

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.