R4R Client Generalization (+ associated module refactor) #4451

rigelrozanski · 2019-05-30T16:46:41Z

Replaces #4296

closes #1124
closes #2955
closes #4299

followup to module genesis generalization

To accomplish the client generalization which is in theory pretty simple there is a bunch of restructuring required due to lots of early design choices which are now leading to lots of circular dependencies.

Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry: clog add [section] [stanza] [message]
rereviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

x/gov/module.go

…lization2

crypto/ledger_mock.go

crypto/ledger_secp256k1.go

simapp/app.go

types/config.go

x/gov/client/cli/tx.go

x/slashing/keeper.go

x/slashing/types/keys.go

x/staking/types/codec.go

crypto/ledger_mock.go

crypto/ledger_test.go

simapp/app.go

x/auth/module.go

x/distribution/client/cli/tx.go

x/gov/types/codec.go

x/staking/types/expected_keepers.go

x/staking/client/rest/utils.go

codecov · 2019-06-03T19:29:26Z

Codecov Report

Merging #4451 into master will decrease coverage by 1.45%.
The diff coverage is 31.49%.

@@            Coverage Diff             @@
##           master    #4451      +/-   ##
==========================================
- Coverage   54.69%   53.23%   -1.46%     
==========================================
  Files         253      259       +6     
  Lines       16150    16173      +23     
==========================================
- Hits         8833     8610     -223     
- Misses       6668     6917     +249     
+ Partials      649      646       -3

crypto/keys/codec.go

simapp/app.go

x/distribution/client/cli/tx.go

x/distribution/types/codec.go

…lization2

jackzampolin

On the first read through here it looks really good and a substantial improvement @rigelrozanski !!!

My only concern was that it looks like we are moving the codec to be on a per module basis and using init in each module to handle the codec? This seems like a codesmell and def not something we should be doing afaik.

fedekunze

Second pass. Minor comments but otherwise LGTM

x/auth/keeper.go

fedekunze · 2019-06-03T21:29:56Z

x/bank/client/rest/sendtx.go

-func RegisterRoutes(cliCtx context.CLIContext, r *mux.Router, cdc *codec.Codec, kb keys.Keybase) {
-	r.HandleFunc("/bank/accounts/{address}/transfers", SendRequestHandlerFn(cdc, kb, cliCtx)).Methods("POST")
+func RegisterRoutes(cliCtx context.CLIContext, r *mux.Router, cdc *codec.Codec) {
+	r.HandleFunc("/bank/accounts/{address}/transfers", SendRequestHandlerFn(cdc, cliCtx)).Methods("POST")


where does the send command live rn ?

x/bank/client/rest/sendtx.go

x/bank/invariants.go

x/gov/keeper.go

x/staking/keeper/querier.go

Co-Authored-By: Federico Kunze <[email protected]>

alexanderbez

Performed a first initial passthrough. Left some general feedback. Overall, looks like a step in the right direction. Still not clear on why some codec are public and others aren't. Also, I think queriers should be consistently defined in terms of their location (ie. x/{module}/querier).

crypto/ledger_mock.go

go.mod

alexanderbez · 2019-06-04T13:20:30Z

types/module/module.go

 	DefaultGenesis() json.RawMessage
 	ValidateGenesis(json.RawMessage) error
+
+	// client functionality
+	RegisterRESTRoutes(context.CLIContext, *mux.Router, *codec.Codec)


Out of scope of this PR, but we can and should stop passing a CLIContext AND a *coded.Codec. The context should already have the codec set. I'll make an issue of this and address it once this PR is merged.

REF: #4479

x/auth/client/txbuilder/stdsignmsg.go

alexanderbez · 2019-06-04T13:43:31Z

x/auth/types/account_test.go

@@ -427,7 +427,7 @@ func TestTrackUndelegationDelVestingAcc(t *testing.T) {
 	now := tmtime.Now()
 	endTime := now.Add(24 * time.Hour)

-	_, _, addr := keyPubAddr()
+	_, _, addr := KeyTestPubAddr()


Does this need to be exposed?

if we don't want duplicate code it does need to be exposed... ideally this will move out into a common testing-helpers root level package at some point!

x/auth/types/querier.go

alexanderbez · 2019-06-04T13:47:26Z

x/bank/client/rest/sendtx.go

-func RegisterRoutes(cliCtx context.CLIContext, r *mux.Router, cdc *codec.Codec, kb keys.Keybase) {
-	r.HandleFunc("/bank/accounts/{address}/transfers", SendRequestHandlerFn(cdc, kb, cliCtx)).Methods("POST")
+func RegisterRoutes(cliCtx context.CLIContext, r *mux.Router, cdc *codec.Codec) {
+	r.HandleFunc("/bank/accounts/{address}/transfers", SendRequestHandlerFn(cdc, cliCtx)).Methods("POST")


Que? The send command lives in bank @jackzampolin. I think you mean the /bank/balances/{address} endpoint needs to move from auth to bank ;-)

alexanderbez · 2019-06-04T13:48:42Z

client/routes.go

+
+// Register routes
+func RegisterRoutes(cliCtx context.CLIContext, r *mux.Router, cdc *codec.Codec) {
+	RegisterRPCRoutes(cliCtx, r)


Once we remove the need for a codec as an argument, this should really only be RegisterRoutes 👍

I'm not sure that will be possible given that client doesn't define it's own "client" level cdc

x/distribution/client/proposal_handler.go

alexanderbez · 2019-06-04T13:52:43Z

x/gov/client/proposal_handler.go

+)
+
+// function to create the rest handler
+type RESTHandlerFn func(context.CLIContext, *codec.Codec) rest.ProposalRESTHandler


Errr shall we not just define this ProposalRESTHandler type in the rest package? Has nothing to do specifically with gov, right?

I'm confused ProposalRESTHandler is already defined in "github.com/cosmos/cosmos-sdk/x/gov/client/rest" - if you were referring to the RESTHandlerFn this should stay where it is, it's only used for defining the ProposalHandler (which belongs outside of rest because its used for both the cli and rest)

…mos/cosmos-sdk into rigel/client-generalization2

…lization2

x/distribution/client/cli/query.go

…lization2

fedekunze

👍 👍

alexanderbez

testedACK

porting changes from first branch

c5b89c3

rigelrozanski requested review from alessio, alexanderbez and jackzampolin as code owners May 30, 2019 16:46

golangcibot reviewed May 30, 2019

View reviewed changes

x/gov/module.go Outdated Show resolved Hide resolved

rigelrozanski mentioned this pull request May 30, 2019

WIP client generalization #4296

Closed

5 tasks

rigelrozanski added 7 commits May 30, 2019 13:57

fix import cycles

f4e2f93

circle dep cont.

ca8f4cd

working on compile errors

8206f89

simapp addition

32e14a5

...

93d6e60

working through compile errors

bf09851

resolve all compile errors

f6decfb

rigelrozanski changed the title ~~WIP Client Generalization~~ WIP Client Generalization (+ associated module refactor) May 31, 2019

rigelrozanski added 4 commits May 31, 2019 16:20

fix bugs

abfbe47

Merge remote-tracking branch 'origin/master' into rigel/client-genera…

0ed158d

…lization2

gov proposal handler type

5c62e49

client routes

d010ed7

rigelrozanski mentioned this pull request May 31, 2019

R4R Update for client generalization cosmos/gaia#31

Merged

5 tasks

cl

f7246b6

rigelrozanski changed the title ~~WIP Client Generalization (+ associated module refactor)~~ R4R Client Generalization (+ associated module refactor) May 31, 2019

jleni requested changes Jun 1, 2019

View reviewed changes

fedekunze reviewed Jun 1, 2019

View reviewed changes

rigelrozanski mentioned this pull request Jun 3, 2019

Add validate genesis for crisis keeper params #4472

Closed

@jleni comments

27a52d4

rigelrozanski requested a review from jleni June 3, 2019 19:29

rigelrozanski mentioned this pull request Jun 3, 2019

find a way to seal governance codec #4473

Closed