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 marshal_strict feature #44

Merged
merged 4 commits into from
Jan 27, 2023
Merged

Conversation

artemreyt
Copy link
Contributor

Hello! We in gowaves use this plug-in to marshal messages for producing signatures futher and compare them to signed messages which would be marshalled by other languages implementations. The problem is that many other implementations of protobuf (for e.g. Java) marshal fields in the strict order by their numbers declared in .proto file. Consequently, the most rational way to use such marshal scheme in all languages we use in our project.

marshal feature doesn't satisfy this requirement and mustn't do as it should behave indentically to proto.Marshal() which doesn't satisfy respectively. To be more precise the difference is that oneOfs fields are marshalled before others(#22) and in the example below ethereum_transaction would be marshalled before proofs:

message SignedTransaction {
    oneof transaction {
        Transaction waves_transaction = 1;
        bytes ethereum_transaction = 3;
    }
    repeated bytes proofs = 2;
}

But we would like ethereum_transaction to be marshalled after proofs.

We came with implementation of the new feature marshal_strict that solves this problem and can be useful in similar cases.

Copy link
Member

@vmg vmg left a comment

Choose a reason for hiding this comment

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

Hey! Sorry for the delay on reviewing this. Code looks good but I'm confused about the varint with a new encoding. Can you clarify?

func (p *marshal) GenerateHelpers() {
p.P(`func encodeVarint(dAtA []byte, offset int, v uint64) int {`)
p.P(`func `, p.encodeVarintMeth(), `(dAtA []byte, offset int, v uint64) int {`)
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why you need a specific name for encodeVarint. The implementation is identical to the previous, so why name it differently?

Copy link
Contributor Author

@artemreyt artemreyt Aug 26, 2022

Choose a reason for hiding this comment

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

As i commented on 56 line in order to generate helpers for marshal and marshal_strict features independently.
E.g. if we want to use both marshal and marshal_strict features then encodeVarint implementation would be generated twice that will lead to compilation error.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay getting back to this, you caught me in holiday. We need to do better than that for this feature: let's check during codegen whether both marshal and marshal_strict are being requested and handle that case by generating the encodeVarint helpers only once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for long delay! Returned to this PR. Watch please, added duplicate checker for helper functions to avoid duplicates among helper functions names which are generated by different features in the same file.

@artemreyt
Copy link
Contributor Author

Hello! @vmg , we would be really grateful if you looked the last changes! Thank you!

@fenollp
Copy link
Contributor

fenollp commented Jan 17, 2023

How about replacing the default marshal feature with this one? Does this take more space on the wire or is less optimized in time or space somehow? Thanks

@artemreyt
Copy link
Contributor Author

How about replacing the default marshal feature with this one? Does this take more space on the wire or is less optimized in time or space somehow? Thanks

I'm afraid it can break something in projects which are using current order.

@vmg
Copy link
Member

vmg commented Jan 27, 2023

Looking at this PR today!

@vmg vmg merged commit 5652387 into planetscale:main Jan 27, 2023
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