-
Notifications
You must be signed in to change notification settings - Fork 64
Proto generation for Vixen Render #589
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
Proto generation for Vixen Render #589
Conversation
`feat`: Add proto oneof msgs for program & allow folder structure change
…toproto-transforms `feat` Add implementation for into_proto transforms
chore : add accounts to dummy idl and test account discriminators
feat : use account discriminators for shank
feat : use anchor account discriminators for parsers
…o-structs-and-rust
🦋 Changeset detectedLatest commit: ec06b88 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
lorisleiva
left a comment
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.
After a first pass on this PR, I can see this is a better approach than before. My main issue here is that this PR does way too much. It's tricky to review and make sure no bugs or misuse of the standard are being introduced.
Since this seems to add a new Codama to Protobuf generator, how would you feel about creating a @codama/renderers-protobuf package that only does this. Then you could use this package as a dependency for the vixen parser and only focus on the parts that are unique to vixen. Would that make sense or is the protobuf conversion too opinionated to be extracted as a generic tool?
renderers-vixen-parserThere 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.
Since this seems to add a new Codama to Protobuf generator, how would you feel about creating a @codama/renderers-protobuf package that only does this. Then you could use this package as a dependency for the vixen parser and only focus on the parts that are unique to vixen. Would that make sense or is the protobuf conversion too opinionated to be extracted as a generic tool?
@lorisleiva for now the proto is an extension of Vixen for its stream feature. There are likely pieces that could be isolated but don't want to split out too early. It is cool that this generates a proto def for the IDL that may be useful in other situations like a grpc based RPC stack that returns parsed results.
Overall, @lorisleiva all these changes are isolated to the renderers-vixen-parser and we have 10+ new parsers based on these updates so we are happy with its capabilities right now and would like to get a release out so we can update community and our docs.
|
Thanks for the update Kyle, I'm happy to get that merged if you guys are happy with the PR. Just need to get CI to green first and I'll approve. 🙏 |
…o-structs-and-rust
Thanks @lorisleiva! 🙏 This is done, just updated the branch and the tests. Also now that we have the shank discriminators merged in, I updated the e2e script here to showcase that here eeb0198#diff-b9cdaef35149142caeaf31a6606a6891e1c88d37cb45e2c240549810487c0fa9 |
sonicfromnewyoke
left a comment
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.
overall looks great to me.
if you don't mind, implementing the next changes, which are described in this solana-sdk-crate-migration-guide
this migration should drastically reduce compilation time and resulting binary size
...ages/renderers-vixen-parser/e2e/orca-whirlpool-parser/src/generated_sdk/accounts/fee_tier.rs
Show resolved
Hide resolved
.../orca-whirlpool-parser/src/generated_sdk/instructions/set_collect_protocol_fees_authority.rs
Show resolved
Hide resolved
.../orca-whirlpool-parser/src/generated_sdk/instructions/set_collect_protocol_fees_authority.rs
Show resolved
Hide resolved
Great suggestions @sonicfromnewyoke and I agree with removing solana-program as a dependency. @fernandodeluret can you gather this info into a follow up issue for use to address. There is a mix requests for changes to the rust SDK and Vixen parser renderers. |
lorisleiva
left a comment
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.
Thanks! I can see there are some ongoing comments but feel free to merge whenever you're happy with the state of the PR.
…o-structs-and-rust
Overview
The render now generates code needed to support proto feature of Vixen. Simplification of end-user API.
Changes