-
Notifications
You must be signed in to change notification settings - Fork 11
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
Improve identification of types that require Box
in types.rs generation.
#22
Comments
@pendulum-chain/product this ticket is a follow-up to #19. @TorstenStueber do you happen to have an idea how we can detect and prevent cyclical references without hard-coding a prefix for types that we know will be cyclical (detected by mere trial-and-error)? |
Hey team! Please add your planning poker estimate with Zenhub @adelarja @b-yap @ebma @TorstenStueber |
@ebma should we move this to icebox as this is nice to have and not impacting anything? |
I think it's okay to move it to the icebox 👍 |
@ebma Detecting cycles is not too hard but would go beyond the simple local replacement that is happening now. One would for example need to build a dependency graph and detect cycles in that graph. For that reason I think that the current solution is fine. Actually I would even think that the extensions in #20 were not strictly necessary as this only happens in the new complex type system related to Soroban. I doubt that we would be required to decode Soroban related types. But anyway, it works now. @gianfra-t There might be experimental support for Rust in xdrgen but is that also suitable in a non-std environment? We need to use types directly exposed by the replacement sp-std here. |
You are right about the non-std, didn't think about that. I don't know of any other way to check that than by generating the types and checking that there is no std import. |
Before they added smart contracts: actually a lot as we were decoding messages from the SCP (consensus protocol) and all possible transactions and operations. |
Given the latest Stellar types update and the fix introduced in this PR, we should review the solution proposed because it is not a resilient one to future type changes.
As a recap, with the update some types started to reference other non-primitives one, which created a cyclical reference error upon compilation. This is solved by Boxing these types. For instance this struct.
The issue was solved simply by identifying the types that required
Box
on it's reference by name prefix, as can be seen here and here.Solution
This types that require boxing should not be identified by name but dynamically so the solution can be resilient to future changes in Stellar types.
Alternative Solutions
It seems that in the future the generation of types directly Rust will be possible, where support is experimental now. We should assess if we should keep the current approach of an intermediate generation to javascript and then using the local
js-xdr
to generate to rust, or test the types generated directly to rust.The text was updated successfully, but these errors were encountered: