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

x/authz: audit updates #9042

Merged
merged 50 commits into from
May 6, 2021
Merged

x/authz: audit updates #9042

merged 50 commits into from
May 6, 2021

Conversation

robert-zaremba
Copy link
Collaborator

@robert-zaremba robert-zaremba commented Apr 1, 2021 •
edited by aaronc
Loading

Description

Suggestions and updates related to x/authz internal audit.

ref: #9114

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

@robert-zaremba robert-zaremba marked this pull request as ready for review April 23, 2021 09:37
amaury1093
amaury1093 reviewed Apr 23, 2021
View reviewed changes
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.

Made a first quick scan, nice work 👍 , I love the changes:

  • method/message names are shorter in proto files
  • merge the 2 grpc queries in one
  • better keeper methods

I think there will be some conflicts with #9139

proto/cosmos/authz/v1beta1/event.proto Outdated Show resolved Hide resolved
proto/cosmos/authz/v1beta1/event.proto Outdated Show resolved Hide resolved
proto/cosmos/authz/v1beta1/event.proto Outdated Show resolved Hide resolved
x/authz/exported/authorizations.go Outdated Show resolved Hide resolved
x/authz/simulation/operations.go Outdated Show resolved Hide resolved
amaury1093
amaury1093 approved these changes May 4, 2021
View reviewed changes
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.

lgtm, pre-approving pending conflicts

x/authz/keeper/grpc_query.go Show resolved Hide resolved
@robert-zaremba
Copy link
Collaborator Author

I prefer to fix conflicts once after having second approval. Let's wrap it and close it.

aaronc
aaronc approved these changes May 4, 2021
View reviewed changes
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

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

Still a couple things to clean up but pre-approving

x/authz/client/cli/query.go Outdated Show resolved Hide resolved
x/authz/client/cli/query.go Show resolved Hide resolved
x/authz/codec/codec.go Outdated Show resolved Hide resolved
blushi
blushi reviewed May 5, 2021
View reviewed changes
Copy link
Contributor

@blushi blushi left a comment

Choose a reason for hiding this comment

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

Should we also add some changelog to the authz ADR and update it according to these changes?

@robert-zaremba
Copy link
Collaborator Author

Should we also add some changelog to the authz ADR and update it according to these changes?

Authz is new in 0.43, so we shouldn't put changes for not released things in a Changelog.

@robert-zaremba robert-zaremba mentioned this pull request May 5, 2021
Merged
9 tasks
@robert-zaremba
Copy link
Collaborator Author

Should we also add some changelog to the authz ADR and update it according to these changes?

Yes, doing it in a separate PR: #9270

@blushi
Copy link
Contributor

blushi commented May 6, 2021

Should we also add some changelog to the authz ADR and update it according to these changes?

Authz is new in 0.43, so we shouldn't put changes for not released things in a Changelog.

I meant the changelog at the beginning of the ADR (not the general changelog file)

@blushi blushi mentioned this pull request May 6, 2021
Merged
9 tasks
@aaronc
Copy link
Member

aaronc commented May 6, 2021 •
edited
Loading

There are failing simulations and tests. Can you check this @robert-zaremba ?

@alessio
Copy link
Contributor

alessio commented May 6, 2021

This should be easy I guess:

# github.com/cosmos/cosmos-sdk/simapp [github.com/cosmos/cosmos-sdk/simapp.test]
simapp/sim_test.go:177:13: undefined: "github.com/cosmos/cosmos-sdk/x/authz".StoreKey

@aaronc aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label May 6, 2021
@mergify mergify bot merged commit 59810f3 into master May 6, 2021
@mergify mergify bot deleted the robert/authz-audit branch May 6, 2021 18:23
@technicallyty
Copy link
Contributor

technicallyty commented May 6, 2021 •
edited
Loading

i noticed this comment wasn't updated to reflect one of the changes in this PR when merging into my working branch - just a heads up

@robert-zaremba
Copy link
Collaborator Author

Thanks @technicallyty - I will update the comment int #9270

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023
@robert-zaremba @aaronc @amaury1093
x/authz: audit updates (cosmos#9042)
928f1ca 
* x/authz: audit updates

* audit with Aaron

* authz: Update Authorization.Accept method

* authz: add event proto definitions

* update query service

* authz: use typed events

* refactore and rename query authorizations

* remve Authorization infix from proto services

* renames wip

* refactoring

* update tests

* fix compilation

* fixing gRPC query tests

* fix simulation tests

* few renames

* more refactore

* add missing file

* moving export genesis to keeper

* Update docs

* update tests

* rename event Msg attribute to MsgTypeURL

* Upate Authorization interface

* rollback Makefile changes

* fix tests

* Apply suggestions from code review

Co-authored-by: Aaron Craelius <[email protected]>

* renames

* refactore authz/exported

* lint fix

* authz/types refactore

* comment update

* conflict updates

* Apply suggestions from code review

Co-authored-by: Amaury <[email protected]>

* authz: move storage keys to keeper

* review updates

* docs update

* Update x/authz/client/cli/query.go

Co-authored-by: Aaron Craelius <[email protected]>

* move codec to the root package

* authz CMD info update

* comment update

* update imports and build flags

* fix functional tests

* update proto comment

* fix tests

* fix test

Co-authored-by: Aaron Craelius <[email protected]>
Co-authored-by: Amaury <[email protected]>
@jayy04 jayy04 mentioned this pull request Jan 12, 2024
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amaury1093 amaury1093 amaury1093 approved these changes

@blushi blushi blushi left review comments

@aaronc aaronc aaronc approved these changes

@alessio alessio Awaiting requested review from alessio

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

Assignees

@amaury1093 amaury1093

@blushi blushi

Labels
A:automerge Automatically merge PR once all prerequisites pass. C:CLI C:Simulations C:x/authz C:x/bank C:x/staking T:Docs Changes and features related to documentation.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@robert-zaremba @aaronc @blushi @alessio @technicallyty @amaury1093