update spec and fix code snippets #9334

robert-zaremba · 2021-05-14T12:50:23Z

Description

closes: #9257

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

ryanchristo

Looking good. A lot of little suggestions.

The code snippets need to be fixed. Also, some of the code snippets include the comment (those in the concepts and state documents), and some do not (those in the messages document). Not sure if this was intentional.

ryanchristo · 2021-05-14T18:11:31Z

x/authz/spec/03_messages.md


-## Msg/Grant
+## Grant


This section is about the message. I think we might want to reference the message not the method.

Suggested change

## Grant

## MsgGrant

I wanted to discuss and describe service methods, not the request objects. That's why I'm not using Msg prefix.

The title of this document is Messages. The next line of this section is "An authorization-grant is created using the MsgGrant message." Then we provide a code snippet for MsgGrant. And then we further discuss the message, "This message is expected to fail if..." Switching to the method names for the title of each section without changing the content or even mentioning the method is confusing.

OK, I can rename the file and the header to 03_tx_service.

[edit] renamed to 03_services.md.

Each service has exactly one argument, so it makes sense to describe the argument (MsgGrant) in this case.

x/authz/spec/03_messages.md

proto/cosmos/authz/v1beta1/tx.proto

x/authz/spec/01_concepts.md

x/authz/spec/03_messages.md

ryanchristo · 2021-05-15T18:53:25Z

It might be nice to order the triple (granter, Authorization, grantee) rather than (granter, grantee, Authorization), which would reflect the RDF order subject-predicate-object.

Also, the use of parenthesis around the triple seems a bit odd to me. We might want to consider removing the parenthesis or using another format such as granter/authorization/grantee. Thoughts?

Also, we need to make sure that we are consistent. Sometimes we use Authorization, sometimes we use AuthorizationType, and sometimes we use msg type.

Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com>

robert-zaremba · 2021-05-16T10:37:40Z

It might be nice to order the triple (granter, Authorization, grantee) rather than (granter, grantee, Authorization), which would reflect the RDF order subject-predicate-object.

Interesting idea. I think we don't need this at this level. The order of this relation is hidden from the user, and it's a storage detail. We use (granter, grantee, Authorization) because we need a prefix scan based on (granter, grantee).

codecov · 2021-05-16T10:45:41Z

Codecov Report

Merging #9334 (a711248) into master (53fe03e) will decrease coverage by 0.08%.
The diff coverage is n/a.

❗ Current head a711248 differs from pull request most recent head 1767cdc. Consider uploading reports for the commit 1767cdc to get more accurate results

@@            Coverage Diff             @@
##           master    #9334      +/-   ##
==========================================
- Coverage   60.39%   60.30%   -0.09%     
==========================================
  Files         589      588       -1     
  Lines       37087    36978     -109     
==========================================
- Hits        22399    22301      -98     
+ Misses      12711    12700      -11     
  Partials     1977     1977

Impacted Files	Coverage Δ
x/upgrade/keeper/grpc_query.go	`58.82% <0.00%> (-15.26%)`	⬇️
x/upgrade/keeper/keeper.go	`76.51% <0.00%> (-3.10%)`	⬇️
x/staking/types/msg.go	`54.92% <0.00%> (-2.40%)`	⬇️
x/staking/client/cli/tx.go	`17.98% <0.00%> (ø)`
x/upgrade/client/testutil/suite.go
x/staking/keeper/msg_server.go	`0.45% <0.00%> (+0.01%)`	⬆️

ryanchristo · 2021-05-16T15:54:32Z

Interesting idea. I think we don't need this at this level. The order of this relation is hidden from the user, and it's a storage detail. We use (granter, grantee, Authorization) because we need a prefix scan based on (granter, grantee).

Ok, just a thought. No need to change the order or the format for the triple.

A few items remaining:

There are two typos in the comment for the Grant method and we should probably be consistent with how we refer to the triple. See comment.
There is still one use of msg type rather than Authorization when referring to the triple that should probably be updated for consistency. See comment.
There are still a couple code snippets that do not start on the correct line (I understand the comment should not be included). See comment and comment.
If we intend to discuss the methods rather than the messages, I think we need to update some of the content. It is not clear that we are describing the service methods. See comment.

Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com>

robert-zaremba · 2021-05-17T14:47:20Z

Thanks for review @ryanchrypto . I've renamed the 3_messages file to 3_services, updated snippets and used Authorization instead of Msg Type when we talk about grant.

ryanchristo

I've renamed the 3_messages file to 3_services

In other module specifications, we describe the messages in a messages.md file and we do not have a services.md file. Do you think we should be breaking the pattern here?

The content for each section in the services.md file (renamed from messages.md) describes the messages. I am not sure why renaming each section to the service method names and then renaming the file to services.md are necessary changes if we are not updating the content to include explanations of the service methods.

Maybe it would make more sense if we added an explanation and a code snippet for each method? If we rename the file, we should probably use the singular tx_service.md rather than services.md unless we intend to include the query service.

I have added more suggestions. I also think we could do a better job throughout our specs and documentation on using consistent naming conventions (i.e. when to use backticks, how to refer to Protobuf services and messages, etc.). It would be nice to have a styleguide that everyone could reference when writing documentation, but this is out of scope for this pull request.

ryanchristo · 2021-05-17T15:35:40Z

x/authz/spec/03_services.md

+## Grant
+
+An authorization-grant is created using the `MsgGrant` message.


The "Grant" section does not include an explanation of Grant. It currently explains MsgGrant and provides a snippet of the MsgGrant message. This is what I was talking about when I said it was confusing to change the title of each section without changing the content.

Correct, we should start describing the method. I have updated the section. Hope it sound better now. (same for comments below)

ryanchristo · 2021-05-17T15:38:42Z

x/authz/spec/03_services.md

+## Revoke
+
+An grant can be removed with `MsgRevoke` message.


The "Revoke" section does not include an explanation of Revoke. It currently explains MsgRevoke and provides a snippet of the MsgRevoke message. This is what I was talking about when I said it was confusing to change the title of each section without changing the content.

ryanchristo · 2021-05-17T15:41:49Z

x/authz/spec/03_services.md

+## Exec
+
+When a grantee wants to execute transaction on behalf of a granter, it must send `MsgExec`.


The "Exec" section does not include an explanation of Exec. It currently explains MsgExec and provides a snippet of the MsgExec message. This is what I was talking about when I said it was confusing to change the title of each section without changing the content.

ryanchristo · 2021-05-17T15:50:54Z

x/authz/spec/03_services.md

+
+# Tx Msg Service
+
+In this section we describe the Protobuf Msg service for the authz module.


Suggested change

In this section we describe the Protobuf Msg service for the authz module.

In this section we describe the Protobuf Msg service for the `x/authz` module.

not sure about it. x/authz emphasis package name, "authz module" is a plain English. Do you have a preference?

That makes sense. I do not have a strong preference. This would be a good example of something to explain in a styleguide for writing documentation/specifications (maybe added to SPEC-SPEC).

x/authz/spec/01_concepts.md

x/authz/spec/03_services.md

robert-zaremba · 2021-05-18T09:29:37Z

In other module specifications, we describe the messages in a messages.md file and we do not have a services.md file. Do you think we should be breaking the pattern here?

This is what I'm proposing here. I think in recent discussions we want to promote the services oriented API.
@aaronc , @AmauryM - do you have an opinion here?

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

amaury1093 · 2021-05-18T11:37:56Z

This is what I'm proposing here. I think in recent discussions we want to promote the services oriented API.
@aaronc , @AmauryM - do you have an opinion here?

we do have a SPEC-SPEC here: https://github.com/cosmos/cosmos-sdk/blob/master/docs/spec/SPEC-SPEC.md. IMO we should strictly follow the spec-spec for now.

We can start a separate issue/discussion about whether we want to update SPEC-SPEC. I agree we should promote the service-oriented design. However, I still think it's useful to separate query services and msg services into 2 files, so I wouldn't call it 03_services.md. Maybe 03_query_service.md and 04_msg_service.md? Anyway, we should discuss this in a separate issue, and keep 03_messages.md in this PR.

cyberbono3

LGTM

ryanchristo · 2021-05-18T14:59:33Z

we do have a SPEC-SPEC here: https://github.com/cosmos/cosmos-sdk/blob/master/docs/spec/SPEC-SPEC.md. IMO we should strictly follow the spec-spec for now.

We can start a separate issue/discussion about whether we want to update SPEC-SPEC. I agree we should promote the service-oriented design. However, I still think it's useful to separate query services and msg services into 2 files, so I wouldn't call it 03_services.md. Maybe 03_query_service.md and 04_msg_service.md? Anyway, we should discuss this in a separate issue, and keep 03_messages.md in this PR.

This makes sense to me. I think @robert-zaremba was moving in the direction of making valid improvements but we should discuss updating the spec-spec document before changing the structure here.

aaronc · 2021-05-18T19:11:42Z

we do have a SPEC-SPEC here: https://github.com/cosmos/cosmos-sdk/blob/master/docs/spec/SPEC-SPEC.md. IMO we should strictly follow the spec-spec for now.
We can start a separate issue/discussion about whether we want to update SPEC-SPEC. I agree we should promote the service-oriented design. However, I still think it's useful to separate query services and msg services into 2 files, so I wouldn't call it 03_services.md. Maybe 03_query_service.md and 04_msg_service.md? Anyway, we should discuss this in a separate issue, and keep 03_messages.md in this PR.

This makes sense to me. I think @robert-zaremba was moving in the direction of making valid improvements but we should discuss updating the spec-spec document before changing the structure here.

I think this approach that @ryanchristo is suggesting makes sense

robert-zaremba · 2021-05-18T20:14:36Z

We discussed it during the stand up and arrived to the same conclusion

we will use messages oriented spec here
I created a task to update the spec and authz module spec: Update module spec towards service oriented architecture #9352 . Other modules can follow after that.

ryanchristo

Looks good to me! A few nitpicks but I approve.

x/authz/spec/03_messages.md

aleem1314

lgtm

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

robert-zaremba · 2021-05-19T13:31:48Z

The GitHub CI strikes again.

* update spec and fix code snippets * updating authz.Grant documentation * Update proto/cosmos/authz/v1beta1/tx.proto Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com> * rename messages to services * Update proto/cosmos/authz/v1beta1/tx.proto Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com> * update snippets * Apply suggestions from code review Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> * authz: update service docs to emphasis the service methods * authz spec: rollback to use message oriented spec * Apply suggestions from code review Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> * Update MsgGrant type doc Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com> Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

update spec and fix code snippets

678f617

robert-zaremba added T:Docs Changes and features related to documentation. C:x/authz labels May 14, 2021

robert-zaremba requested review from clevinson, ryanchristo, technicallyty and aleem1314 May 14, 2021 12:50

robert-zaremba requested review from aaronc and alexanderbez as code owners May 14, 2021 12:50

robert-zaremba added this to the v0.43 milestone May 14, 2021

robert-zaremba added the Status: Backlog label May 14, 2021

robert-zaremba removed this from the v0.43 milestone May 14, 2021

clevinson assigned ryanchristo May 14, 2021

updating authz.Grant documentation

af9aa24

ryanchristo reviewed May 14, 2021

View reviewed changes

robert-zaremba and others added 2 commits May 16, 2021 12:27

Update proto/cosmos/authz/v1beta1/tx.proto

2288c0a

Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com>

Apply suggestions from code review

06eb965

Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com>

robert-zaremba and others added 3 commits May 17, 2021 16:38

rename messages to services

049f785

Update proto/cosmos/authz/v1beta1/tx.proto

a858500

Co-authored-by: Ryan Christoffersen <12519942+ryanchrypto@users.noreply.github.com>

update snippets

22f0d28

robert-zaremba requested a review from ryanchristo May 17, 2021 14:47

Merge branch 'master' into robert/authz-spec

a8cf046

amaury1093 assigned cyberbono3 May 17, 2021

ryanchristo reviewed May 17, 2021

View reviewed changes

robert-zaremba and others added 2 commits May 18, 2021 11:33

Apply suggestions from code review

13d9a93

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

authz: update service docs to emphasis the service methods

9019b20

cyberbono3 self-requested a review May 18, 2021 11:32

cyberbono3 approved these changes May 18, 2021

View reviewed changes

authz spec: rollback to use message oriented spec

bc52fa2

robert-zaremba requested a review from ryanchristo May 18, 2021 21:01

ryanchristo approved these changes May 19, 2021

View reviewed changes

aleem1314 approved these changes May 19, 2021

View reviewed changes

robert-zaremba and others added 3 commits May 19, 2021 12:00

Apply suggestions from code review

c361433

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

Update MsgGrant type doc

913c3c2

Merge branch 'master' into robert/authz-spec

c6e210a

robert-zaremba added the A:automerge Automatically merge PR once all prerequisites pass. label May 19, 2021

github-actions bot added the C:x/slashing label May 19, 2021

robert-zaremba and others added 2 commits May 19, 2021 13:28

Merge branch 'master' into robert/authz-spec

a711248

Merge branch 'master' into robert/authz-spec

d99f863

mergify bot and others added 2 commits May 19, 2021 13:32

Merge branch 'master' into robert/authz-spec

3523195

Merge branch 'master' into robert/authz-spec

1767cdc

mergify bot merged commit b36664d into master May 19, 2021

mergify bot deleted the robert/authz-spec branch May 19, 2021 20:22

aaronc removed the Status: Backlog label May 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update spec and fix code snippets #9334

update spec and fix code snippets #9334

robert-zaremba commented May 14, 2021 •

edited

Loading

ryanchristo left a comment •

edited

Loading

ryanchristo May 14, 2021

robert-zaremba May 16, 2021

ryanchristo May 16, 2021 •

edited

Loading

robert-zaremba May 17, 2021 •

edited

Loading

robert-zaremba May 17, 2021

ryanchristo commented May 15, 2021 •

edited

Loading

robert-zaremba commented May 16, 2021

codecov bot commented May 16, 2021 •

edited

Loading

ryanchristo commented May 16, 2021 •

edited

Loading

robert-zaremba commented May 17, 2021

ryanchristo left a comment •

edited

Loading

ryanchristo May 17, 2021

robert-zaremba May 18, 2021

ryanchristo May 17, 2021

ryanchristo May 17, 2021

ryanchristo May 17, 2021

robert-zaremba May 18, 2021

ryanchristo May 18, 2021

robert-zaremba commented May 18, 2021

amaury1093 commented May 18, 2021

cyberbono3 left a comment

ryanchristo commented May 18, 2021

aaronc commented May 18, 2021

robert-zaremba commented May 18, 2021

ryanchristo left a comment

aleem1314 left a comment

robert-zaremba commented May 19, 2021

		## Grant

		An authorization-grant is created using the `MsgGrant` message.

		## Exec

		When a grantee wants to execute transaction on behalf of a granter, it must send `MsgExec`.


		# Tx Msg Service

		In this section we describe the Protobuf Msg service for the authz module.

update spec and fix code snippets #9334

update spec and fix code snippets #9334

Conversation

robert-zaremba commented May 14, 2021 • edited Loading

Description

ryanchristo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo May 16, 2021 • edited Loading

Choose a reason for hiding this comment

robert-zaremba May 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo commented May 15, 2021 • edited Loading

robert-zaremba commented May 16, 2021

codecov bot commented May 16, 2021 • edited Loading

Codecov Report

ryanchristo commented May 16, 2021 • edited Loading

robert-zaremba commented May 17, 2021

ryanchristo left a comment • edited Loading

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

Choose a reason for hiding this comment

robert-zaremba commented May 18, 2021

amaury1093 commented May 18, 2021

cyberbono3 left a comment

Choose a reason for hiding this comment

ryanchristo commented May 18, 2021

aaronc commented May 18, 2021

robert-zaremba commented May 18, 2021

ryanchristo left a comment

Choose a reason for hiding this comment

aleem1314 left a comment

Choose a reason for hiding this comment

robert-zaremba commented May 19, 2021

robert-zaremba commented May 14, 2021 •

edited

Loading

ryanchristo left a comment •

edited

Loading

ryanchristo May 16, 2021 •

edited

Loading

robert-zaremba May 17, 2021 •

edited

Loading

ryanchristo commented May 15, 2021 •

edited

Loading

codecov bot commented May 16, 2021 •

edited

Loading

ryanchristo commented May 16, 2021 •

edited

Loading

ryanchristo left a comment •

edited

Loading