feat: add message option extension to signal msg signers in a language agnostic way #10977

fdymylja · 2022-01-19T13:47:31Z

Description

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

fdymylja · 2022-01-19T13:50:32Z

This PR includes only proto changes and generated code changes.

I have already written the code that does runtime check but I haven't pushed it yet.

The runtime code asserted that every signer is correctly formed.

The only thing I'd need right now for the review is to make sure that cosmos.msg.signer marks the correct field as the GetSigners() implementation.

I mean I've checked it multiple times but an extra pair of eyes helps.

…fdymylja/agnostic-signers

codecov · 2022-01-19T14:23:06Z

Codecov Report

Merging #10977 (025e02a) into master (95e65fe) will decrease coverage by 0.11%.
The diff coverage is 79.76%.

@@            Coverage Diff             @@
##           master   #10977      +/-   ##
==========================================
- Coverage   65.95%   65.84%   -0.12%     
==========================================
  Files         647      644       -3     
  Lines       65873    65630     -243     
==========================================
- Hits        43444    43211     -233     
+ Misses      19941    19939       -2     
+ Partials     2488     2480       -8

Impacted Files	Coverage Δ
crypto/armor.go	`83.08% <ø> (ø)`
x/gov/keeper/grpc_query.go	`73.01% <ø> (-0.29%)`	⬇️
x/gov/keeper/vote.go	`92.50% <ø> (+2.17%)`	⬆️
x/gov/types/v1beta2/msgs.go	`63.69% <ø> (ø)`
x/nft/keeper/grpc_query.go	`69.38% <60.41%> (-1.51%)`	⬇️
crypto/xsalsa20symmetric/symmetric.go	`65.71% <65.71%> (ø)`
x/upgrade/abci.go	`92.15% <84.61%> (-2.58%)`	⬇️
x/nft/keeper/keys.go	`96.22% <90.90%> (-3.78%)`	⬇️
x/gov/abci.go	`90.29% <100.00%> (-2.92%)`	⬇️
x/gov/keeper/tally.go	`91.20% <100.00%> (ø)`
... and 15 more

fdymylja · 2022-01-19T14:24:03Z

Anyone knows why this test might be failing? First time seeing this.

https://github.com/cosmos/cosmos-sdk/runs/4868894745?check_suite_focus=true

aaronc · 2022-01-19T14:55:36Z

proto/cosmos/msg/v1beta1/msg.proto

+  // The field must either be of string kind, or of message
+  // kind in case the signer information is contained within
+  // a message inside the cosmos message.
+  repeated string signers = 11110000;


Suggested change

repeated string signers = 11110000;

repeated string signer = 11110000;

This is more intuitive naming given how repeated options are declared

aaronc · 2022-01-19T14:56:19Z

proto/cosmos/msg/v1beta1/msg.proto

@@ -0,0 +1,22 @@
+syntax="proto3";
+
+package cosmos.msg.v1beta1;


Suggested change

package cosmos.msg.v1beta1;

package cosmos.msg.v1beta1;

Let's change this to v1 before release (or change it now)

fdymylja · 2022-01-21T14:23:04Z

Tests added, and added more docs.

We have no real way to test the protoregistry part due to how gogoproto registry works.

The policy in case of missing/malformed signers is a panic.

I was wondering if we should relax it or use a global to set it from panic to warning, reason being unexpected panics due to some unforeseen errors in the registry.

cc: @aaronc, @AmauryM

fdymylja · 2022-01-25T11:56:08Z

Putting on draft again in order to try to resolve protobuf filenames conflicts in a more elegant way.

aaronc · 2022-01-25T14:49:04Z

What if we just merge the annotations in this PR and deal with the validation in a follow up?

# Conflicts: # x/gov/types/v1beta2/tx.pb.go

fdymylja · 2022-01-27T13:54:55Z

Opening this again for review, removed the validation part as doing the gogoproto registry fix alongside this PR requires too much dependency synchronisation.

cc: @aaronc, @AmauryM, @marbar3778

types/module/configurator.go

tac0turtle

utACK

tac0turtle · 2022-01-28T09:37:08Z

x/upgrade/keeper/keeper.go

+	storetypes "github.com/cosmos/cosmos-sdk/store/types"
+


Suggested change

storetypes "github.com/cosmos/cosmos-sdk/store/types"

storetypes "github.com/cosmos/cosmos-sdk/store/types"

Co-authored-by: Marko <marbar3778@yahoo.com>

amaury1093 · 2022-01-28T12:51:03Z

proto/cosmos/staking/v1beta1/tx.proto

+  // if validator_address == delegator_address then only one
+  // is expected to sign, otherwise both are.
+  option (cosmos.msg.v1.signer) = "delegator_address";
+  option (cosmos.msg.v1.signer) = "validator_address";


What do you think of option (cosmos.msg.v1.signer) = "delegator_address,validator_address"; which would be similar to e.g. the orm indices syntax

Considering the instances in which we have multiple signers, I'd keep it this way, also because the decision to rename the field signers to signer was mainly driven by the most common use case.

But no strong opinion otherwise, let me know.

aaronc · 2022-01-28T13:13:48Z

I'd like to take some time in today's architecture call to present this and other pulsar/app wiring related changes that are coming

fdymylja added 5 commits January 19, 2022 13:18

add: signers proto and modify authz

fd1f255

add: signers in bank

10c556f

add: signers in crisis, distribution, evidence, feegrant, group

6dfa14c

add: signers in nft, slashing, staking.

d2969d6

add: vesting and gov, finalise codegen

35460b2

github-actions bot added C:x/auth C:x/authz C:x/bank C:x/crisis C:x/distribution distribution module related C:x/evidence C:x/feegrant C:x/gov C:x/nft C:x/slashing C:x/staking labels Jan 19, 2022

fdymylja changed the title ~~add: message option extension to signal msg signers in a language agnostic way~~ feat: message option extension to signal msg signers in a language agnostic way Jan 19, 2022

Merge branch 'master' into fdymylja/agnostic-signers

876716d

fdymylja changed the title ~~feat: message option extension to signal msg signers in a language agnostic way~~ feat: add message option extension to signal msg signers in a language agnostic way Jan 19, 2022

fdymylja added 3 commits January 19, 2022 15:01

add: fix go.sum in api module

3cd815a

Merge remote-tracking branch 'origin/fdymylja/agnostic-signers' into …

7eefcee

…fdymylja/agnostic-signers

add: msg.pulsar.go file

f77d8a0

aaronc reviewed Jan 19, 2022

View reviewed changes

fdymylja added 4 commits January 19, 2022 16:29

change: address pr comments

1b3bd0c

remove: old proto generated gofile

e167f1a

add: assert signers

fb3bf33

chore: extra docs on hacks...

dd279fd

fdymylja requested review from aaronc and amaury1093 January 21, 2022 14:23

chore: update CHANGELOG.md

ace8e16

fdymylja marked this pull request as draft January 25, 2022 11:56

Merge branch 'master' into fdymylja/agnostic-signers

7e0c199

# Conflicts: # x/gov/types/v1beta2/tx.pb.go

github-actions bot added C:CLI C:orm C:x/upgrade labels Jan 27, 2022

chore: regen files and remove verification

d7e5d13

github-actions bot removed the C:orm label Jan 27, 2022

fdymylja marked this pull request as ready for review January 27, 2022 13:53

fdymylja requested a review from tac0turtle January 27, 2022 13:53

tac0turtle reviewed Jan 28, 2022

View reviewed changes

types/module/configurator.go Outdated Show resolved Hide resolved

tac0turtle approved these changes Jan 28, 2022

View reviewed changes

fdymylja and others added 3 commits January 28, 2022 10:40

Update types/module/configurator.go

17ae06e

Co-authored-by: Marko <marbar3778@yahoo.com>

merge

1eb0ac7

rever configurator.go changes

025e02a

amaury1093 approved these changes Jan 28, 2022

View reviewed changes

fdymylja added the A:automerge Automatically merge PR once all prerequisites pass. label Jan 28, 2022

mergify bot merged commit 3a1027c into master Jan 28, 2022

mergify bot deleted the fdymylja/agnostic-signers branch January 28, 2022 17:01

aleem1314 mentioned this pull request Aug 1, 2022

feat(app)!: cosmos SDK v0.46 upgrade regen-network/regen-ledger#1244

Merged

19 tasks

aaronc mentioned this pull request Sep 27, 2022

Require cosmos.msg.v1 annotations #13405

Closed

crodriguezvega mentioned this pull request May 22, 2023

Use option extension to signal message signers in protobuf files cosmos/ibc-go#3624

Closed

charleenfei mentioned this pull request Aug 21, 2023

Check MsgChannelUpgradeInit is signed by authority cosmos/ibc-go#4186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add message option extension to signal msg signers in a language agnostic way #10977

feat: add message option extension to signal msg signers in a language agnostic way #10977

fdymylja commented Jan 19, 2022

fdymylja commented Jan 19, 2022

codecov bot commented Jan 19, 2022 •

edited

Loading

fdymylja commented Jan 19, 2022

aaronc Jan 19, 2022

aaronc Jan 19, 2022

fdymylja commented Jan 21, 2022

fdymylja commented Jan 25, 2022

aaronc commented Jan 25, 2022

fdymylja commented Jan 27, 2022

tac0turtle left a comment

tac0turtle Jan 28, 2022

amaury1093 Jan 28, 2022

fdymylja Jan 28, 2022

aaronc commented Jan 28, 2022

	repeated string signers = 11110000;
	repeated string signer = 11110000;

	storetypes "github.com/cosmos/cosmos-sdk/store/types"
	storetypes "github.com/cosmos/cosmos-sdk/store/types"

feat: add message option extension to signal msg signers in a language agnostic way #10977

feat: add message option extension to signal msg signers in a language agnostic way #10977

Conversation

fdymylja commented Jan 19, 2022

Description

Author Checklist

Reviewers Checklist

fdymylja commented Jan 19, 2022

codecov bot commented Jan 19, 2022 • edited Loading

Codecov Report

fdymylja commented Jan 19, 2022

aaronc Jan 19, 2022

Choose a reason for hiding this comment

aaronc Jan 19, 2022

Choose a reason for hiding this comment

fdymylja commented Jan 21, 2022

fdymylja commented Jan 25, 2022

aaronc commented Jan 25, 2022

fdymylja commented Jan 27, 2022

tac0turtle left a comment

Choose a reason for hiding this comment

tac0turtle Jan 28, 2022

Choose a reason for hiding this comment

amaury1093 Jan 28, 2022

Choose a reason for hiding this comment

fdymylja Jan 28, 2022

Choose a reason for hiding this comment

aaronc commented Jan 28, 2022

codecov bot commented Jan 19, 2022 •

edited

Loading