Basic support for `dyn Trait` to allow cross-contract calls only with trait #1673

xgreenx · 2023-02-22T22:30:07Z

The change uses the ideas from the issue. But instead of #[ink(dyn)] proc macro, it uses the contract_ref! macro rule.

The difference between those approaches is that with contract_ref!, we cover all possible use cases of the usage but don't have the highlighting and hinting in the IDEs. While with #[ink(dyn)], we would have basic highlighting for methods from the trait in the IDEs, the usage will be specific and limited by the number of supported syntax constructions.

The contract_ref! doesn't block us from adding #[ink(dyn)] in the future. We can have both because they use the exact mechanism under the hood.

Added contract_ref! macro rule that allows creating a wrapper around the trait defined via #[ink::trait_definition].
Added From<AccountId> for the contract builder and forwarder to simplify the usage.
Added AsRef<AccountId> and AsMut<AccountId> for builder, forwarder, and contract_ref to provide the way modify the wrapper.

…nd the trait defined via `#[ink::trait_definition]`. Added `From<AccountId>` for the contract builder and forwarder to simplify the usage. Added `AsRev<AccountId>` and `AsMut<AccountId>` for builder, forwarder, and contract_ref to provide the way modify the wrapper.

deuszx · 2023-02-23T08:24:48Z

crates/ink/codegen/src/generator/as_dependency/call_builder.rs

@@ -132,6 +132,18 @@ impl CallBuilder<'_> {
                    <AccountId as ::core::clone::Clone>::clone(&self.account_id)
                }
            }
+
+            impl ::core::convert::AsRef<AccountId> for #cb_ident {


<3 such a small change and so much less code on my side :)

ascjones

Very nice.

Just the question mark of whether we need the $env arg on the contract_ref! macro.

crates/ink/codegen/src/generator/trait_def/call_builder.rs

crates/ink/src/contract_ref.rs

ascjones · 2023-02-23T17:42:19Z

integration-tests/trait-dyn-cross-contract-calls/lib.rs

@@ -0,0 +1,106 @@
+#![cfg_attr(not(feature = "std"), no_std)]
+
+/// We can't run E2E test without the contract. So add `Dummy` contract.


Is this the same issue as #1672

No, it is another reason. During running of the cargo test --features e2e-tests ink! tries to build the crate as a contract via cargo-contract. But if it is not a contract, it will fail because it doesn't have ___ink_generate_metadata function=)

That seems like a bug, we shouldn't need the metadata anymore for ink_e2e

Anyway we should attempt to make this more ergonomic for structuring e2e tests in this way

This prompted me to open #1683 which means it doesn't need to generate the metadata anymore.

Not sure it will make this structure work without the dummy contract, but at least it is an immprovement.

See #1691 for a possible solution

…tract`. Added more examples into documentation

ascjones

LGTM

HCastano

Nice, less code that I would've expected to get something working 💪

I still want to look through the E2E tests a bit more, so I'll do that tomorrow

integration-tests/trait-dyn-cross-contract-calls/traits/lib.rs

integration-tests/trait-dyn-cross-contract-calls/traits/Cargo.toml

integration-tests/trait-dyn-cross-contract-calls/contracts/incrementer/Cargo.toml

integration-tests/trait-dyn-cross-contract-calls/contracts/incrementer-caller/Cargo.toml

integration-tests/trait-dyn-cross-contract-calls/Cargo.toml

HCastano · 2023-03-01T03:53:50Z

crates/env/src/types.rs

+///
+/// If someone wants to use another type than [`AccountId`] in the [`Environment::AccountId`],
+/// he should implement this trait for the corresponding type.


Why should this be an option? Doesn't that circumvent the whole point of the trait?

We can add the trait AccountIdGuard as a constraint for the Environment::AccountId, but in this case, it will be breaking change=) But we will force users to implement it

That doesn't really answer my question though. Why would a user want to implement this for a type other an AccountId?

If we don't want this, but don't want to introduce a breaking change, then we shouldn't be highlighting the fact that they can implement this trait on other types

The user wants to implement it if he uses another type for the Environment::AccountId than ink::types::AccountId.

We want this, because it will give the From<AccountId> implementation to the user. If it is not a problem to add constrain for Environment::AccountId, I prefer to add it.

Okay I see what you mean now. I'm gonna update the comment to make this more clear

integration-tests/trait-dyn-cross-contract-calls/contracts/incrementer-caller/lib.rs

integration-tests/trait-dyn-cross-contract-calls/traits/lib.rs

Co-authored-by: Hernando Castano <[email protected]>

HCastano

Couples questions around the integration tests

integration-tests/trait-dyn-cross-contract-calls/lib.rs

crates/ink/src/contract_ref.rs

integration-tests/trait-dyn-cross-contract-calls/lib.rs

integration-tests/trait-dyn-cross-contract-calls/contracts/incrementer/lib.rs

integration-tests/trait-dyn-cross-contract-calls/contracts/incrementer-caller/lib.rs

integration-tests/trait-dyn-cross-contract-calls/lib.rs

HCastano · 2023-03-01T22:49:54Z

crates/env/src/types.rs

+///
+/// If someone wants to use another type than [`AccountId`] in the [`Environment::AccountId`],
+/// he should implement this trait for the corresponding type.


That doesn't really answer my question though. Why would a user want to implement this for a type other an AccountId?

If we don't want this, but don't want to introduce a breaking change, then we shouldn't be highlighting the fact that they can implement this trait on other types

Co-authored-by: Hernando Castano <[email protected]>

HCastano

Cool, thanks 💪

HCastano · 2023-03-03T04:42:14Z

@cmichi wants to take a look at this as well, so let's not ape this in just yet

cmichi · 2023-03-03T04:55:27Z

Please don't merge just yet, I'll take a look this morning as well.

deuszx · 2023-03-03T08:48:02Z

Normally, when working with Rust, dyn Trait are discouraged due to dynamic dispatching that affects the performance. Are there any implications for Wasm programs that use dyn Traits? Is there anything unusual b/c we're interpreting the contracts (wasmi)?

xgreenx · 2023-03-03T10:07:52Z

Normally, when working with Rust, dyn Trait are discouraged due to dynamic dispatching that affects the performance.

With dynamic dispatching, the compiler is unable to optimize the code, and at runtime, it requires additional steps to find the function to execute.

Are there any implications for Wasm programs that use dyn Traits? Is there anything unusual b/c we're interpreting the contracts (wasmi)?

The program will work as usual. Only that can affect the size of the final contract. It may be more or less than without dynamic dispatching(It is based on several factors).

BTW, the current solution doesn't use dynamic dispatching=) it is called dyn Trait, but the truth is that we use the type that implements the Trait trait.

cmichi

Thank you, Green! Just some doc nitpicks.

crates/ink/src/contract_ref.rs

Co-authored-by: Michael Müller <[email protected]>

cmichi · 2023-03-03T10:57:24Z

Oh and Green, CHANGELOG.md entry would be great.

… trait (#1673) * Added `contract_ref!` macro rule that allows to create a wrapper around the trait defined via `#[ink::trait_definition]`. Added `From<AccountId>` for the contract builder and forwarder to simplify the usage. Added `AsRev<AccountId>` and `AsMut<AccountId>` for builder, forwarder, and contract_ref to provide the way modify the wrapper. * Added comments for the method in the example of the `contract_ref!` * Make clippy happy * Make CI happy * Changed the default behaviour to use alias generated by the `ink::contract`. Added more examples into documentation * Apply suggestions from code review Co-authored-by: Hernando Castano <[email protected]> * Apply comments from the PR * Missed the comment * Remove error from the example * Apply suggestions from code review Co-authored-by: Hernando Castano <[email protected]> * Fixed comments from PR * Update comments around `AccountIdGuard` * Apply suggestions from code review Co-authored-by: Michael Müller <[email protected]> * Mentioned the PR in the `Unreleased` section --------- Co-authored-by: Hernando Castano <[email protected]> Co-authored-by: Hernando Castano <[email protected]> Co-authored-by: Michael Müller <[email protected]>

xgreenx requested review from cmichi, ascjones, HCastano and SkymanOne as code owners February 22, 2023 22:30

xgreenx added 3 commits February 22, 2023 22:46

Added comments for the method in the example of the contract_ref!

27b58cf

Make clippy happy

b5ab8ac

Make CI happy

e1f92ac

deuszx reviewed Feb 23, 2023

View reviewed changes

ascjones reviewed Feb 23, 2023

View reviewed changes

Changed the default behaviour to use alias generated by the `ink::con…

e29d408

…tract`. Added more examples into documentation

xgreenx requested review from ascjones and removed request for cmichi, HCastano and SkymanOne February 24, 2023 10:26

ascjones approved these changes Feb 24, 2023

View reviewed changes

cmichi assigned xgreenx Feb 27, 2023

HCastano reviewed Mar 1, 2023

View reviewed changes

xgreenx and others added 4 commits March 1, 2023 11:06

Apply suggestions from code review

687f57f

Co-authored-by: Hernando Castano <[email protected]>

Apply comments from the PR

38782cb

Missed the comment

6352d13

Remove error from the example

c6401b6

HCastano reviewed Mar 1, 2023

View reviewed changes

xgreenx and others added 3 commits March 2, 2023 18:46

Apply suggestions from code review

eaad3a3

Co-authored-by: Hernando Castano <[email protected]>

Fixed comments from PR

7cea918

Update comments around AccountIdGuard

13cf225

HCastano approved these changes Mar 3, 2023

View reviewed changes

cmichi approved these changes Mar 3, 2023

View reviewed changes

Apply suggestions from code review

37494c8

Co-authored-by: Michael Müller <[email protected]>

xgreenx added 2 commits March 3, 2023 10:59

Merge branch 'master' into feature/contract-ref

6e4018b

Mentioned the PR in the Unreleased section

011762e

cmichi merged commit f5f3ebe into use-ink:master Mar 3, 2023

HCastano mentioned this pull request Mar 4, 2023

Re-write cross-contract call documentation use-ink/ink-docs#186

Merged

This was referenced Mar 19, 2023

[Feature] Dynamic trait based contract calling #631

Open

Allow default method implementations in ink! traits #1689

Open

Add clarification for [Wrapper for Traits] section use-ink/ink-docs#188

Merged

SkymanOne mentioned this pull request Mar 23, 2023

4.1.0 Release #1727

Merged

deuszx mentioned this pull request Aug 4, 2023

Generate ink traits out of contract's metadata. Cardinal-Cryptography/ink-wrapper#53

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic support for `dyn Trait` to allow cross-contract calls only with trait #1673

Basic support for `dyn Trait` to allow cross-contract calls only with trait #1673

xgreenx commented Feb 22, 2023 •

edited

Loading

deuszx Feb 23, 2023 •

edited

Loading

ascjones left a comment

ascjones Feb 23, 2023

xgreenx Feb 23, 2023 •

edited

Loading

ascjones Feb 24, 2023

ascjones Feb 24, 2023

ascjones Feb 24, 2023

ascjones Mar 2, 2023

ascjones left a comment

HCastano left a comment

HCastano Mar 1, 2023

xgreenx Mar 1, 2023

HCastano Mar 1, 2023

xgreenx Mar 1, 2023

HCastano Mar 3, 2023

HCastano left a comment

HCastano Mar 1, 2023

HCastano left a comment

HCastano commented Mar 3, 2023

cmichi commented Mar 3, 2023 •

edited

Loading

deuszx commented Mar 3, 2023

xgreenx commented Mar 3, 2023

cmichi left a comment

cmichi commented Mar 3, 2023

		@@ -0,0 +1,106 @@
		#![cfg_attr(not(feature = "std"), no_std)]

		/// We can't run E2E test without the contract. So add `Dummy` contract.

Basic support for dyn Trait to allow cross-contract calls only with trait #1673

Basic support for dyn Trait to allow cross-contract calls only with trait #1673

Conversation

xgreenx commented Feb 22, 2023 • edited Loading

deuszx Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xgreenx Feb 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ascjones left a comment

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

HCastano commented Mar 3, 2023

cmichi commented Mar 3, 2023 • edited Loading

deuszx commented Mar 3, 2023

xgreenx commented Mar 3, 2023

cmichi left a comment

Choose a reason for hiding this comment

cmichi commented Mar 3, 2023

Basic support for `dyn Trait` to allow cross-contract calls only with trait #1673

Basic support for `dyn Trait` to allow cross-contract calls only with trait #1673

xgreenx commented Feb 22, 2023 •

edited

Loading

deuszx Feb 23, 2023 •

edited

Loading

xgreenx Feb 23, 2023 •

edited

Loading

cmichi commented Mar 3, 2023 •

edited

Loading