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

refactor: fn_selector to be resolved during runtime via ParamTypes #587

Merged
merged 13 commits into from
Sep 22, 2022

Conversation

segfault-magnet
Copy link
Contributor

While implementing vector support I needed to introduce a ParamType::Vector and a Token::Vector in order to handle the encoding and decoding of a vector. They are defined as:

pub enum ParamType {
    // ..
    Vector(Box<ParamType>),
    // ..

and

pub enum Token {
    // ..
    Vector(Vec<Token>),
    // ..

And there is an implementation of ParamType for Vec<T> which looks something like this:

impl<T: Parameterize> Parameterize for Vec<T> {
    fn param_type() -> ParamType {
        ParamType::Vector(Box::new(T::param_type()))
    }
}

A Vector, as far as the ABI json is concerned, is defined as follows:

pub struct Vec<T> {
    buf: RawVec<T>,
    len: u64,
}

struct RawVec<T> {
    ptr: u64,
    cap: u64,
}

which means that, if you called ParamType::from_type_declaration on them you would have gotten a Struct([Struct(U64, u64), U64]). This is not equal to what calling Vec<T>::param_type() would have gotten you.

The param type gotten from the from_type_declaration is difficult to use in order to detect you're dealing with a vector when decoding/encoding. It might very well be just a structure in a structure with some U64s thown in. Our encoding/decoding code needs to differentiate vectors and that is why they are a ParamType of their own.

This got me thinking about other problems caused by having two ways of constructing ParamTypes.

Ultimately I've deduced that you would call from_type_declaration in a context where you couldn't use the other one. And the only such place is during the expansion of the abigen! macro where you don't actually have the code to call ::param_type on it -- you're in the process of creating it.

Furthermore, ParamTypes are needed in the abigen because they are used to generate the fn_selector which is then hardcoded for each of the contract methods as a bunch of bytes.

If we don't resolve the function selector during the macro expansion but rather leave the methods to do that for themselves upon being called, we would then have no need for ParamTypes during the macro expansion. Then from_type_declaration could be removed.

If ParamTypes are no longer used during macro expansion, this would mean that they could never be given an unresolved generic, since if you're generating a ParamType via ::param_type then the Rust compiler would have already resolved all the generics for you.

This has the nice consequence of not having those pesky panic!("you cannot encode/decode/fn_sel_resolve a generic parameter".

Furthermore, since the rust compiler resolves the generics, we can significantly simplify the process of creating the function selector. The only thing you need is the name of the function and a slice of the ParamTypes which come from the function arguments.

So the generated code for a method would look something like this:

      #[doc = "Calls the contract's `some_abi_funct` function"]
                pub fn some_abi_funct(&self, s_1: MyStruct1, s_2: MyStruct2) -> ContractCallHandler<MyStruct1> {
                    let provider = self.wallet.get_provider().expect("Provider not set up");
                    let encoded_fn_selector = resolve_fn_selector(
                        "some_abi_funct",
                        &[<MyStruct1> :: param_type(), <MyStruct2> :: param_type()]
                    );
                    let tokens = [s_1.into_token(), s_2.into_token()];
                    Contract::method_hash(
                        &provider,
                        self.contract_id.clone(),
                        &self.wallet,
                        encoded_fn_selector,
                        &tokens
                    )
                    .expect("method not found (this should never happen)")
                }

This way all the recursive generics resolving hassle can be removed and delegated to the rust compiler.

I didn't notice any significant performance impacts of having the methods constantly resolve the fn selector on every call, but if those should ever arise we can always cache the resolved fn selector.

There were also parts of the generics resolving code in resolve_type which used ParamTypes only to figure out whether the current type is a struct/enum -- i.e. the rough details -- the internal fields were ignored since from_type_declaration was not capable of resolving generics. With this change the logic from from_type_declaration was merged with resolved_type and there is no more confusion as to who resolves the generic parameters.

The downside is that json_abi.rs had to go. It was used in the CLI fuels-rs tool which could have given you encoded parameters when provided with a json abi. Its usage is mainly to be a debugging tool for the sdk devs. I had a talk with @digorithm and he is ok with it, especially since it isn't very difficult to use the SDK to get whatever you're interested in.

The CLI tools have been commented out since the new abigen format, don't hear anybody missing it too much :D

If we ever find that we need the CLI desperately, I've got a few ideas on how to have your cake and eat it too.

So now the flow is as follows

abigen -> structs + functions
structs -> param_types

@segfault-magnet segfault-magnet added the tech-debt Improves code quality or safety label Sep 21, 2022
@segfault-magnet segfault-magnet requested a review from a team September 21, 2022 13:50
@segfault-magnet segfault-magnet self-assigned this Sep 21, 2022
@digorithm digorithm mentioned this pull request Sep 21, 2022
7 tasks
hal3e
hal3e previously requested changes Sep 22, 2022
Copy link
Contributor

@hal3e hal3e left a comment

Choose a reason for hiding this comment

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

Beautiful! I like it a lot! Left some minor nits.

packages/fuels-types/src/param_types.rs Outdated Show resolved Hide resolved
packages/fuels-types/src/param_types.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/code_gen/custom_types/utils.rs Outdated Show resolved Hide resolved
packages/fuels-core/src/types/native.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@MujkicA MujkicA left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link
Member

@Salka1988 Salka1988 left a comment

Choose a reason for hiding this comment

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

LGTM!

@segfault-magnet segfault-magnet dismissed hal3e’s stale review September 22, 2022 14:50

I've implemented Halil's request. Just don't know how to communciate that with github's request changes :)

hal3e
hal3e previously approved these changes Sep 22, 2022
Copy link
Member

@digorithm digorithm left a comment

Choose a reason for hiding this comment

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

This is so good. Excellent job. This is a massive reduction in complexity.

Comment on lines +253 to +256
let encoded_fn_selector = resolve_fn_selector(
"some_abi_funct",
&[<MyStruct1> :: param_type(), <MyStruct2> :: param_type()]
);
Copy link
Member

Choose a reason for hiding this comment

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

Ya love to see it! 🤘

I really disliked having to maintain that hardcoded selector byte array there.

error_message
);
}
}
Copy link
Member

Choose a reason for hiding this comment

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

🔥

Huge weight off of our shoulders.

@segfault-magnet segfault-magnet merged commit 32f895b into master Sep 22, 2022
@segfault-magnet segfault-magnet deleted the param_type_extraction branch September 22, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tech-debt Improves code quality or safety
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants