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.

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

x/bank v0.43 Audit updates #9271

Merged
merged 24 commits into from
May 10, 2021
Merged

x/bank v0.43 Audit updates #9271

merged 24 commits into from
May 10, 2021

Conversation

technicallyty
Copy link
Contributor

@technicallyty technicallyty commented May 5, 2021

Description

  • update depreciated Supply proto message to point to correct path to interface
  • add comments to keeper methods
  • move event in SendCoins to emit after error checking
  • update keeper spec (removed SupplyI)
  • update messages spec
  • general cleanup (typos, redundancies, etc)
  • change mention of "blacklist" -> "blocklist"
  • change SendEnableCoin(s) -> IsSendEnabledCoin(s)

ref: #9218


Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • 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.
  • Code follows the module structure standards.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@github-actions github-actions bot added C:CLI C:x/bank T:Docs Changes and features related to documentation. labels May 5, 2021
@technicallyty technicallyty requested a review from amaury1093 May 5, 2021 23:30
@technicallyty technicallyty marked this pull request as draft May 5, 2021 23:44
@codecov
Copy link

codecov bot commented May 6, 2021

Codecov Report

Merging #9271 (fc58236) into master (75d3547) will not change coverage.
The diff coverage is 72.22%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9271   +/-   ##
=======================================
  Coverage   60.32%   60.32%           
=======================================
  Files         591      591           
  Lines       36936    36936           
=======================================
  Hits        22282    22282           
  Misses      12683    12683           
  Partials     1971     1971           
Impacted Files Coverage Δ
x/auth/vesting/msg_server.go 72.72% <0.00%> (ø)
x/bank/keeper/keeper.go 71.35% <ø> (ø)
x/bank/keeper/msg_server.go 2.12% <0.00%> (ø)
x/bank/simulation/operations.go 80.64% <0.00%> (ø)
x/bank/types/key.go 80.00% <ø> (ø)
x/bank/keeper/send.go 84.87% <100.00%> (ø)
x/bank/module.go 59.09% <100.00%> (ø)

@@ -14,15 +14,15 @@ permissions are limited in the way that you expect.

## Blacklisting Addresses

The `x/bank` module accepts a map of addresses that are considered blocklisted
The `x/bank` module accepts a map of addresses that are considered blacklisted
Copy link
Member

Choose a reason for hiding this comment

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

We need an alternative to "blacklist": https://developers.google.com/style/word-list#letter-b. This wording was intentional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i was thinking about that when i was reading through the spec. How about denylist? I think i've seen a few org's make the change to denylist and allowlist.

Copy link
Member

Choose a reason for hiding this comment

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

I don't have a strong preference personally

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like blocklisted was already heavily used, just a few comments weren't changed. I'll go with blocklist

@technicallyty technicallyty marked this pull request as ready for review May 6, 2021 16:17
@github-actions github-actions bot removed the C:CLI label May 7, 2021
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

just a changelog nit

CHANGELOG.md Outdated Show resolved Hide resolved
DelegateCoins(ctx sdk.Context, delegatorAddr, moduleAccAddr sdk.AccAddress, amt sdk.Coins) error
UndelegateCoins(ctx sdk.Context, moduleAccAddr, delegatorAddr sdk.AccAddress, amt sdk.Coins) error

types.QueryServer
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this was reformatted? Go style is to use tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i repasted the keeper definitions to make sure no small changes were missed in docs - I indented using tabs for this PR and double checked my editor this morning - it is set to tabs

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

Why indentation style has changed ? (from tabs to spaces)?

@aaronc aaronc merged commit 709ab08 into master May 10, 2021
@aaronc aaronc deleted the ty/9218-bank-audit branch May 10, 2021 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Simulations C:x/auth C:x/bank T:Docs Changes and features related to documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants