Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

R4R: Limit total number of signatures per transaction #2800

Merged
merged 10 commits into from
Nov 15, 2018
Merged

R4R: Limit total number of signatures per transaction #2800

merged 10 commits into from
Nov 15, 2018

Conversation

alessio
Copy link
Contributor

@alessio alessio commented Nov 14, 2018 •
edited by alexanderbez
Loading

closes: #2019

  • 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)

@codecov
Copy link

codecov bot commented Nov 14, 2018 •
edited
Loading

Codecov Report

Merging #2800 into develop will increase coverage by 0.09%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop    #2800      +/-   ##
===========================================
+ Coverage    56.65%   56.75%   +0.09%     
===========================================
  Files          131      131              
  Lines         8539     8554      +15     
===========================================
+ Hits          4838     4855      +17     
+ Misses        3368     3366       -2     
  Partials       333      333

alexanderbez
alexanderbez reviewed Nov 14, 2018
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alessio, so far so good! I just the language txSigLimit is a bit a cleaner is all. Might wanna update that?

baseapp/baseapp.go Outdated Show resolved Hide resolved
x/auth/ante.go Outdated Show resolved Hide resolved
@alessio alessio force-pushed the alessio/2019-antispam-prevention branch 2 times, most recently from bbcad54 to fd99353 Compare November 14, 2018 01:17
@alessio alessio changed the title WIP: Limit total number of signatures per transaction R4R: Limit total number of signatures per transaction Nov 14, 2018
@alessio alessio changed the title R4R: Limit total number of signatures per transaction WIP: Limit total number of signatures per transaction Nov 14, 2018
cwgoes
cwgoes reviewed Nov 14, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments.

baseapp/baseapp.go Outdated Show resolved Hide resolved
server/config/config.go Outdated Show resolved Hide resolved
x/auth/ante.go Outdated Show resolved Hide resolved
@alessio alessio force-pushed the alessio/2019-antispam-prevention branch from 0daf8e8 to 6e6a029 Compare November 14, 2018 11:58
@jackzampolin jackzampolin mentioned this pull request Nov 14, 2018
Closed
27 tasks
@alessio alessio changed the title WIP: Limit total number of signatures per transaction R4R: Limit total number of signatures per transaction Nov 15, 2018
cwgoes
cwgoes suggested changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes & testcases look good to me.

  • Needs PENDING.md entry
  • Can we add a note on the signature limit to the ante handler docs (or the baseapp docs)?

@alessio alessio requested a review from zramsay as a code owner November 15, 2018 11:17
@alessio
Copy link
Contributor Author

alessio commented Nov 15, 2018

@cwgoes done, thanks

cwgoes
cwgoes reviewed Nov 15, 2018
View reviewed changes
x/auth/ante.go Show resolved Hide resolved
@alessio alessio force-pushed the alessio/2019-antispam-prevention branch from 4d80c70 to c7c434c Compare November 15, 2018 12:32
cwgoes
cwgoes approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested ACK, this should see an additional review.

alexanderbez
alexanderbez approved these changes Nov 15, 2018
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Although we should note this somewhere. Perhaps in the BaseApp doc.

@cwgoes cwgoes merged commit 7b7d45d into develop Nov 15, 2018
@cwgoes cwgoes deleted the alessio/2019-antispam-prevention branch November 15, 2018 14:30
kevlubkcm
kevlubkcm reviewed Nov 15, 2018
View reviewed changes
types/errors.go
@@ -103,6 +104,8 @@ func CodeToDefaultMsg(code CodeType) string {
return "memo too large"
case CodeInsufficientFee:
return "insufficient fee"
case CodeTooManySignatures:
return "maximum numer of signatures exceeded"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numer -> number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh! Noted, will fix in another PR. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have misspell in CI?

@HaoyangLiu HaoyangLiu mentioned this pull request Dec 6, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevlubkcm kevlubkcm kevlubkcm left review comments

@alexanderbez alexanderbez alexanderbez approved these changes

@cwgoes cwgoes cwgoes approved these changes

@ebuchman ebuchman Awaiting requested review from ebuchman

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

@zramsay zramsay Awaiting requested review from zramsay

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AnteHandler spam prevention
4 participants
@alessio @alexanderbez @cwgoes @kevlubkcm