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

Feat: respect gogoproto nullable #2

Merged
merged 7 commits into from
Nov 11, 2024
Merged

Conversation

pr0n00gler
Copy link
Collaborator

@pr0n00gler pr0n00gler commented Oct 14, 2024

Currently rust protobuf libraries do not respect gogoproto.nullable option in proto files, thus some fields are required i generated rust code, but go has a different behaviour.

This PR solves this problem in a kinda dirty way, but it does solve it

Related PRs:

@pr0n00gler
Copy link
Collaborator Author

pr0n00gler commented Oct 31, 2024

@@ -312,6 +362,96 @@ pub fn serde_alias_id_with_uppercased(s: ItemStruct) -> ItemStruct {
syn::ItemStruct { fields, ..s }
}

fn is_field_gogoproto_nullable(
Copy link
Contributor

Choose a reason for hiding this comment

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

please move it after pub funcs

NeverHappened
NeverHappened previously approved these changes Nov 1, 2024
packages/neutron-std/src/serde/mod.rs Outdated Show resolved Hide resolved
packages/neutron-std/src/serde/mod.rs Outdated Show resolved Hide resolved
proto-build/src/transformers.rs Outdated Show resolved Hide resolved
false
}

pub fn respect_gogoproto_nullable(

Choose a reason for hiding this comment

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

I wonder if we can have tests for it. would be nice to get to know if some of this fragile logic breaks because of either ours or counterparty's changes to proto tags or GOGOPROTO_NULLABLE_EXTENSION_FIELD_NUMBER or something else. for example, apply serialization/deserialization on several proto messages taken from different sources and make sure the results re optionality are as expected

Copy link
Collaborator Author

@pr0n00gler pr0n00gler Nov 2, 2024

Choose a reason for hiding this comment

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

apply serialization/deserialization on several proto messages taken from different sources and make sure the results re optionality are as expected

I don't get it, could you elaborate? What exactly do you want to test?

Choose a reason for hiding this comment

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

I'd like to have some tests that would cover proto-to-rust code generation in terms of gogoproto_nullable with verification of the end rust struct fields optionality. ideally end-to-end tests with different (from different sources e.g. neutron, cosmos-sdk, osmosis, etc.) proto messages as input and rust structure fields assertion (gogoproto_nullable fields are optional, other are required). that could be done as a separate task. what do you think?

@@ -151,12 +154,14 @@ fn transform_items(
let s = transformers::add_derive_eq_struct(&s);
let s = transformers::append_attrs_struct(src, &s, descriptor);
let s = transformers::serde_alias_id_with_uppercased(s);
let s = transformers::respect_gogoproto_nullable(s, descriptor);

Choose a reason for hiding this comment

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

since structures are always optional as a result of rust proto generation (e.g. this field is not supposed to be optional), I wonder if it's possible to do the opposite: make structs optional only if there is a gogoproto_nullable set, and make them required otherwise?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm sure it's possible, but it's out of scope. Could you create a task for that?

Choose a reason for hiding this comment

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

sure, will do

@pr0n00gler pr0n00gler merged commit b17f083 into main Nov 11, 2024
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.

3 participants