-
Notifications
You must be signed in to change notification settings - Fork 383
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
chore: In Amino, use ToLowerSnakeCase for Protobuf field names #1213
chore: In Amino, use ToLowerSnakeCase for Protobuf field names #1213
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1213 +/- ##
==========================================
- Coverage 55.93% 47.16% -8.77%
==========================================
Files 420 372 -48
Lines 65415 61715 -3700
==========================================
- Hits 36592 29110 -7482
- Misses 25966 30220 +4254
+ Partials 2857 2385 -472
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hey @jefft0, could you share a before/after |
I updated the description with the changes to the test .proto output of genproto_test.go . |
Ok, @jefft0, I apologize for asking multiple small things separately. Could you please provide a before/after example of a generated Go file using the tool you use (buf, gogo, or protoc-gen-go)? As far as I remember, the generated Go code should be mostly the same, as snake case is translated into Go-style variable naming. However, we have a concern regarding other fields, such as JSON in Go tags ( In the main repository, I don't think it will affect anything since we directly use amino marshalling. However, for some clients, it might create incompatible JSON clients. |
I updated the description with before and after generated Go structs. |
Based on my understanding, it appears that our server-side case will remain unaffected since we utilize amino instead of protobuf to expose our API. Therefore, when an RPC/HTTP/REST request is made using JSON as the format, the resulting output will remain unchanged (CamelCase). However, clients who previously used protobuf-generated JS clients will now anticipate snake_case keys, whereas we currently provide CamelCase keys. Would it be possible to consider incorporating protobuf field options to ensure that JSON keys are formatted in CamelCase? |
306c473
to
3868f12
Compare
We can update the Protobuf output to be:
In this case, the Go struct that buf creates is:
Will that work? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jefft0 The change seems solid. However, we’d like to further understand the impact on existing transactions after replacing the current proto files. For instance, if we substitute vm.proto, would it be possible to use the new client to make calls to test3.gno.land? Also, will the node still be capable of unmarshalling the existing call messages and addpkg messages in the genesis transactions?
We might not necessarily require backward compatibility at this stage, but we aim to understand the current implications to make informed decisions later.
@piux2 : I restarted my local gno.land node from genesis. I loads all the genesis transactions and I see the initial state of the "boards" realm. (This is not surprising. The changes in this PR don't affect reading existing messages.) I can try changing vm.proto. What is the correct command to regenerate it using Amino? Is it to do |
3868f12
to
29e79b8
Compare
@jefft0 that is correct command. it will generate vm.proto according to the package.go in the folder. We want to verify
|
Hello @piux2 . Here is the generated vm.proto:
The newly-built gno.land does start from genesis. I'm able to use the command-line gnokey to query account balances (both local and on test3.gno.land), and the web interface works. To test if the newly-built gnokey can call contracts on test3.gno.land, I ran the command:
As expected it prints:
Is this the right way to test calling a contract on test3.gno.land? |
29e79b8
to
bcbea26
Compare
@jefft0 thank you for running the tests. We want to verify the gnokey call | send and gnokey addpkg -deposit against the test3 |
Signed-off-by: Jeff Thompson <[email protected]>
Signed-off-by: Jeff Thompson <[email protected]>
Signed-off-by: Jeff Thompson <[email protected]>
Signed-off-by: Jeff Thompson <[email protected]>
…e(field.Name)) Signed-off-by: Jeff Thompson <[email protected]>
Signed-off-by: Jeff Thompson <[email protected]>
bcbea26
to
4a4900a
Compare
✅ Deploy Preview for gno-docs2 canceled.
|
As before, I start with a fresh local node with
As expected, the CreateBoard command prints "(1 gno.land/r/demo/boards2.BoardID)", and the local web page http://localhost:8888/r/demo/boards2 shows the created board "test_snake_case". The above commands correctly use |
Hi @piux2 . Are you saying that you want me to use the code in this PR as the client to call the test3 node, to check backwards compatibility? I ask because when I run the client against a local node which is also compiled with the code in this PR, then the functions mentioned in my previous post all work. But you want to check backwards compatibility between different versions? |
We're on our thursday review calls; the answer is yes, that's what we're looking for :) |
Hi @piux2 . I got some GNOT on test3.gno.land and entered the following. (My account was already registered with the node, so I didn't run the Register command again.)
As expected, the CreateBoard command prints "(1 gno.land/r/demo/jefft0_test2_boards.BoardID)" and you can see the result. To test
I enter:
Now the piux2 account has 1000000000ugnot . (You're welcome.) In summary, these commands show the |
This PR resolves issue #1211 so that Amino uses snake case for Protobuf field names, as required by the official style guide. Otherwise the Protobuf linter in our CI workflow outputs many warnings which get in the way of seeing issues that need to be fixed.
We copy
ToLowerSnakeCase
from the buf repository which is the same code that the linter uses to produce the expected field name. If the Go struct field isPackagePath
then the Protobuf field should bepackage_path
. We simply updateGenerateProto3MessagePartial
to useToLowerSnakeCase(field.Name)
when outputting the Protobuf field name. For backwards compatibility with existing tools that generate JSON from the Protobuf file, we include[json_name = "OriginalCamelCase"]
where needed.You can see the difference by the changes in genproto_test.go. Before:
After:
Here is the difference in the Go structs for Protobuf that buf creates. Before:
After: