search

pendulum-chain / substrate-stellar-sdk

A Rust SDK for Stellar that can be used for Substrate projects
Apache License 2.0
11 stars 11 forks source link

Improve identification of types that require `Box` in types.rs generation. #22

Open gianfra-t opened 11 months ago

gianfra-t commented 11 months ago

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.

ebma commented 11 months ago

@pendulum-chain/product this ticket is a follow-up to https://github.com/pendulum-chain/substrate-stellar-sdk/issues/19. It is about improving the type generation, maybe making the generated code more efficient. It is not immediately needed and of rather low priority/nice-to-have.

@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)?

prayagd commented 11 months ago

Hey team! Please add your planning poker estimate with Zenhub @adelarja @b-yap @ebma @TorstenStueber

prayagd commented 9 months ago

@ebma should we move this to icebox as this is nice to have and not impacting anything?

ebma commented 9 months ago

I think it's okay to move it to the icebox 👍

TorstenStueber commented 9 months ago

@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 https://github.com/pendulum-chain/substrate-stellar-sdk/pull/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.

gianfra-t commented 9 months ago

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. Anyways, how many of the stellar types are used in the non-std environment of Substrate vs in the vault code?

TorstenStueber commented 9 months ago

Before they added smart contracts: actually a lot as we were decoding messages from the SCP (consensus protocol) and all possible transactions and operations.