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

feat: Validate Msg proto annotations #13793

Merged
merged 65 commits into from
Mar 20, 2023
Merged

feat: Validate Msg proto annotations #13793

Show file tree
Hide file tree
Changes from 50 commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
e867834
wip
amaury1093 Nov 8, 2022
88cb125
feat: Validate Msg proto annotations
amaury1093 Nov 8, 2022
4e88fe6
Regenerate
amaury1093 Nov 8, 2022
419418a
Update
amaury1093 Nov 8, 2022
40b3958
Add comments
amaury1093 Nov 8, 2022
fe94644
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Nov 21, 2022
1ac9468
go mod tidy
amaury1093 Nov 21, 2022
db4cafe
wip merge fd sets
amaury1093 Nov 22, 2022
3b5d07d
fix gomods
amaury1093 Nov 28, 2022
168d125
wip
amaury1093 Nov 28, 2022
9981a0b
Fix nil any
amaury1093 Dec 1, 2022
59d9a83
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Dec 1, 2022
0dad494
Revert
amaury1093 Dec 1, 2022
2ae4914
Small cleanup
amaury1093 Dec 1, 2022
3b59f38
Move registration fix to types
amaury1093 Dec 1, 2022
e48ded0
revert some stuff
amaury1093 Dec 1, 2022
f0f2fd8
comments
amaury1093 Dec 1, 2022
0458789
Return err
amaury1093 Dec 1, 2022
6aea9ee
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Dec 5, 2022
708c5e9
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Dec 15, 2022
b2a0414
go mod tidy
amaury1093 Dec 15, 2022
22c6718
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Jan 16, 2023
3fcd4c0
Revert some stuff
amaury1093 Jan 18, 2023
43b0a04
Revert
amaury1093 Jan 18, 2023
61e9b54
revert
amaury1093 Jan 18, 2023
bd9dd66
Make it cleaner
amaury1093 Jan 18, 2023
39eaea5
Add tests
amaury1093 Jan 18, 2023
33f09ed
Add comments
amaury1093 Jan 19, 2023
64be084
Support MultiMsgsend
amaury1093 Jan 19, 2023
7152c27
Add comments
amaury1093 Jan 19, 2023
ab20001
Revert
amaury1093 Jan 19, 2023
dd1d67f
revert
amaury1093 Jan 19, 2023
e6e6a98
revert
amaury1093 Jan 19, 2023
cbeddfb
Don't panic
amaury1093 Jan 19, 2023
f0d7560
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Jan 31, 2023
489b2f3
go-mod-tidy-all
amaury1093 Jan 31, 2023
862cbec
Revert
amaury1093 Jan 31, 2023
a7823a0
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Feb 20, 2023
e5d877b
make proto-gen
amaury1093 Feb 20, 2023
a229a64
Use merged registry
amaury1093 Feb 20, 2023
767c236
Use os.StdErr
amaury1093 Feb 20, 2023
b02329e
Merge branch 'main' into am/13405-valid-annotations
amaury1093 Feb 20, 2023
4f84a83
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Mar 8, 2023
fbd9a35
Merge branch 'am/13405-valid-annotations' of ssh://github.com/cosmos/…
amaury1093 Mar 8, 2023
69a6ed4
Use MergedRegistry
amaury1093 Mar 8, 2023
b8e7bee
Use standalone validation
amaury1093 Mar 8, 2023
0ea3bd2
Merge branch 'main' into am/13405-valid-annotations
amaury1093 Mar 8, 2023
d226a51
Log instead of panic
amaury1093 Mar 8, 2023
719da5a
Merge branch 'am/13405-valid-annotations' of ssh://github.com/cosmos/…
amaury1093 Mar 8, 2023
aa62be7
revert
amaury1093 Mar 8, 2023
7fa3497
Move msg validation to GetSignersContext
amaury1093 Mar 13, 2023
55bf5d4
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Mar 13, 2023
b8435e2
Use commit hash for api
amaury1093 Mar 13, 2023
34fa561
Put in ProvideApp
amaury1093 Mar 13, 2023
b39b168
Fix build
amaury1093 Mar 13, 2023
0e20595
Remove replace
amaury1093 Mar 13, 2023
a302c5d
Fix test
amaury1093 Mar 13, 2023
e1042a1
Merge branch 'main' into am/13405-valid-annotations
amaury1093 Mar 13, 2023
c596277
Update x/tx/signing/get_signers.go
amaury1093 Mar 14, 2023
b6d287c
Merge branch 'main' into am/13405-valid-annotations
amaury1093 Mar 14, 2023
7c8b8b9
Merge branch 'main' of ssh://github.com/cosmos/cosmos-sdk into am/134…
amaury1093 Mar 20, 2023
90bdbe8
Fix build
amaury1093 Mar 20, 2023
2cb4fab
Return err
amaury1093 Mar 20, 2023
449d98c
Fix PR number in CL
amaury1093 Mar 20, 2023
cdfe4a9
Merge branch 'main' into am/13405-valid-annotations
amaury1093 Mar 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 15 additions & 15 deletions api/cosmos/consensus/v1/tx.pulsar.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

245 changes: 123 additions & 122 deletions api/cosmos/group/v1/tx.pulsar.go
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 3 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ require (
nhooyr.io/websocket v1.8.6 // indirect
)

// Here are the short-lived replace for the Cosmos SDK
replace cosmossdk.io/api => ./api

// Below are the long-lived replace of the Cosmos SDK
replace (
// use cosmos fork of keyring
Expand Down
2 changes: 0 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ cloud.google.com/go/storage v1.6.0/go.mod h1:N7U0C8pVQ/+NIKOBQyamJIeKQKkZ+mxpohl
cloud.google.com/go/storage v1.8.0/go.mod h1:Wv1Oy7z6Yz3DshWRJFhqM/UCfaWIRTdp0RXyy7KQOVs=
cloud.google.com/go/storage v1.10.0/go.mod h1:FLPqc6j+Ki4BU591ie1oL6qBQGu2Bl/tZ9ullr3+Kg0=
cloud.google.com/go/storage v1.14.0/go.mod h1:GrKmX003DSIwi9o29oFT7YDnHYwZoctc3fOKtUw0Xmo=
cosmossdk.io/api v0.3.1 h1:NNiOclKRR0AOlO4KIqeaG6PS6kswOMhHD0ir0SscNXE=
cosmossdk.io/api v0.3.1/go.mod h1:DfHfMkiNA2Uhy8fj0JJlOCYOBp4eWUUJ1te5zBGNyIw=
cosmossdk.io/collections v0.0.0-20230214153846-b6c6e4e99177 h1:At2M0aYQKEAWqr6JtZrJPOlfhdvENiGwJg2NCNHwIdc=
cosmossdk.io/collections v0.0.0-20230214153846-b6c6e4e99177/go.mod h1:uqCi8FG+Bh2vv/qf5xZ8iab0E0c/DMA/cwafbp8dkcU=
cosmossdk.io/core v0.6.0 h1:V2zyaMVFN6hJSVENYx2XE9CMhzqwJPMjzSQpj0MyXAU=
Expand Down
2 changes: 2 additions & 0 deletions proto/cosmos/consensus/v1/tx.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ option go_package = "github.com/cosmos/cosmos-sdk/x/consensus/types";

// Msg defines the bank Msg service.
service Msg {
option (cosmos.msg.v1.service) = true;

// UpdateParams defines a governance operation for updating the x/consensus_param module parameters.
// The authority is defined in the keeper.
//
Expand Down
2 changes: 1 addition & 1 deletion proto/cosmos/group/v1/tx.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ message MsgVoteResponse {}

// MsgExec is the Msg/Exec request type.
message MsgExec {
option (cosmos.msg.v1.signer) = "signer";
option (cosmos.msg.v1.signer) = "executor";
option (amino.name) = "cosmos-sdk/group/MsgExec";

// proposal is the unique ID of the proposal.
Expand Down
15 changes: 15 additions & 0 deletions runtime/app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package runtime
import (
"encoding/json"
"fmt"
"os"

abci "github.com/cometbft/cometbft/abci/types"
"golang.org/x/exp/slices"
Expand All @@ -23,7 +24,9 @@ import (
servertypes "github.com/cosmos/cosmos-sdk/server/types"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/types/msgservice"
authtx "github.com/cosmos/cosmos-sdk/x/auth/tx"
"github.com/cosmos/gogoproto/proto"
)

// App is a wrapper around BaseApp and ModuleManager that can be used in hybrid
Expand Down Expand Up @@ -112,6 +115,18 @@ func (a *App) Load(loadLatest bool) error {
}
}

// At startup, check that all proto annotations are correct.
protoFiles, err := proto.MergedRegistry()
if err != nil {
return err
}
err = msgservice.ValidateProtoAnnotations(protoFiles)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronc Is this a good place to put the proto annotations startup check?

Copy link
Member

Choose a reason for hiding this comment

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

hmm, should we be thinking how to use runtime in normal app.go settings or should we try to get people to migrate to depinject sooner?

Copy link
Member

Choose a reason for hiding this comment

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

Yes although we can probably do it even sooner in ProvideApp. We should probably pass *proto.Files into InterfaceRegistry and make it define the resolver interfaces and/or provide the resolver interfaces directly into the container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK i'll do that.

@tac0turtle for the old simapp way, I can probably add it somewhere on the bottom of NewSimApp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In 34fa561 I put this logic:

  • in ProvideApp for depinjected apps
  • in NewSimapp for legacy apps

We should probably pass *proto.Files into InterfaceRegistry and make it define the resolver interfaces and/or provide the resolver interfaces directly into the container.

With protov2, InterfaceRegistry should become legacy, right? I decided to do your 2nd proposal, expose those 2 resolver interfaces in the container.

if err != nil {
// Once we switch to using protoreflect-based antehandlers, we might
// want to panic here instead of logging a warning.
fmt.Fprintln(os.Stderr, err.Error())
}

return nil
}

Expand Down
8 changes: 8 additions & 0 deletions simapp/app_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/module"
"github.com/cosmos/cosmos-sdk/types/msgservice"
"github.com/cosmos/cosmos-sdk/x/auth"
"github.com/cosmos/cosmos-sdk/x/auth/vesting"
authzmodule "github.com/cosmos/cosmos-sdk/x/authz/module"
Expand Down Expand Up @@ -282,3 +283,10 @@ func TestMergedRegistry(t *testing.T) {
require.NoError(t, err)
require.Greater(t, r.NumFiles(), 0)
}

func TestProtoAnnotations(t *testing.T) {
r, err := proto.MergedRegistry()
require.NoError(t, err)
err = msgservice.ValidateProtoAnnotations(r)
require.NoError(t, err)
}
1 change: 1 addition & 0 deletions simapp/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ require (
// Replace here are pending PRs, or version to be tagged
replace (
// TODO tag all extracted modules after SDK refactor
cosmossdk.io/api => ../api
cosmossdk.io/tools/confix => ../tools/confix
cosmossdk.io/tools/rosetta => ../tools/rosetta
cosmossdk.io/x/evidence => ../x/evidence
Expand Down
2 changes: 0 additions & 2 deletions simapp/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,6 @@ cloud.google.com/go/webrisk v1.4.0/go.mod h1:Hn8X6Zr+ziE2aNd8SliSDWpEnSS1u4R9+xX
cloud.google.com/go/webrisk v1.5.0/go.mod h1:iPG6fr52Tv7sGk0H6qUFzmL3HHZev1htXuWDEEsqMTg=
cloud.google.com/go/workflows v1.6.0/go.mod h1:6t9F5h/unJz41YqfBmqSASJSXccBLtD1Vwf+KmJENM0=
cloud.google.com/go/workflows v1.7.0/go.mod h1:JhSrZuVZWuiDfKEFxU0/F1PQjmpnpcoISEXH2bcHC3M=
cosmossdk.io/api v0.3.1 h1:NNiOclKRR0AOlO4KIqeaG6PS6kswOMhHD0ir0SscNXE=
cosmossdk.io/api v0.3.1/go.mod h1:DfHfMkiNA2Uhy8fj0JJlOCYOBp4eWUUJ1te5zBGNyIw=
cosmossdk.io/client/v2 v2.0.0-20230220152935-67f04e629623 h1:QzZA1P+twvj10ylCVXzHodLge+RvrwoNIlWpU+MrRa0=
cosmossdk.io/client/v2 v2.0.0-20230220152935-67f04e629623/go.mod h1:Yo6R3XSXKxi+G642kK7+ZOh2czNTwTO1b8ywOvwaFQo=
cosmossdk.io/collections v0.0.0-20230214153846-b6c6e4e99177 h1:At2M0aYQKEAWqr6JtZrJPOlfhdvENiGwJg2NCNHwIdc=
Expand Down
6 changes: 6 additions & 0 deletions testutil/testdata/testpb/tx.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,23 @@ package testpb;

import "gogoproto/gogo.proto";
import "testpb/testdata.proto";
import "cosmos/msg/v1/msg.proto";

option go_package = "github.com/cosmos/cosmos-sdk/testutil/testdata";

// Msg tests the Protobuf message service as defined in
// https://github.com/cosmos/cosmos-sdk/issues/7500.
service Msg {
option (cosmos.msg.v1.service) = true;

rpc CreateDog(MsgCreateDog) returns (MsgCreateDogResponse);
}

message MsgCreateDog {
option (cosmos.msg.v1.signer) = "owner";

testpb.Dog dog = 1;
string owner = 2;
}

message MsgCreateDogResponse {
Expand Down
Loading