Authz #2206

vuong177 · 2022-07-23T09:22:32Z

Closes: #2099

What is the purpose of the change

Apply API changes from fix: implement Amino serialization for x/authz and x/feegrant cosmos-sdk#224

Brief Changelog

(for example:)

RegisterLegacyAminoCodec in codec.go in every module. This allows the x/authz module to properly serialize and de-serialize MsgExec instances using Amino

Testing and Verifying

Add golang test in msgs_test.go. This test confirms authz serialize and de-serializes working right.
I created a testnet with the current mainnet state. My ledger can sign authz msg in this testnet.

p0mvn

Awesome work @vuong177 . Thank you so much for the tests, including on the testnet.

Had some minor comments. Please take a look

p0mvn · 2022-07-26T01:16:27Z

x/gamm/types/msgs_test.go

@@ -867,3 +872,148 @@ func TestMsgExitSwapShareAmountIn(t *testing.T) {
 		}
 	}
 }
+
+// Test authz


Please elaborate elaborate here that this test is meant more so to test the authz than the gamm message

p0mvn · 2022-07-26T01:17:50Z

x/gamm/types/msgs_test.go

+		gammMsg                    sdk.Msg
+	}{
+		{
+			expectedGrantSignByteMsg:   `{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgGrant","value":{"grant":{"authorization":{"type":"cosmos-sdk/GenericAuthorization","value":{"msg":"/osmosis.gamm.v1beta1.MsgExitSwapShareAmountIn"}},"expiration":"0001-01-01T02:01:01.000000001Z"},"grantee":"cosmos1def","granter":"cosmos1abc"}}],"sequence":"1","timeout_height":"1"}`,


Is there a way to have this represented in a struct format and the unmarshal to json string if needed?

It's hard to follow this in a string format.

Is there a way to have this represented in a struct format and the unmarshal to json string if needed?

It's hard to follow this in a string format.

I think it's a bit complex. Why we don't use pretty json in expectedGrantSignByteMsg. Like that

expectedGrantSignByteMsg : `{ "account_number":"1", "chain_id":"foo", "fee":{ "amount":[ ], "gas":"0" }, "memo":"memo", "msgs":[ { "type":"cosmos-sdk/MsgGrant", "value":{ "grant":{ "authorization":{ "type":"cosmos-sdk/GenericAuthorization", "value":{ "msg":"/osmosis.gamm.v1beta1.MsgExitSwapShareAmountIn" } }, "expiration":"0001-01-01T02:01:01.000000001Z" }, "grantee":"cosmos1def", "granter":"cosmos1abc" } } ], "sequence":"1", "timeout_height":"1" }`

instead of use one line string

Sounds good, thanks for suggesting the alternative

x/gamm/types/msgs_test.go

p0mvn · 2022-07-26T01:20:16Z

x/gamm/types/msgs_test.go

+		typeURL := sdk.MsgTypeURL(tc.gammMsg)
+		grant, err := authz.NewGrant(someDate, authz.NewGenericAuthorization(typeURL), someDate.Add(time.Hour))
+		require.NoError(t, err)
+		msgGrant := &authz.MsgGrant{Granter: "cosmos1abc", Grantee: "cosmos1def", Grant: grant}


can we extract cosmos1abc and cosmos1def to constants instead of hardcoding in multiple places, please?

x/superfluid/types/msg_test.go

p0mvn · 2022-07-26T01:22:31Z

x/superfluid/types/msg_test.go

+		msg                        sdk.Msg
+	}{
+		{
+			expectedGrantSignByteMsg:   `{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgGrant","value":{"grant":{"authorization":{"type":"cosmos-sdk/GenericAuthorization","value":{"msg":"/osmosis.superfluid.MsgLockAndSuperfluidDelegate"}},"expiration":"0001-01-01T02:01:01.000000001Z"},"grantee":"cosmos1def","granter":"cosmos1abc"}}],"sequence":"1","timeout_height":"1"}`,


Similar comment about making these structs and unmarshalling, if possible

x/tokenfactory/types/msgs_test.go

p0mvn · 2022-07-26T01:23:28Z

x/tokenfactory/types/msgs_test.go

+		msg                        sdk.Msg
+	}{
+		{
+			expectedGrantSignByteMsg:   `{"account_number":"1","chain_id":"foo","fee":{"amount":[],"gas":"0"},"memo":"memo","msgs":[{"type":"cosmos-sdk/MsgGrant","value":{"grant":{"authorization":{"type":"cosmos-sdk/GenericAuthorization","value":{"msg":"/osmosis.tokenfactory.v1beta1.MsgCreateDenom"}},"expiration":"0001-01-01T02:01:01.000000001Z"},"grantee":"cosmos1def","granter":"cosmos1abc"}}],"sequence":"1","timeout_height":"1"}`,


ditto: structs instead of strings

p0mvn · 2022-07-28T14:50:05Z

Hi @vuong177 . Have you gotten a chance to look at my comments? Would love to help you get this over the finish line as we are prioritizing this feature for the current sprint completion

vuong177 · 2022-07-28T16:01:07Z

@p0mvn sorry for my late response. I'm making this test cleaner.

p0mvn · 2022-07-28T16:29:48Z

@p0mvn sorry for my late response. I'm making this test cleaner.

No problem and no rush, just making sure that we are making progress here 👍

ValarDragon · 2022-07-31T22:37:11Z

Woah thanks for this PR!

I think somethings off about having this many test vectors for authz in every module. Whats the exact authz property were trying to test for every message?

What I was hoping would be the case is that we can have a suite.RunStandardMessageTest[M: sdk.Msg](msg M), and then authz having completeness could be tested there. But we don't need to care about the exact sign bytes, we can trust the authz module to get that correct

vuong177 · 2022-08-01T01:05:22Z

@ValarDragon This test is for checking serialize and de-serialize in MsgExec. Example in a gamm msg :

Current version:

{
  "account_number": "1",
  "chain_id": "foo",
  "fee": {
    "amount": [],
    "gas": "0"
  },
  "memo": "memo",
  "msgs": [
    {
      "type": "cosmos-sdk/MsgExec",
      "value": {
        "grantee": "cosmos1def",
        "msgs": [
          {
            "pool_id": "1",
            "sender": "osmo14kva2pk8xkpkal0v46yp4vsn27pylayq0zdlls",
            "share_in_amount": "100",
            "token_out_denom": "test",
            "token_out_min_amount": "100"
          }
        ]
      }
    }
  ],
  "sequence": "1",
  "timeout_height": "1"
}

This PR :

{
  "account_number": "1",
  "chain_id": "foo",
  "fee": {
    "amount": [],
    "gas": "0"
  },
  "memo": "memo",
  "msgs": [
    {
      "type": "cosmos-sdk/MsgExec",
      "value": {
        "grantee": "cosmos1def",
        "msgs": [
          {
            "type": "osmosis/gamm/exit-swap-share-amount-in",
            "value": {
              "pool_id": "1",
              "sender": "osmo14kva2pk8xkpkal0v46yp4vsn27pylayq0zdlls",
              "share_in_amount": "100",
              "token_out_denom": "test",
              "token_out_min_amount": "100"
            }
          }
        ]
      }
    }
  ],
  "sequence": "1",
  "timeout_height": "1"
}

ValarDragon · 2022-08-01T01:16:32Z

What confuses is me is that there should be some generic validation logic for checking this -- if there wasn't, then there wouldn't have been any failures.

Can we just re-use that method (or write it), and then ensure prior_deserialization(correct_authz_serialization(msg)) doesn't return an error?

vuong177 · 2022-08-01T02:20:20Z

What confuses is me is that there should be some generic validation logic for checking this -- if there wasn't, then there wouldn't have been any failures.

Can we just re-use that method (or write it), and then ensure prior_deserialization(correct_authz_serialization(msg)) doesn't return an error?

Oh great idea. I think we can do that. It makes this test more sense. Thank you @ValarDragon ! I'm going to make that.

ValarDragon · 2022-08-01T03:12:53Z

Awesome, thank you so much! That will become much more maintainable as well =)

ValarDragon · 2022-08-04T19:33:51Z

x/gamm/types/msgs_test.go

+			var (
+				mockMsgGrant  authz.MsgGrant
+				mockMsgRevoke authz.MsgRevoke
+				mockMsgExec   authz.MsgExec
+			)
+
+			// Authz: Grant Msg
+			typeURL := sdk.MsgTypeURL(tc.gammMsg)
+			grant, err := authz.NewGrant(someDate, authz.NewGenericAuthorization(typeURL), someDate.Add(time.Hour))
+			require.NoError(t, err)
+
+			msgGrant := authz.MsgGrant{Granter: mockGranter, Grantee: mockGrantee, Grant: grant}
+			msgGrantBytes := json.RawMessage(sdk.MustSortJSON(authzcodec.ModuleCdc.MustMarshalJSON(&msgGrant)))
+			err = authzcodec.ModuleCdc.UnmarshalJSON(msgGrantBytes, &mockMsgGrant)
+			require.NoError(t, err)
+
+			// Authz: Revoke Msg
+			msgRevoke := authz.MsgRevoke{Granter: mockGranter, Grantee: mockGrantee, MsgTypeUrl: typeURL}
+			msgRevokeByte := json.RawMessage(sdk.MustSortJSON(authzcodec.ModuleCdc.MustMarshalJSON(&msgRevoke)))
+			err = authzcodec.ModuleCdc.UnmarshalJSON(msgRevokeByte, &mockMsgRevoke)
+			require.NoError(t, err)
+
+			// Authz: Exec Msg
+			msgAny, _ := cdctypes.NewAnyWithValue(tc.gammMsg)
+			msgExec := authz.MsgExec{Grantee: mockGrantee, Msgs: []*cdctypes.Any{msgAny}}
+			execMsgByte := json.RawMessage(sdk.MustSortJSON(authzcodec.ModuleCdc.MustMarshalJSON(&msgExec)))
+			err = authzcodec.ModuleCdc.UnmarshalJSON(execMsgByte, &mockMsgExec)
+			require.NoError(t, err)
+			require.Equal(t, msgExec.Msgs[0].Value, mockMsgExec.Msgs[0].Value)
+		})


Can we move this all to apptesting function, and call it TestMessageSerialization ?

Should I rename package in msg_test.go from types to types_test ? This is to avoid import cycle error.

ah yeah, this makes sense to do

ValarDragon

Ty for doing this! Looks good to me!

Note to others, diff is only large due to the _testing package switch, which makes sense

Lets add a changelog and this is good to go?

vuong177 · 2022-08-05T14:55:39Z

I added changelog. Thanks for your reviews !

p0mvn

LGTM overall, nice work @vuong177

I'm wondering though if it makes sense to separate the types package name change so that we can backport it to v11.x. Please let me know what you think

p0mvn · 2022-08-05T15:21:13Z

app/apptesting/test_suite.go

+	require.NoError(t, err)
+
+	// Authz: Exec Msg
+	msgAny, _ := cdctypes.NewAnyWithValue(msg)


Let's handle the error please

p0mvn · 2022-08-05T15:23:56Z

x/gamm/types/msgs_test.go

 				msg.TokenOutMinAmount = sdk.NewInt(0)
 				return msg
 			}),
 			expectPass: false,
 		},
 		{
 			name: "negative amount criteria",
-			msg: createMsg(func(msg MsgSwapExactAmountIn) MsgSwapExactAmountIn {
+			msg: createMsg(func(msg gammtypes.MsgSwapExactAmountIn) gammtypes.MsgSwapExactAmountIn {


q: Would it make sense to split up pass-through changes into a separate PR so that we can backport it?

Otherwise, backporting other work can get bottlenecked on this later

cc: @ValarDragon

I agree with creating a new PR to change package name.

I'm going to create it.

ah thats a great point, should also be a really quick review + merge

Ty for doing that vuong!

Thank you @vuong177 , merged #2316

x/gamm/types/msgs_test.go

p0mvn · 2022-08-05T15:29:06Z

x/gamm/types/msgs_test.go

+	appParams.SetAddressPrefixes()
+	pk1 := ed25519.GenPrivKey().PubKey()
+	addr1 := sdk.AccAddress(pk1.Address()).String()
+	coin := sdk.NewCoin("stake", sdk.NewInt(1))


Suggested change

coin := sdk.NewCoin("stake", sdk.NewInt(1))

coin := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1))

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

ValarDragon · 2022-08-05T18:29:36Z

oh rip merge conflicts, can you try git merge main?

(Thanks for making the other PR off the osmosis repo, once this is on the repo, we can get these last steps handled with async round trip lol)

vuong177 · 2022-08-06T08:04:13Z

yes. No conflict anymore!

ValarDragon · 2022-08-06T19:25:18Z

Thanks for making this!-

vuong177 added 3 commits July 20, 2022 19:20

registerCodec for authz

6672f36

gamm msg: test authz grant revoke exec

de7ef7a

gamm msg: test authz grant revoke exec

b694cf8

github-actions bot added C:x/gamm Changes, features and bugs related to the gamm module. C:x/incentives C:x/lockup C:x/superfluid labels Jul 23, 2022

vuong177 added 3 commits July 24, 2022 19:43

test authz for incentives module

415cb39

test authz for superfluid module

f73dd97

test authz for tokenfactory module

00ea208

github-actions bot added the C:x/tokenfactory label Jul 24, 2022

vuong177 marked this pull request as ready for review July 24, 2022 22:33

vuong177 requested a review from a team July 24, 2022 22:33

p0mvn reviewed Jul 26, 2022

View reviewed changes

vuong177 added 6 commits July 29, 2022 08:55

gamm test clean

89a04f9

incentives test clean

791b5b5

superfluid test clean

a00b4c5

tokenfactory test clean

0f33897

tokenfactory

6506326

lockup test

f433f3b

vuong177 mentioned this pull request Jul 30, 2022

amino encoding not enabled for tokenfactory #2259

Closed

refactor test for more maintainable

2760edf

Update msgs_test.go

f086c52

ValarDragon reviewed Aug 4, 2022

View reviewed changes

refactor authz test in gamm module

2a47d22

github-actions bot added the C:app-wiring Changes to the app folder label Aug 5, 2022

vuong177 and others added 7 commits August 5, 2022 13:42

Merge branch 'main' into authz

eec4579

minor lint

ccada40

incentives

a221d02

lockup

dd2d1f6

minor

17391d6

superfluid

11344a6

token factory

1ad2e65

ValarDragon approved these changes Aug 5, 2022

View reviewed changes

add changelog

f9f5fbd

github-actions bot added the C:docs Improvements or additions to documentation label Aug 5, 2022

p0mvn reviewed Aug 5, 2022

View reviewed changes

vuong177 and others added 4 commits August 6, 2022 00:14

Update x/gamm/types/msgs_test.go

f3cebe9

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

minor

f047f32

fumpt

187b95f

handler err when create msgAny

73de3b6

Merge branch 'main' into authz

5e13222

ValarDragon merged commit 9bc52d4 into osmosis-labs:main Aug 6, 2022

czarcas7ic mentioned this pull request Aug 19, 2022

Gather v12 QA items #2444

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authz #2206

Authz #2206

vuong177 commented Jul 23, 2022 •

edited

Loading

p0mvn left a comment

p0mvn Jul 26, 2022

p0mvn Jul 26, 2022

vuong177 Jul 28, 2022

p0mvn Jul 28, 2022

p0mvn Jul 26, 2022

p0mvn Jul 26, 2022

p0mvn Jul 26, 2022

p0mvn commented Jul 28, 2022

vuong177 commented Jul 28, 2022 •

edited

Loading

p0mvn commented Jul 28, 2022

ValarDragon commented Jul 31, 2022

vuong177 commented Aug 1, 2022 •

edited

Loading

ValarDragon commented Aug 1, 2022

vuong177 commented Aug 1, 2022 •

edited

Loading

ValarDragon commented Aug 1, 2022

ValarDragon Aug 4, 2022 •

edited

Loading

vuong177 Aug 5, 2022

vuong177 Aug 5, 2022

ValarDragon Aug 5, 2022

ValarDragon left a comment

vuong177 commented Aug 5, 2022

p0mvn left a comment

p0mvn Aug 5, 2022

p0mvn Aug 5, 2022

vuong177 Aug 5, 2022

vuong177 Aug 5, 2022

ValarDragon Aug 5, 2022 •

edited

Loading

p0mvn Aug 5, 2022

p0mvn Aug 5, 2022

ValarDragon commented Aug 5, 2022

vuong177 commented Aug 6, 2022

ValarDragon commented Aug 6, 2022

	coin := sdk.NewCoin("stake", sdk.NewInt(1))
	coin := sdk.NewCoin(sdk.DefaultBondDenom, sdk.NewInt(1))

Authz #2206

Authz #2206

Conversation

vuong177 commented Jul 23, 2022 • edited Loading

What is the purpose of the change

Brief Changelog

Testing and Verifying

p0mvn left a 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

Choose a reason for hiding this comment

p0mvn commented Jul 28, 2022

vuong177 commented Jul 28, 2022 • edited Loading

p0mvn commented Jul 28, 2022

ValarDragon commented Jul 31, 2022

vuong177 commented Aug 1, 2022 • edited Loading

ValarDragon commented Aug 1, 2022

vuong177 commented Aug 1, 2022 • edited Loading

ValarDragon commented Aug 1, 2022

ValarDragon Aug 4, 2022 • 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

ValarDragon left a comment

Choose a reason for hiding this comment

vuong177 commented Aug 5, 2022

p0mvn left a 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

ValarDragon Aug 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ValarDragon commented Aug 5, 2022

vuong177 commented Aug 6, 2022

ValarDragon commented Aug 6, 2022

vuong177 commented Jul 23, 2022 •

edited

Loading

vuong177 commented Jul 28, 2022 •

edited

Loading

vuong177 commented Aug 1, 2022 •

edited

Loading

vuong177 commented Aug 1, 2022 •

edited

Loading

ValarDragon Aug 4, 2022 •

edited

Loading

ValarDragon Aug 5, 2022 •

edited

Loading