-
Notifications
You must be signed in to change notification settings - Fork 76
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
Add support for custom encode/decode logic #1074
Comments
Hey Serhii, do these custom types really change how fields are encoded on the wire? string amount = 2 [
(cosmos_proto.scalar) = "cosmos.Dec",
(gogoproto.customtype) = "Dec",
(gogoproto.nullable) = false
]; I expect that To support something similar in Protobuf-ES, the generated type for the field + import type { Dec } from "??";
/**
* @generated from field: string amount = 2;
*/
- amount: string;
+ amount: Dec; So it's also necessary to support the custom type option in protoc-gen-es. Because the option doesn't provide any information where to import the type from, a plugin option is necessary to tell protoc-gen-es where to import In the runtime, it's already possible to get the custom type option, but something like Provided that my assumption is correct that |
You are right, the actual representation on a wire doesn't change. It's still a string but this string is pre-processed (i.e., formatted) in a special way. Basically Dec type just formats the number from decimal to bigInt and on decode it does reverse operation. So, this doesn't require any changes in types or generated code. I assume that the only change is needed in Basically, I'd say that you should not implement support for cosmos sdk Dec type but just implement extension point that would allow app or library developers to add this support without hassle. I can't just do this in runtime manually, because I'm working on data level. And this So, maybe the solution can be much simpler like adding interceptors on value when it's encoded/decoded. Basically possibility to provide custom transformation functions for every type: BinaryWriter.addIntercepter((field: DescField, value: unknown) => {
if (field.options.some(....)) { //check whether it's a Dec field
return decToBigInt(value);
}
return value;
}); and similar thing for |
Basically I need to do something similar as currently done for BigInt implementation. It can be either From another look I see this logic can be implemented on UPD: it has both |
Well, I personally don’t like the idea of such transformations. And I think it should not spread over different protobuf implementations. Instead it needs to be properly done in cosmos-sdk. There is even a bug for this (2years old). Though would be good to have at least extension point to implement it on my side. |
Yes, the reflection layer can potentially adapt to different representations of a message. Support for this hasn't been fully fleshed out though, and it will likely require significant effort to fully support different use cases. But circling back to #1074 (comment): The generated type for a modified field needs to change as well. This isn't supported by I wonder how Cosmos solves this problem for Go? Do they maintain their own generator and marshaller? |
I don’t know the exact implementation in Golang. My understanding (which may not correlate with reality) is that they do it in the way you described in your comment, with custom type representation. And yes, there is a separate generator. Probably it’s specifically designed for blockchain usage. In JS, there is telescope project which supports DecCoin type and customtype option. But in telescope, they do conversion inside decode & encode methods of that specific type. To the end client developer it’s the same type as in protobuf. It’s just a string |
It looks like cosmos is using a fork of gogoprotobuf (deprecated, see gogo/protobuf#691), which is a fork of golang/protobuf. Telescope appears to use a fork of ts-proto, with custom generated code that parses the Protobuf |
Custom field representations are a great idea. But let's take a look at the field option:
The code generator needs to know what type to generate for this field (and where to import the type from), and the runtime needs to know how to convert it (the generator needs provide a piece of code to the runtime). This means the option needs to provide more information specifically for TypeScript, for example:
This will still lead to unexpected behavior though. For example, you can provide a partial message to If my understanding from #1074 (comment) is correct, my recommendation is to use regular generated code from protobuf-es, then use |
yes, telescope parses it with Decimal but then it converts it back to string :) That's why to the client developer it's still a string. And there is no need for generator to provide something custom. The only thing which needs to be changed is a runtime a possibility to hook into encode/decode logic for scalars or all types. Having some hook on reflection level would allow me to write this same logic without introducing any issues. Anyway, I plan to take a look into this more deeply in the next few days probably I will understand the complexity you are trying to avoid but from the bird flight point of view it seems not hard. |
I'm confused 😆 Is this just for validation, or does it modify the string? |
It modifies the string. So, on clients it’s a string that represents double BUT when it’s sent over wire it’s a string that represents BigInt with 18 numbers. So this is just a trick over wire, and I guess it’s specifically for GoLang 😄 so, yeah it’s super awkward |
Why
For example, in CosmosSDK DecCoin amount uses custom type implementation (i.e.,
(gogoproto.customtype) = "Dec"
). This custom type changes how field is pre-processed on wire level. Basically instead of encoding it as just a string they preprocess it, convert from decimal string to bigint string and then encode as a string on protobuf level. On decode, operation is reversedWhat
I need to customize logic of
toBinary
andfromBinary
functions to support custom encode/decode logic for some fields would be nice to have this out of the box or have possibility to implement this. This would allow me to reuse existing libraries like@connectrpc/connect
to send grpc requests to my server.Alternatives
Potentially I can implement a wrapper function on top of
toBinary
andfromBinary
but then this will require me to change every dependent code and basically loose possibility to re-use existing open source libraries that rely on the same functionsPossible implementation
protogen-es plugin generate base64 encoded AST for proto messages which can be enhanced with special field (
customType
). This customType implementation can be provided in runtime for example like this:The text was updated successfully, but these errors were encountered: