Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

[contracts] API host functions: remove seal_ name prefix + enable aliasing #12126

Merged
merged 19 commits into from
Sep 7, 2022

Conversation

agryaznov
Copy link
Contributor

Resolves #11444

  • Added #[prefixed_alias] to the #[define_env] proc attribute macro.

    For a host_fn() marked by the attribute, the macro creates an additional alias function seal_host_fn() which is exactly the same as the original one but has the prefixed name for backwards compatibility. See this comment for a usage example.

  • Renamed API functions + added aliases.

  • Updated tests and benchmarks.

@agryaznov agryaznov added A0-please_review Pull request needs code review. B3-apinoteworthy I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Aug 27, 2022
@agryaznov agryaznov requested a review from athei as a code owner August 27, 2022 08:21
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

  • Rustdocs need to be updated with the new attribute
  • No non-prefixed version of an aliased function is used in any test

Bonus: Make the alias call the non-aliased function instead of duplicating the body.

frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
@agryaznov
Copy link
Contributor Author

agryaznov commented Aug 27, 2022

  • Rustdocs need to be updated with the new attribute

They are updated for the macro thing. Or are there any other places I forgot about?

  • No non-prefixed version of an aliased function is used in any test

Not exactly. For now, all tests for __unstable__ functions use non-prefixed version, as they will eventually become stabilized and get used by users with the new naming. For all other tests prefixed functions left because they are the ones used by pallet_contract users.

Bonus: Make the alias call the non-aliased function instead of duplicating the body.

Not realistic in current design. See e.g. how gas host fn gets expanded by the macro:

       fn impls<F: FnMut(&[u8], &[u8], crate::wasm::env_def::HostFunc<E>)>(f: &mut F) {
            f("seal0".as_bytes(), "gas".as_bytes(), {
                fn gas<E: Ext>(
                    ctx: &mut crate::wasm::Runtime<E>,
                    args: &[sp_sandbox::Value],
                ) -> Result<sp_sandbox::ReturnValue, sp_sandbox::HostError>
                where
                    <E::T as frame_system::Config>::AccountId: sp_core::crypto::UncheckedFrom<<E::T as frame_system::Config>::Hash>
                        + AsRef<[u8]>,
                {
                    #[allow(unused)]
                    let mut args = args.iter();
                    let mut body = || {
                        let amount : < u32 as crate :: wasm :: env_def :: ConvertibleToWasm > :: NativeType = args . next () . and_then (| v | < u32 as crate :: wasm :: env_def :: ConvertibleToWasm > :: from_typed_value (v . clone ())) . expect ("precondition: all imports should be checked against the signatures of corresponding
                                functions defined by `#[define_env]` proc macro by the user of the macro;
                                thus this can never be `None`;
                                qed;") ;
                        {
                            ctx.charge_gas(RuntimeCosts::MeteringBlock(amount))?;
                            Ok(())
                        }
                    };
                    body().map_err(|reason| {
                        ctx.set_trap_reason(reason);
                        sp_sandbox::HostError
                    })?;
                    return Ok(sp_sandbox::ReturnValue::Unit);
                }
                gas::<E>
            });
...

So basically each host fn becomes visible only inside a block which is a closure parameter. We can't call them from within each other.

@agryaznov agryaznov requested a review from athei August 27, 2022 16:04
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

They are updated for the macro thing. Or are there any other places I forgot about?

No. I was just too stupid to scroll down. The docs are great.

Not exactly. For now, all tests for unstable functions use non-prefixed version, as they will eventually become stabilized and get used by users with the new naming. For all other tests prefixed functions left because they are the ones used by pallet_contract users.

But only for the function which don't use the new attribute. So there is no code which tests whether the new prefix leaves generates version.

frame/contracts/proc-macro/src/lib.rs Show resolved Hide resolved
@agryaznov
Copy link
Contributor Author

But only for the function which don't use the new attribute. So there is no code which tests whether the new prefix leaves generates version.

I've added a test which catches this case.

@agryaznov agryaznov requested a review from athei August 29, 2022 15:33
@athei
Copy link
Member

athei commented Aug 30, 2022

I think your changes broke the build.

@agryaznov
Copy link
Contributor Author

agryaznov commented Aug 31, 2022

Yeah, making examples no-run in macro docs was not a good idea. It is a tricky puzzle to make it compile with rustdoc (at least no one of frame_support_procedural doc examples are intended to really compile).

@agryaznov
Copy link
Contributor Author

Added prefixes for the unstable host functions versions. Otherwise we break in-progress PR in ink!. We will remove the alias once we stabilize.

Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Thanks :)

frame/contracts/proc-macro/src/lib.rs Show resolved Hide resolved
frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/proc-macro/src/lib.rs Show resolved Hide resolved
frame/contracts/proc-macro/src/lib.rs Show resolved Hide resolved
frame/contracts/proc-macro/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM!

@agryaznov
Copy link
Contributor Author

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 3f70bce into master Sep 7, 2022
@paritytech-processbot paritytech-processbot bot deleted the ag-seal-remove branch September 7, 2022 08:54
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…liasing (paritytech#12126)

* works but ugly

* refactored + renamed host fns

* fixed tests

* fix benchmarks

* updated marco docs

* Update frame/contracts/proc-macro/src/lib.rs

Co-authored-by: Alexander Theißen <alex.theissen@me.com>

* fix for the duplicated prefixed alias bug + test

* refactored a bit

* fix warnings + try to make macro rustdoc compile

* fmt after clearing

* examples update + nocompile

* add seal_ prefixes to unstable host functions

* updated after a review

Co-authored-by: Alexander Theißen <alex.theissen@me.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. I9-optimisation An enhancement to provide better overall performance in terms of time-to-completion for a task.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Remove seal_ prefixes from contracts API function identifiers
3 participants