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

Add support for custom encode/decode logic #1074

Open
stalniy opened this issue Feb 27, 2025 · 4 comments
Open

Add support for custom encode/decode logic #1074

stalniy opened this issue Feb 27, 2025 · 4 comments

Comments

@stalniy
Copy link

stalniy commented Feb 27, 2025

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 reversed

What

I need to customize logic of toBinary and fromBinary 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 and fromBinary 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 functions

Possible 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:

import { BinaryWriter, BinaryReader } from "@bufbuild/protobuf"

BinaryWriter.registerCustomType('Dec', (writer, field, msg) =>  ...);
BinaryReader.registerCustomType('Dec', (writer, field, msg) =>  ...);
@timostamm
Copy link
Member

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 gogoproto generated code will represent the value as a Dec, but I'd expect the field to be serialized as a Protobuf string in the binary and the ProtoJSON formats. Otherwise, it wouldn't be possible to parse a message from the wire with any Protobuf implementation except gogoproto, which seems like a really big drawback.

To support something similar in Protobuf-ES, the generated type for the field amount would need to change to Dec:

+ 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 Dec from.

In the runtime, it's already possible to get the custom type option, but something like BinaryWriter.registerCustomType wouldn't be sufficient. We also need to know how to initialize a Dec field, how to compare Dec values, and we need to know what a zero Dec value is. Implementing this is a very significant amount of work, and even for the most simple cases, users would need matching configuration at several places for it to work. It's also likely to introduce costs in perf, bundle size, and / or complexity. I'm afraid it seems unlikely that we can support this feature.

Provided that my assumption is correct that customtype = "Dec" doesn't change what's encoded on the wire, wouldn't it be possible to use something similar to math.NewDecFromString? After parsing a message from binary or JSON, you create a decimal object from the string value, and convert it back to a string before setting the new field value.

@stalniy
Copy link
Author

stalniy commented Feb 27, 2025

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 toBinary, fromBinary functions, to run this custom preprocessing before encoding this string to proto binary string format.

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 DecCoin type may be anywhere in data structure, this means I will need to manually track this, update when proto files are updated manually as well.

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 BinaryReader

@stalniy
Copy link
Author

stalniy commented Feb 27, 2025

Basically I need to do something similar as currently done for BigInt implementation. It can be either string or bigint type.

From another look I see this logic can be implemented on ReflectMessageImpl class which already has similar logic for bigint in JS.

UPD: it has both set and get methods which can be intercepted. The logic related to bigint can be extracted as a plugin as well. Very likely WKT and wrapper types also could be converted to pluggable custom types

@stalniy
Copy link
Author

stalniy commented Feb 28, 2025

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.

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

No branches or pull requests

2 participants