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

WIP: Add x/msg_authorization module #7370

Closed
wants to merge 89 commits into from
Closed

Conversation

atheeshp
Copy link
Contributor

ref: #5412

Description

closes: #XXXX


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

@atheeshp atheeshp changed the title Adds x/authorization module WIP: Adds x/authorization module Sep 23, 2020
@codecov
Copy link

codecov bot commented Sep 23, 2020

Codecov Report

Merging #7370 into master will decrease coverage by 0.06%.
The diff coverage is 51.32%.

@@            Coverage Diff             @@
##           master    #7370      +/-   ##
==========================================
- Coverage   54.21%   54.14%   -0.07%     
==========================================
  Files         611      631      +20     
  Lines       38464    39291     +827     
==========================================
+ Hits        20852    21276     +424     
- Misses      15503    15864     +361     
- Partials     2109     2151      +42     

@anilcse anilcse changed the title WIP: Adds x/authorization module WIP: Adds x/msg_authorization module Sep 24, 2020
@@ -0,0 +1,130 @@
package keeper
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe having an internal folder was the old way of writing modules. Let's stick to having keeper at the root folder of the module

}
granter := signers[0]
if !bytes.Equal(granter, grantee) {
authorization, _ := k.GetAuthorization(ctx, grantee, granter, msg.Type())
Copy link
Member

Choose a reason for hiding this comment

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

I'll need to update the ADR draft but we should use proto.MessageName(msg) now instead of msg.Type()

@lgtm-com
Copy link

lgtm-com bot commented Oct 16, 2020

This pull request introduces 2 alerts when merging cf097d4 into 332d8ec - view on LGTM.com

new alerts:

  • 2 for Useless assignment to local variable

@@ -0,0 +1,12 @@
syntax = "proto3";
package cosmos.msg_authorization.v1beta1;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
package cosmos.msg_authorization.v1beta1;
package cosmos.authz.v1beta1;

@@ -0,0 +1,46 @@
package msg_authorization
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
package msg_authorization
package authz

@aleem1314
Copy link
Contributor

closing this PR in favour of #7629

@aleem1314 aleem1314 closed this Nov 30, 2020
@amaury1093 amaury1093 deleted the msg_authorization branch December 18, 2020 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants