R4R: Auth Module Param Store #2998

alexanderbez · 2018-12-04T19:59:12Z

Drastically cleanup/DRY unit tests (via testInput type)
Remove remnants of old am variable and receiver usage
Implement new Params type
Update ante handler to use params (notably: consumeSignatureVerificationGas)

closes: #2996

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added entries in PENDING.md with issue #
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/auth/ante.go

codecov · 2018-12-05T21:59:01Z

Codecov Report

Merging #2998 into develop will increase coverage by 0.13%.
The diff coverage is 70.14%.

@@             Coverage Diff             @@
##           develop    #2998      +/-   ##
===========================================
+ Coverage    54.72%   54.86%   +0.13%     
===========================================
  Files          132      133       +1     
  Lines         9425     9546     +121     
===========================================
+ Hits          5158     5237      +79     
- Misses        3949     3988      +39     
- Partials       318      321       +3

x/auth/genesis.go

jackzampolin

Was looking through here for the client functionality and not seeing it. Shouldn't we be able to query these params from both the LCD and CLI?

Also we might want to add genesis validation like #3006

cwgoes

Quick review, structural changes look fine, a few suggestions and I would second @ValarDragon / @jackzampolin's comments.

x/auth/ante.go

x/auth/context.go

cwgoes · 2018-12-12T12:26:13Z

x/auth/stdtx.go

@@ -55,28 +55,21 @@ func (tx StdTx) ValidateBasic() sdk.Error {
 if len(stdSigs) != len(tx.GetSigners()) {
 return sdk.ErrUnauthorized("wrong number of signers")
 }
- if len(tx.GetMemo()) > maxMemoCharacters {


Hmm, what about having ValidateBasic require a params? I like separating the stateless checks, it's cleaner.

Sure! That would require updating the Tx interface. Are we OK with this?

That seems fine, maybe we could have a sub-struct in Params just for tx validation

See comment below.

cwgoes · 2018-12-12T12:26:26Z

x/auth/stdtx.go


 sigCount := 0
 for i := 0; i < len(stdSigs); i++ {
 sigCount += countSubKeys(stdSigs[i].PubKey)
- if sigCount > txSigLimit {
+ if uint64(sigCount) > DefaultTxSigLimit {


This seems like it should definitely be a param

Very strange. It is a param. Must've forgotten to update this.

Ohhh wait, this is because it's in ValidateBasic too. I don't know of a clean way really to support this. Maybe ValidateBasic can take a params interface{}...? Keep in mind, the Tx interface is defined in /types.

I guess this is OK for now. Maybe let's discuss in an issue

Created an issue here: #3164

alexanderbez · 2018-12-13T16:13:09Z

@cwgoes updated.

cwgoes · 2018-12-18T18:25:38Z

Needs a rebase.

alexanderbez · 2018-12-18T19:11:13Z

@cwgoes rebased.

cwgoes

Tested ACK

A few remaining concerns, but they don't need to block merge. This should see an additional review.

cwgoes · 2018-12-18T20:17:12Z

x/auth/ante.go

+ switch {
+ case strings.Contains(pubkeyType, "ed25519"):
+ meter.ConsumeGas(params.SigVerifyCostED25519, "ante verify: ed25519")
+ case strings.Contains(pubkeyType, "secp256k1"):


This does add quite a bit of technical debt, although the struct names are unlikely to change. At minimum let's create an issue upstream.

cwgoes · 2018-12-18T20:18:33Z

x/auth/stdtx.go


 sigCount := 0
 for i := 0; i < len(stdSigs); i++ {
 sigCount += countSubKeys(stdSigs[i].PubKey)
- if sigCount > txSigLimit {
+ if uint64(sigCount) > DefaultTxSigLimit {


I guess this is OK for now. Maybe let's discuss in an issue

cwgoes · 2018-12-19T20:00:45Z

Needs a rebase now @alexanderbez (sorry) - and also another review.

cmd/gaia/app/genesis.go

jackzampolin

This is a great addition. Tested locally and all working for me. I think this just needs a rebase.

Aleksandr Bezobchuk added 2 commits December 4, 2018 14:56

Move auth types to new types package with params

436930c

Slight cleanup of account keeper

0975772

alexanderbez added wip labels Dec 4, 2018

alexanderbez requested review from cwgoes, ebuchman and rigelrozanski as code owners December 4, 2018 19:59

Cleanup and update unit tests

70d127c

alexanderbez added the Type: Code Hygiene General cleanup and restructuring of code to provide clarity, flexibility, and modularity. label Dec 4, 2018

alexanderbez changed the title ~~WIP: Auth Module Param Store~~ WIP: Auth Module Param Store - Pt I Dec 4, 2018

alexanderbez closed this Dec 4, 2018

Update auth module to use params

762982a

alexanderbez reopened this Dec 5, 2018

Update consumeSignatureVerificationGas panic

d84ce10

alexanderbez changed the title ~~WIP: Auth Module Param Store - Pt I~~ WIP: Auth Module Param Store Dec 5, 2018

alexanderbez requested review from ValarDragon, alessio and jackzampolin December 5, 2018 01:32

Aleksandr Bezobchuk added 2 commits December 4, 2018 23:08

Update genesis logic

2ee7a46

Update existing app param keeper init

d24232e

alexanderbez requested a review from zramsay as a code owner December 5, 2018 04:18

alessio reviewed Dec 5, 2018

View reviewed changes

x/auth/ante.go Outdated Show resolved Hide resolved

Update unit tests account keeper usage

b2a5b70

jackzampolin and others added 4 commits December 5, 2018 16:52

Merge branch 'develop' into bez/2996-auth-param-store

58a72cb

Reduce number of changes

bc3427b

Merge branch 'develop' into bez/2996-auth-param-store

7169d16

Make countSubKeys private

c805972

alexanderbez added the T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). label Dec 6, 2018

Add PENDING log

75fbce8

Aleksandr Bezobchuk added 2 commits December 10, 2018 10:22

Merge branch 'develop' into bez/2996-auth-param-store

077c623

Merge branch 'develop' into bez/2996-auth-param-store

7fb1389

jackzampolin reviewed Dec 11, 2018

View reviewed changes

x/auth/genesis.go Show resolved Hide resolved

jackzampolin reviewed Dec 11, 2018

View reviewed changes

cwgoes suggested changes Dec 12, 2018

View reviewed changes

Aleksandr Bezobchuk added 6 commits December 13, 2018 09:39

Remove auth context

73d2601

Add ref TODO

3d6fc31

Merge branch 'develop' into bez/2996-auth-param-store

8ab1704

Add auth genesis validation

a200051

Add auth sim params

5276247

Fix sim auth params

d50889f

jackzampolin mentioned this pull request Dec 17, 2018

Release 0.29 Tracking #3139

Closed

cwgoes mentioned this pull request Dec 18, 2018

"Red Wedding" (first GoS hard fork) master to-do list #3079

Closed

7 tasks

Merge branch 'develop' into bez/2996-auth-param-store

0b47963

cwgoes approved these changes Dec 18, 2018

View reviewed changes

This was referenced Dec 19, 2018

Update PubKey interface to include Name or Type tendermint/tendermint#3050

Closed

Tx ValidateBasic params #3164

Closed

Aleksandr Bezobchuk added 2 commits December 19, 2018 09:22

Merge branch 'develop' into bez/2996-auth-param-store

bc5c147

Minor cleanup

301ee4e

jackzampolin reviewed Dec 19, 2018

View reviewed changes

cmd/gaia/app/genesis.go Outdated Show resolved Hide resolved

jackzampolin approved these changes Dec 19, 2018

View reviewed changes

Aleksandr Bezobchuk added 2 commits December 20, 2018 10:09

Merge branch 'develop' into bez/2996-auth-param-store

039e7eb

Update GaiaValidateGenesisState

77b9d23

jackzampolin added the will-merge label Dec 20, 2018

jackzampolin merged commit 217a292 into develop Dec 20, 2018

jackzampolin deleted the bez/2996-auth-param-store branch December 20, 2018 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

R4R: Auth Module Param Store #2998

R4R: Auth Module Param Store #2998

alexanderbez commented Dec 4, 2018 •

edited

Loading

codecov bot commented Dec 5, 2018 •

edited

Loading

jackzampolin left a comment •

edited

Loading

cwgoes left a comment

cwgoes Dec 12, 2018

alexanderbez Dec 12, 2018

cwgoes Dec 12, 2018

alexanderbez Dec 13, 2018

cwgoes Dec 12, 2018

alexanderbez Dec 12, 2018

alexanderbez Dec 13, 2018 •

edited

Loading

cwgoes Dec 18, 2018

alexanderbez Dec 19, 2018

alexanderbez commented Dec 13, 2018

cwgoes commented Dec 18, 2018

alexanderbez commented Dec 18, 2018

cwgoes left a comment

cwgoes Dec 18, 2018

cwgoes Dec 18, 2018

cwgoes commented Dec 19, 2018

jackzampolin left a comment •

edited

Loading

R4R: Auth Module Param Store #2998

R4R: Auth Module Param Store #2998

Conversation

alexanderbez commented Dec 4, 2018 • edited Loading

codecov bot commented Dec 5, 2018 • edited Loading

Codecov Report

jackzampolin left a comment • edited Loading

Choose a reason for hiding this comment

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Dec 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez commented Dec 13, 2018

cwgoes commented Dec 18, 2018

alexanderbez commented Dec 18, 2018

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes commented Dec 19, 2018

jackzampolin left a comment • edited Loading

Choose a reason for hiding this comment

alexanderbez commented Dec 4, 2018 •

edited

Loading

codecov bot commented Dec 5, 2018 •

edited

Loading

jackzampolin left a comment •

edited

Loading

alexanderbez Dec 13, 2018 •

edited

Loading

jackzampolin left a comment •

edited

Loading