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

Implement new trait definition codegen 🚀 #665

Merged
merged 516 commits into from
Oct 20, 2021
Merged

Conversation

Robbepop
Copy link
Collaborator

@Robbepop Robbepop commented Jan 29, 2021

Implements #631.

  • Implementation TODOs:

    • Try to get rid of TraitUniqueId and its associated unique ID calculation.
      • The problem with it is that it is possible to have overlaps with other ink! trait definitions since the calculation is based on syntactical input.
      • We could potentially "abuse" the generated and associated ink! trait info object instead of the unique trait ID for TraitCallForwarderFor and others.
    • Find a solution for ink! trait definitions making use of environmental types such as Balance or AccounId by themselves.
    • Add more UI tests for the attribute macros:
    • Restructure ink_lang definitions into public reflect and private/hidden codegen modules.
      • The reflect module and definitions can be used by ink! smart contract authors to reflect upon their own smart contract code to a certain extend.
      • The codegen module is hidden from users and only serves as purpose to provide definitions that can be used by the ink! codegen to enforce some Rust compiler type system based checks on the ink! smart contract code.
    • Improve codegen to guard that ink! event definitions respect the topic limitation.
      • Codegen and generated code is more readable.
      • The error now indicates the amount of event topics and the event topic limitation and is less verbose.
      • Fix some macro hygiene problems.
    • Modernize UI tests.
    • Improve compile time errors for invalid ink! constructor and message inputs and outputs.
      • This applies to ink! message and constructor inputs that are not scale::Decode + 'static.
      • Also this applies to ink! message outputs that are not scale::Encode + 'static.
    • Make ink! root smart contract messages revert execution when returning Result::Err(_).
    • Adjust ink! metadata producer codegen for the new dispatcher and ink! trait definition semantics.
    • Write new dispatcher for ink! smart contracts that works with the new ink! trait system.
    • Remove ink_lang::TraitImplementer<const ID: u32> trait implementations and super traits.
      • The generated trait implementations do not serve a real purpose and just make codegen and the generated ink! trait definitions a lot less readable.
    • Add way to refer from ink! trait implementation block to the trait and its ID in order to calculate ink! trait message selectors and query properties such as payable for them.
      • Possibility to compute ink! trait message selectors.
      • Query ink! trait message payable.
    • Generate documentation attributes for <Contract>Ref and its API.
  • Overall ink! design:

    • Remove ink-as-dependency crate feature for ink! smart contracts. (Moved to a follow-up PR)
    • Add ink-as-root crate feature for ink! smart contracts. (Moved to a follow-up PR)
    • ink! smart contracts now always generated two contract types. Given MyContract:
      • MyContract will still be the storage struct. However, it can now additionally be used as static dependency in other smart contracts. Static dependencies can be envisioned as being directly embedded into a smart contract.
      • MyContractRef is pretty much the same of what we had gotten with the old ink-as-dependency. It is a typed thin-wrapper around an AccountId that is mirroring the ink! smart contract's API and implemented traits.
  • ink! IR module:

    • Extend capabilities of ink! trait definition IR items.
      • It is now possible to query selectors of ink! trait messages.
      • It is now possible to query payable:bool modifier for ink! trait messages.
      • It is now possible to query the namespace:str modifier for ink! trait definitions.
  • ink! trait definitions: #[ink::trait_definition(..)]

    • No longer allow #[ink(constructor)] definitions in ink! trait definitions.
    • ink! trait definitions now know by themselves how to dispatch into their messages.
      • Call builder abstractions for concrete ink! smart contracts are simply forwarding to those trait specific abstractions.
        This allows us to later introduce abstractions such as dynamic trait calling. (Not implemented in this PR.)
    • Allow for #[ink::trait_definition(namespace: str)].
    • Allow for #[ink(selector: u32)] modifier.
  • ink! trait implementation blocks: impl MyTrait for MyContract { ... }

    • Disallow #[ink(namespace: str)] modifier on ink! trait implementation blocks.
      • Overwriting the namespace modifier would destroy trait calling invariants.
    • Implement guard semantic for #[ink(selector: u32)] modifier.
      • Overwriting the selector in trait impl blocks would destroy trait calling invariants.
      • Users can use the selector property on ink! trait messages in order to guard against the ink! trait definition silently changing the selector property from the respective ink! message. This cannot be used to alter the selector of an ink! message in an ink! implementation block. If used the selector must match the selector given via the ink! trait definition of the ink! trait message.
    • Implement guard semantic for #[ink(payable: bool)] modifier.
      • Users can use the payable property on ink! trait messages in order to guard against the ink! trait definition silently removing the payable property from the respective ink! message. Applying payable on ink! trait message in implementation blocks won't alter the payability of the ink! message.
  • Tests for the implementation:

    • Tests for the new ink! trait definition mechanics.
    • Tests for the changed ink! trait implementation block mechanics.
  • Miscellaneous changes:

    • Generated integer literals used for identifications are now generated in hex-decimal representation.
      • This affects selectors and unique IDs amonst others and should improve readability of generated code.
    • Lots of clean ups of duplicated code in codegen and IR modules.
    • Macro hygiene improvements.
      • ink! metadata codegen
      • Derives for ink_storage traits.
      • Codegen for #[ink::contract(..)] macro.
      • Codegen for #[ink::trait_definition(..)] macro.

Drawback of the Implementation

  • Users can no longer specify ink! constructors in ink! trait definitions.

Follow-Ups:

  • Implement #[ink(payable: bool)] modifier.
    • Allow to specify #[ink(payable = flag:bool)] instead of just #[ink(payable)].
    • Make other value: bool ink! attributes work the same to unify user experience.
  • Replace ink-as-dependency optional crate feature of ink! smart contracts with ink-as-root crate feature.
    • The ink-as-root crate feature just states the opposite.
    • The effect of this change is that depending on another smart contract won't require you to enable any ink! specific feature.
  • Allow ink! messages and ink! constructors to have trait implementers as inputs.
    • For example
      #[ink(constructor)]
      fn my_constructor_1(handler: &dyn MyInkTrait);
      #[ink(constructor)]
      fn my_constructor_2(handler: &mut dyn MyInkTrait);
      #[ink(message)]
      fn my_message_1(&self, handler: &dyn MyInkTrait);
      #[ink(message)]
      fn my_message_2(&self, handler: &mut dyn MyInkTrait);
      The above syntax and implied semantics are not final and means for change and iteration.
  • Allow to use ink! trait implementers as fields in the ink! storage type definition.
    • For example
      #[ink(storage)]
      pub struct MyStorage {
          handler_1: Box<dyn MyInkTrait>,
          handler_2: &dyn MyInkTrait,
          handler_3: &mut dyn MyInkTrait,
      }
      The above syntax and implied semantics are not final and means for change and iteration.
  • Allow to use super ink! trait definitions in ink! trait defnitions.
    • Right now we disallow defining supertraits for any ink! trait definition.
      However, it should be totally fine and maybe even useful to have ink! trait definitions as supertraits of other ink! trait definitions.

Oursourced Work

@hugopeixoto
Copy link

Does the scope of this PR include the "As Storage Entity" functionality from #631?
I don't know much about ink internals so it's a bit hard for me to map it to the concepts in the todo list.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Mar 1, 2021

Does the scope of this PR include the "As Storage Entity" functionality from #631?
I don't know much about ink internals so it's a bit hard for me to map it to the concepts in the todo list.

Is this a very much needed feature? Actually I would like to try to keep the scope of the initial MVP small.

@hugopeixoto
Copy link

Is this a very much needed feature? Actually I would like to try to keep the scope of the initial MVP small.

That's alright, I was sincerely asking if it's part of it or not, not requesting it to be included.

@Robbepop
Copy link
Collaborator Author

Robbepop commented Mar 5, 2021

Is this a very much needed feature? Actually I would like to try to keep the scope of the initial MVP small.

That's alright, I was sincerely asking if it's part of it or not, not requesting it to be included.

The part should open up possibility to add this feature at a later point in time. However, there are still some open questions as to how the storage-related parts should syntactically look like. There are some ways, non of them obviously the best.

@xgreenx
Copy link
Collaborator

xgreenx commented Sep 14, 2021

Do you have ETA for this cool change?(=

@Robbepop
Copy link
Collaborator Author

Robbepop commented Sep 14, 2021

Do you have ETA for this cool change?(=

The latest commit no longer produces compile errors in the new codegen. So the new codegen basically works now. However, plenty of stuff still needs to be implemented. This whole thing is much more than just the new trait system as it will feature quite a bit other stuff, too. Will write about this more soon.

The following smart contract works with the latest commit:

use ink_lang as ink;

/// Allows to increment and get the current value.
#[ink::trait_definition]
pub trait Increment {
    /// Increments the current value of the implementer by 1.
    #[ink(message)]
    fn inc(&mut self);

    /// Returns the current value of the implementer.
    #[ink(message)]
    fn get(&self) -> u64;
}

/// Allows to reset the current value.
#[ink::trait_definition]
pub trait Reset {
    /// Increments the current value of the implementer by 1.
    #[ink(message)]
    fn reset(&mut self);
}

#[ink::contract]
pub mod incrementer {
    use super::{Increment, Reset};

    #[ink(storage)]
    pub struct Incrementer {
        value: u64,
    }

    impl Incrementer {
        /// Creates a new incrementer smart contract initialized with `0`.
        #[ink(constructor)]
        pub fn new() -> Self {
            Self { value: Default::default() }
        }

        /// Increases the value of the incrementer by the given delta.
        #[ink(message)]
        pub fn inc_by(&mut self, delta: u64) {
            self.value += delta;
        }
    }

    impl Increment for Incrementer {
        #[ink(message)]
        fn inc(&mut self) {
            self.inc_by(1)
        }

        #[ink(message)]
        fn get(&self) -> u64 {
            self.value
        }
    }

    impl Reset for Incrementer {
        #[ink(message)]
        fn reset(&mut self) {
            self.value = 0;
        }
    }
}

@xgreenx
Copy link
Collaborator

xgreenx commented Sep 22, 2021

Could you describe a little bit how static dependency will work, please?

@Robbepop
Copy link
Collaborator Author

Robbepop commented Sep 22, 2021

Could you describe a little bit how static dependency will work, please?

Best described by this delegator example:
https://gist.github.com/Robbepop/eb32d8b7306681243461520c24b61d0f

The gist examples already compile on this branch.

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2021

Codecov Report

Merging #665 (f1b0bd6) into master (8a745ff) will decrease coverage by 3.61%.
The diff coverage is 68.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
- Coverage   82.75%   79.14%   -3.62%     
==========================================
  Files         191      243      +52     
  Lines        8239     9128     +889     
==========================================
+ Hits         6818     7224     +406     
- Misses       1421     1904     +483     
Impacted Files Coverage Δ
crates/lang/codegen/src/generator/env.rs 100.00% <ø> (ø)
crates/lang/codegen/src/generator/mod.rs 100.00% <ø> (ø)
crates/lang/codegen/src/lib.rs 100.00% <ø> (ø)
crates/lang/ir/src/ir/chain_extension.rs 93.50% <ø> (ø)
crates/lang/ir/src/ir/item_impl/constructor.rs 91.52% <ø> (-6.78%) ⬇️
crates/lang/macro/src/lib.rs 100.00% <ø> (ø)
crates/lang/src/codegen/dispatch/execution.rs 0.00% <0.00%> (ø)
crates/lang/src/env_access.rs 12.00% <ø> (-15.28%) ⬇️
crates/lang/src/reflect/dispatch.rs 0.00% <0.00%> (ø)
...tes/lang/tests/ui/blake2b/pass/bytestring_input.rs 100.00% <ø> (ø)
... and 120 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a745ff...f1b0bd6. Read the comment docs.

crates/metadata/src/tests.rs Outdated Show resolved Hide resolved
examples/delegator/lib.rs Show resolved Hide resolved
@Robbepop
Copy link
Collaborator Author

@Robbepop There are a number of changes to the way how ink! works in this PR and it would be helpful for the review to have the "Unreleased" section in RELEASES.md updated as well. Since we plan on a short-term release candidate as well, we need to do this soon anyway. Could you add this to the PR?

Done. However, I just saw that some of the things that have been implemented as side-PRs have not been included properly into the RELEASES.md for the ink! 3.0-rc6. E.g. addition of the new proc. macros blake2, selector_id and selector_bytes.

@Robbepop Robbepop changed the title [WIP] Implement new trait definition codegen Implement new trait definition codegen 🚀 Oct 19, 2021
Copy link
Collaborator

@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.

394 / 406

crates/lang/ir/src/ir/chain_extension.rs Outdated Show resolved Hide resolved
crates/lang/codegen/src/generator/dispatch.rs Outdated Show resolved Hide resolved
crates/lang/ir/src/ir/selector.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants