-
Notifications
You must be signed in to change notification settings - Fork 336
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
Code Generator for Go Types #1805
Conversation
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.
Very very nice!
We need naming convension conversion for acronyms. E.g. pub struct Ibc3ChannelOpenResponse
vs. type IBC3ChannelOpenResponse struct
or field names pub port_id: String
vs. PortID string
. Maybe a map which converts words to uppercase (Ibc
, Id
, ..)
match ty { | ||
"Uint128" => Some("string"), | ||
"Binary" => Some("[]byte"), | ||
"HexBinary" => Some("Checksum"), |
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.
This should be string
by default and only Checksum
in cases where we have a 32 byte checksum.
Do we have a way to annotate this somehow in Rust?
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.
That's a bit of a problem. What we could do is use #[schemars(title = "Checksum")]
on the field. That would set metadata.title
for the field in the schema.
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.
Oh but what we also do is change CodeInfoResponse::checksum
from HexBinary
to cosmwasm_std::Checksum
. With that we can map
// both hex strings in JSON
"HexBinary" => Some("string"),
"Checksum" => Some("Checksum"),
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.
Unfortunately, that only works if we make Checksum
a newtype like struct Checksum(HexBinary)
, which would be a contract-breaking change for CodeInfoResponse
.
Type aliases are already resolved to their underlying types at the json schema level (type Checksum = HexBinary
becomes HexBinary
in the schema).
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.
Yeah, I'm thinking about a source breaking change here. In contrast to HexBinary, Checksum would guarantee to store 32 bytes. We have Checksum already and would just have to add serde.
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.
Oh, I see now, we have Checksum in cosmwasm-vm
. Yeah, that seems like a good solution and would even make the type a bit nicer (as you mentioned, guaranteed 32 bytes, but also the docs specify that it is sha256, etc.).
But that means this would have to wait for 2.0, right?
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.
we have Checksum in
cosmwasm-vm
.
Ah, right. I did not realize we don't have it in cosmwasm-std yet. But yes, this can be the same type by moving it to cosmwasm-std and adding serde support.
wait for 2.0, right?
Yes. In the meantime we can keep "HexBinary" => Some("Checksum"),
because this is only used in one place.
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.
Alright, I created an issue for that: #1835
Anything left to do here, or can we merge?
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.
🔥🔥🔥 Now we're talking
packages/go-gen/tests/cosmwasm_std__AllDenomMetadataResponse.go
Outdated
Show resolved
Hide resolved
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.
Very nice! Let's merge here and see if that is useful for #1788
This adds a code generator for the Go types we need in
wasmvm
. It usescosmwasm-schema
to generate json schema from a type and traverses that to generate Go code from it.I'm open to refactoring recommendations. Some of it is a bit messy (though that's mostly because of the
schemars
api and the many cases that have to be covered)