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

[x/group] Add amino JSON signing support and CLI #220

Merged
merged 112 commits into from
Feb 17, 2021
Merged

Conversation

blushi
Copy link
Member

@blushi blushi commented Jan 15, 2021

closes: #218
closes: #209 (not using ADR 031 approach for amino support)
closes: #197

@blushi blushi marked this pull request as ready for review February 12, 2021 07:57
Copy link
Contributor

@amaury1093 amaury1093 left a comment

Choose a reason for hiding this comment

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

Looks good, great job! Amino wiring looks good, but i'd like to see 1 test for peace of mind.

x/group/client/testsuite/cli_test.go Outdated Show resolved Hide resolved
x/group/module/module.go Outdated Show resolved Hide resolved
types/module/server/manager.go Outdated Show resolved Hide resolved
return nil, err
}

err = json.Unmarshal(contents, &members)
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that group.Member only has string subfields, encoding/json works here, but technically it should be clientCtx.JSONMarshaler.UnmarshalJSON, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

good point

Copy link
Member Author

Choose a reason for hiding this comment

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

the thing is that members a just a regular slice, not proto.Message so I guess we might need some Members proto type in this case

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yeah. That's annoying about protobuf, if you want an array of something you need to create a struct with an array subfield.

x/group/server/msg_server.go Show resolved Hide resolved
x/group/server/msg_server.go Outdated Show resolved Hide resolved
@@ -0,0 +1,2047 @@
package testsuite
Copy link
Contributor

Choose a reason for hiding this comment

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

2k lines for a file is becoming hard to navigate. How about splitting into 2 tests files: txs and queries?

return testutil.WriteToNewTempFile(s.T(), tx).Name()
}

func TestIntegrationTestSuite(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be useful to add one test using --sign-mode=amino-json, but to make sure the wiring works.

blushi and others added 2 commits February 17, 2021 10:09
Co-authored-by: Amaury <amaury.martiny@protonmail.com>
@blushi blushi mentioned this pull request Feb 17, 2021
Copy link
Contributor

@anilcse anilcse left a comment

Choose a reason for hiding this comment

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

@blushi @AmauryM I am going ahead and merge this. Let's have follow-up PRs for the review comments

@anilcse anilcse merged commit d19b384 into master Feb 17, 2021
@anilcse anilcse deleted the marie/218-group-amino branch February 17, 2021 10:36
@blushi blushi mentioned this pull request Feb 17, 2021
3 tasks
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.

[x/group] Support amino json signing [x/group] Add CLI support [x/group] Add integration tests
4 participants