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

contract-transcoder for non-ink projects #659

Closed
extraymond opened this issue Jul 24, 2022 · 32 comments · Fixed by use-ink/ink#1357
Closed

contract-transcoder for non-ink projects #659

extraymond opened this issue Jul 24, 2022 · 32 comments · Fixed by use-ink/ink#1357
Assignees
Labels
question Further information is requested

Comments

@extraymond
Copy link

Info

background:
Thanks for splitting the contract-transcode crate into a separate crate! We're a new team working on substrate which uses pallet-contracts underneath.

We're currently trying to build helper libraries to bring solang based contracts closer to the metadata formats ink based contract used.
And having able to reuse as much code from cargo-contract as possible so that solang users can benefit from using tooling specifically build for ink as well. Devs report to us that when using polkadot.js based tools, they have problem using contracts other than primitive types.

My question is around ways to produce the same internal data structure and feed it to cargo-transcode.
To my understanding, currently contract-transcode relied on two parts to work:

  1. PortableRegistry from scale_info, which is produced by Registry during ink codegen.
  2. InkProject which shares information about messages/constructors/events which references type_id within PortableRegistry

attempts

1. follow InkProject

I tried forking solang and in the last step when building the solang::ABI, I tried to create a InkProject and PortableRegistry as close as possible.

Currently unsuccessful due to:

  1. Registry requires MetaType to register types in it, but to have a MetaType the function signature requires meta_type::<T: TypeInfo> which we're only able to provide primitive types which solang::abi::Type uses. For Composite and Enum it's not possible to generate the meta_type due to not being a rust data structure.
  2. Ink InkProject, the layout also requires StructLayout::<T: TypeInfo> which we're unable to generate on the fly
  3. ReturnTypeSpec is also impossible to generate due to need to generate the Composite type at compile time.

2. convert ABI

I tried converting existing ABI and build InkProject with its internal data.

Currently also impossible because for InkProject::new($args), the args are all generic across Spec<T: Form=MetaForm> which means it needs to know the type at compile time again.

3. inplace replacement with serialize <-> deserialize

all fields in InkProject's currently only require <Form=PortableForm>, and all those types have <Serialize, Deserialize> on it.

Without attempting to fork scale-info:

  1. I tried maintaining a copy of all type that require <T:Form=MetaForm> locally with <T: Form=PortableForm>
  2. create and assemble them locally
  3. convert inplace struct to buffer, serde_json::to_writer(LocalInkProject)
  4. convert them back into InkProject with serde_json::from_reader

Currently also failed due to some internal serde bonds not satisfied.

questions

Currently I'm having difficulty to directly reuse cargo-transcode for non-ink projects due to unable to produce InkProject from either end. Also custom_env_transcoder didn't work because it'll need the types to be in the PortableRegistry before hand.

  1. Is is feasible for cargo-contract to rely on a more relaxed InkProject format that can be opened to other non-ink projects to convert their ABI into it?
  2. If following InkProject is the desired behavior. How feasible would it be to inject runtime types into Registry without knowing the concrete type?
@HCastano HCastano added the question Further information is requested label Jul 27, 2022
@HCastano HCastano changed the title [question] contract-transcoder for non-ink projects contract-transcoder for non-ink projects Jul 27, 2022
@ascjones ascjones self-assigned this Aug 8, 2022
@ascjones
Copy link
Collaborator

ascjones commented Aug 8, 2022

Hi @extraymond, apologies for the delay I have been away on holiday. I am probably the best person to answer your questions, I will get to it this week.

@extraymond
Copy link
Author

No problem, currently I tried converting solang ABI to InkProject by abusing serde_json, which isn't the best approach for this IMO.
https://github.com/extraymond/solang/blob/bc0623bf9c160718567ad4d4feda7883853f5498/src/abi/substrate/registry/converter.rs#L422-L428

It would be nice to share something with InkProject, which could basically be a subset of InkProject which the constructor don't require anything to have rust type known in compile time, such as Registry, TypeSpec and Layout and so on. So we can construct the internal directly without breaking the promises of scale-codec.

@ascjones
Copy link
Collaborator

ascjones commented Aug 11, 2022

I have created an experimental PR paritytech/scale-info#164 which allows constructing a MetaType at runtime.

Can you try this branch out to see if it suits your needs? It will require patching in that scale-info branch.

@xermicus
Copy link
Contributor

I have created an experimental PR paritytech/scale-info#164 which allows constructing a MetaType at runtime.

Can you try this branch out to see if it suits your needs? It will require patching in that scale-info branch.

I think this goes into the right direction. How would I then use the type created in your example/test as message parameter type? (Including an end-to-end example somewhere in the docs would be very helpful for newcomers I guess, however I'm aware this is not yet finished ☺️ )

@extraymond
Copy link
Author

I have created an experimental PR paritytech/scale-info#164 which allows constructing a MetaType at runtime.

Can you try this branch out to see if it suits your needs? It will require patching in that scale-info branch.

Thanks I'll try to play with this.

@ascjones
Copy link
Collaborator

How would I then use the type created in your example/test as message parameter type

You will need to pass your constructed MetaType to the TypeSpec, as part of a MessageParamSpec injected here: https://github.com/xermicus/metadat/blob/main/src/main.rs#L47. However it looks like it will require adding an additional constructor for that, which you should be able to do in your local ink_metadata source.

https://github.com/paritytech/ink/blob/47b1fa3291dd7551002e780ace31df4d80e8539d/crates/metadata/src/specs.rs#L863

@xermicus
Copy link
Contributor

However it looks like it will require adding an additional constructor for that, which you should be able to do in your local ink_metadata source.

Thanks @ascjones 😊 I was tinkering with this yesterday but couldn't figure out a (sane) way without changing ink_metadata. Adding a new constructor seems like the obvious way, should I open a PR in ink_metadata or do you want to take over?

@ascjones
Copy link
Collaborator

Adding a new constructor seems like the obvious way, should I open a PR in ink_metadata or do you want to take over?

For the purposes of experimenting to see if this approach works, I would just add that constructor in your local ink_metadata source and change your manifest to point at your local source code e.g. ink_metadata = { path = "../ink/crates/ink_metadata" } https://github.com/xermicus/metadat/blob/main/Cargo.toml#L9

@xermicus
Copy link
Contributor

I got this close to working by adding a simple custom constructor to TypeSpec. The problem I'm facing now is I think that MetaType is not generic over the return type of the fn_type_info function pointer. It could probably be resolved by making MetaType generic, or is there a simpler solution I can't think of?

error[E0308]: mismatched types
   --> src/main.rs:60:47
    |
60  |     let meta_type = MetaType::new_custom(123, runtime_meta_type);
    |                     --------------------      ^^^^^^^^^^^^^^^^^ expected `&str`, found struct `std::string::String`
    |                     |
    |                     arguments to this function are incorrect
    |
    = note: expected fn pointer `fn() -> Type<MetaForm<&'static str>>`
                  found fn item `fn() -> Type<MetaForm<std::string::String>> {runtime_meta_type}`

@ascjones
Copy link
Collaborator

Ok I've done this with paritytech/scale-info@4500520.

@xermicus
Copy link
Contributor

This might be a bit more of a rabbit hole than I thought... ink_metadata might require quite some changes as well, since all the specs in spec.rs rely on <F: Form = MetaForm> as their default type (where as MetaForm itself uses <S: FormString = &'static str> as default type)?

I can start a PR for ink_metadata tomorrow

@ascjones
Copy link
Collaborator

I might have a better idea, hold your horses while I give it a go.

@ascjones
Copy link
Collaborator

Ok try the latest of my branch. Key difference is that MetaForm::String is now always an owned String in std. https://github.com/paritytech/scale-info/pull/164/files#diff-68f6e556a04385e5cbb83ac439e9409e2af40b5d8c1baa87055eaf1d5cdf05cdR78

The reason we need all that is because of substrate runtimes which uses the minimal version of parity-scale-codec which doesn't implement Encode for String, only &'static str. On the ink! side we are operating in std so can use owned Strings

@xermicus
Copy link
Contributor

Nice, that should make it a lot simpler indeed! I try it out later today, thanks!

@xermicus
Copy link
Contributor

I can make my example compile with some slight changes to ink_metadata 🎉 However I'm afraid this changes its API... How does this look to you @ascjones ?

@ascjones
Copy link
Collaborator

Yes that looks good, you could probably even make it a non breaking change if you make the docs methods generic over S: ToString.

@xermicus
Copy link
Contributor

xermicus commented Aug 18, 2022

Gave it a shot. cargo test -p ink_metadata is green now. Caveat: by introducing the new generic param it might now require a type hint in some cases. This will break stuff:

❯ cargo test -p ink_lang
   Compiling ink_lang v4.0.0 (/home/cyrill/mess/ink/crates/lang)
error[E0282]: type annotations needed
  --> crates/lang/tests/unique_topics.rs:19:1
   |
19 | #[ink::contract]
   | ^^^^^^^^^^^^^^^^ cannot infer type of the type parameter `D` declared on the associated function `docs`
   |
   = note: this error originates in the attribute macro `ink::contract` (in Nightly builds, run with -Z macro-backtrace for more info)
help: consider specifying the generic arguments
   |
19 | #[ink::contract]::<[_; 0], Str>
   |                 +++++++++++++++

For more information about this error, try `rustc --explain E0282`.
error: could not compile `ink_lang` due to previous error

@xermicus
Copy link
Contributor

xermicus commented Aug 19, 2022

by introducing the new generic param it might now require a type hint in some cases. This will break stuff:

Maybe I've found the middle-ground, by using a &str ref, using &'static str is still valid, but it would also allow any other string ref.

With that latest commit cargo test --workspace is passing again @ascjones

@extraymond
Copy link
Author

I tried using the scale-info and ink-metadata fork mentioned above, and currently when trying to generate runtime types for solang, I'm facing the issue of not able to reference another type within a type.

So for example in solang, the ast can reference another type which should contain most of the types:
ast::Types::Array(ast::Types, dimention)

// For example for `[u8; 32]` ink-metadata, we can do this:
// this is possible for known type:
let meta = meta_type::<[u8; 32]>;
let spec = TypeSpec::new_from_ty(meta);
// or 
let spec = TypeSpec::new::<[u8; 32]>()

// But if we have some type created earlier for example type_id_1 -> TypeSpec::new_from_ty(meta_of_1)
// and want to create [type_1; 32]

let inner_meta = todo!("get the meta from a cache or hashmap");
let arr = TypeDefArray::new(inner_meta, 32);
let ty = Type::from(arr);

// this is impossible due to `fn() - Type` can not capture variable, therefore it cannot reference external meta.                                                              
let meta = MetaType::new_custom(type_id, || { ty });
// needed for the below
let spec = TypeSpec::new_from_ty(meta);
let m = MessageParamSpec::new().of_type(spec);

Ideally solang's ast can be converted into mostly three groups:

  1. primitive types: which we can use rust types to represent it u8, boo, string
  2. types that reference others, so ast::Type::Array, ast::Type::Enum, ast::Type::Struct, where ast::Type::Array(ast::Type::Unint(8), vec![FixedLength(32)]) -> [u8; 32]
  3. and others being able to be represented using those referencing types, for example both DynamicBytes | Bytes-> Array(ast::Type::Uint(8), dimention),

This is my current working branch:
https://github.com/extraymond/solang/blob/feature_ink_metadata/src/abi/substrate_ink_metadata.rs

@xermicus

@xermicus
Copy link
Contributor

xermicus commented Aug 21, 2022

Yes, I think as the final step (hopefully), we should think about opening up the fn() -> Type function signature or refactor MetaType in some other way, to make it actually usable in a runtime context.

EDIT: With this change it would be possible to use a closure that captures values. What do you think @ascjones @extraymond ?

@ascjones
Copy link
Collaborator

Okay so my experiment to attempt to use the existing static Registry did not work because of the requirement of the static fn_type_info function pointer.

Having looked at @extraymond's working branch, I think we should attempt again the path of constructing the PortableRegistry manually. See my latest commits in paritytech/scale-info#164.

Essentially it is putting the full responsibility to the caller of producing the type ids and putting them in the correct order in the PortableRegistry. See https://github.com/paritytech/scale-info/blob/c46186dd5632580f5b120a27c7f359f272b5b8f9/src/tests.rs#L429.

I assume since ast::Type implements Hash https://github.com/hyperledger-labs/solang/blob/main/src/sema/ast.rs#L22 you will be able to maintain a mapping between the hash of the type and it's id as a position in the types vector.

Note that I have made all those fields public as a temporary measure, if this is the correct approach I will update the builders API to allow construction with PortableForm.

@extraymond
Copy link
Author

@ascjones
I'll give it a try tomorrow! Thanks for the update.

@extraymond
Copy link
Author

@ascjones @xermicus

I tried the latest commit on scale-info, and while it's possible to create portable registry now. From InkProject's POV, all the Spec's constructors are bounded to have something similar as struct ConstructorSpec<F: Form = MetaForm>{}

Which means currently it's not possible to use most of the methods from ink_metadata to construct a new instance, compared to previously where everything looks the same, also there's several trait and type that's used to control the life-cycle of construction, such as NameAssigned, TypeAssigned, and gradually expose method for the constructor if it met the requirement. If we change that the flow for testing against InkProject might get a bit harder.

@ascjones
Copy link
Collaborator

I tried the latest commit on scale-info, and while it's possible to create portable registry now. From InkProject's POV, all the Spec's constructors are bounded to have something similar as struct ConstructorSpec<F: Form = MetaForm>{}

Which means currently it's not possible to use most of the methods from ink_metadata to construct a new instance

For the purposes of this experiment, could you do as I did and use a fork of ink_metadata which temporarily makes the fields of the various structs public? This will allow construction of them with the PortableForm. If this works and is a satisfactory solution we can do it properly and update all the builders and constructors to be generic over Form.

@extraymond
Copy link
Author

extraymond commented Aug 23, 2022

Thanks, after opening up most of the fields for all the needed types, currently blocked because PortableType is under a private module registry, might also need it to be exported under crate or make the whole module public.

@ascjones
Copy link
Collaborator

I've made registry pub for now

@extraymond
Copy link
Author

extraymond commented Aug 23, 2022

If I open most of the types on ink_metadata, based on the latest change from @xermicus 's branch, it works! Already pushed to my working branch.

All needed information can be passed from solang to InkProject, with only type DisplayName = Path needs to open the segments part within it from scale-info

Currently instead of using Hash from ast::Type to distinguish types, I directly use PortableRegistry to get or register new types within it.

@ascjones
Copy link
Collaborator

Awesome! So the next step is to hide all the fields again and update all the builders in scale-info and ink_metadata to be generic over Form.

@xermicus
Copy link
Contributor

xermicus commented Aug 26, 2022

I updated the constructors and builders in ink_metadata, see the last 3 commits. Could you please have a look at this @ascjones ?

@xermicus
Copy link
Contributor

Alternatively I can open the PR for review, if it is going into the right direction.

@ascjones
Copy link
Collaborator

If it provides a working solution to this problem, then we should definitely open it up for review.

However it will depend on paritytech/scale-info#164 being updated to hide the fields again and update all the builders to make them generic (ideally in a backwards compatible way). I may not get to that in the next few days, so feel free to push to my branch there or open your own PR.

@xermicus
Copy link
Contributor

Sounds reasonable, thanks for the feedback. Sure, I'll have a look into the builders from scale-info as well 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants