Refactor implementation of call builders for traits #983

xgreenx · 2021-10-25T13:06:15Z

Before that pull request #[ink_lang::contract] generated the implementation of call builders for the contract. That generated code used ink_lang::codegen::TraitCallForwarderFor<TraitInfoType> to implement traits defined via #[ink_lang::trait_definition]. But if traits are foreign(defined in the another crate) it caused a compilation error that two implementation of ink_lang::codegen::TraitCallForwarderFor<TraitInfoType1> and ink_lang::codegen::TraitCallForwarderFor<TraitInfoType2> are conflicting.

The reason the types were conflicting (despite being different) is because of rust-lang/rust#51445.

The idea of the change is to generate the blanket implementation of the traits for call builders during the definition of the traits(In the same crate where the trait is defined). It will prevent the compilation error because __ink_TraitInfo is not foreign.

Closes #982.

polkadot address: 1nNaTpU9GHFvF7ZrSMu2CudQjXftR8Aqx58oMDgcuoH8dKe

xgreenx · 2021-10-27T11:36:09Z

I think the problem is that in the case of the local crate we know all implementation of the trait, so we know that __ink_TraitInfo is a local type. In the case of another crate, it is a foreign type. So it can have conflicts to ink_lang::codegen::TraitCallForwarderFor.

xgreenx · 2021-11-02T15:48:11Z

I implemented idea from the comment. To show that it works=) It is not a final change, I need validation from @Robbepop and @cmichi. If they agree, I will implement it fully. Also maybe you want to use this approach in several places.

I tested delegator example - it works. Also tests with two traits defined in separate crate works.

HCastano

Hey @xgreenx!

I'm gonna try and get up to speed to this so we can move forward with it. So from what I understand the problem is that right now ink! does not support contracts which implement multiple external traits. However, it does support contracts with:

Multiple internal traits (so defined by the contract itself)
A single external trait

From the compiler error it looks like this has to do with the structure of the codegen
around the CallBuilder.

I still need to dig into the code, but can you describe (at a high level) how the solution you propose here with the extra trait implementation fixes this problem?

Also, can you either merge or rebase master?

examples/trait-incrementer/traits/Cargo.toml

examples/trait-incrementer/traits/lib.rs

examples/trait-incrementer/traits/Cargo.toml

xgreenx · 2022-01-26T23:04:43Z

I still need to dig into the code, but can you describe (at a high level) how the solution you propose here with the extra trait implementation fixes this problem?

At the moment the code generates the implementation of traits for CallBuilder in the body of the contract(#[ink::contract]).

The idea of the suggested fix is to define a generic implementation for T: ::ink_lang::reflect::ContractEnv + ::ink_lang::ToAccountId<<T as ::ink_lang::reflect::ContractEnv>::Env> + ::ink_lang::reflect::InkStruct during the trait definition. All these traits will be implemented for CallBuilder in the body of the contract -> The CallBuilder will use a generic implementation.

So in two words, I suggest implementing traits for CallBuilder on the level of trait definition instead of the body of the contract.

Also, can you either merge or rebase master?

I can do that, but if you are okay with the idea of the change, I can implement it fully(clean up the code, update tests). I did a draft implementation to show that it fixes the problem and still creates a right CallBuidler.

HCastano · 2022-01-27T04:46:15Z

I can do that, but if you are okay with the idea of the change, I can implement it fully(clean up the code, update tests). I did a draft implementation to show that it fixes the problem and still creates a right CallBuidler.

I need to understand the original code before I can assess if your solution should go beyond a draft, so hang tight for now

HCastano · 2022-01-28T04:13:46Z

Okay, so I'm still trying to wrap my head around the root of the problem, maybe you can help me understand.

So again, the compiler complains about "conflicting implementations of trait ink_lang::codegen::TraitCallForwarderFor<_> for type incrementer::_::CallBuilder".

From the expanded example contract you provided, here are the two places where we try and implement TraitCallForwarderFor

Since the compiler is complaining, this implies that:

let T1 = <::ink_lang::reflect::TraitDefinitionRegistry<Environment> as Increment>::__ink_TraitInfo;
let T2 = <::ink_lang::reflect::TraitDefinitionRegistry<Environment> as Reset>::__ink_TraitInfo;
T1 == T2

Otherwise the trait implementations would be different.

The definitions for <TraitDefinitionRegistry as $trait>::__ink_TraitInfo can be found here:

This leaves us with two different structs, __ink_TraitInfoIncrement<E> and __ink_TraitInfoReset<E>.

Since these are different types, the trait implementations should be different. What am I missing here?

xgreenx · 2022-01-28T14:34:39Z

Everything is right. The code generates the implementation for the Callbuilder:

impl ::ink_lang::codegen::TraitCallForwarderFor<#trait_info> for #cb_ident

#trait_info here is <::ink_lang::reflect::TraitDefinitionRegistry<Environment> as #trait_path>::__ink_TraitInfo and it is a problem. If the trait is foreign(defined in external create) then the compiler doesn't know that type exactly in __ink_TraitInfo. It can __ink_TraitInfoIncrement<E> and __ink_TraitInfoReset<E>, because it is external. So it is why these two implementation conflicts with each other. The __ink_TraitInfo is foreign and it is limitation of the rust.

When two traits are defined in the crate of the contract then __ink_TraitInfo is a local type and compilers know all possible implementations.

HCastano · 2022-01-31T03:55:37Z

Okay, so I was pretty stumped here and luckily @ascjones came to the rescue identifying this as rust-lang/rust#51445.

So the first fix that comes to mind for me is to have the macro extract the concrete type behind __ink_TraitInfo and use that instead. This should be a lot less code. Andrew do you have any other suggestions here?

xgreenx · 2022-01-31T06:59:40Z

So the first fix that comes to mind for me is to have the macro extract the concrete type behind __ink_TraitInfo and use that instead

Could you elaborate more on it? Because I didn't get it=)

HCastano · 2022-01-31T15:21:25Z

So the first fix that comes to mind for me is to have the macro extract the concrete type behind __ink_TraitInfo and use that instead

Could you elaborate more on it? Because I didn't get it=)

So instead of the macro generating <Foo as Bar>::__ink_TraitInfo we get it to generate __ink_TraitInfoIncrement<E> instead (the issue is around the use of the full syntax). It's kinda like we're hardcoding the type there, haha 😅

xgreenx · 2022-01-31T23:22:59Z

So instead of the macro generating ::__ink_TraitInfo we get it to generate __ink_TraitInfoIncrement instead (the issue is around the use of the full syntax). It's kinda like we're hardcoding the type there, haha 😅

The problem is that __ink_TraitInfoIncrement<E> is a new generated type and we don't know the exact path to that type -> The user should import that type somehow or we need to get the path to that type, to be able to use it in the #[ink_lang::contract].

It is why it was initially moved to <Foo as Bar>::__ink_TraitInfo. During the generation of the contract, we have access to the trait(because the user imported it) -> we have access to the type alias __ink_TraitInfo.

Your idea requires the user to import an additional generated type or explicitly write a path to the trait(in that case we can use the same prefix for the type like ::path::Increment -> ::path::__ink_TraitInfoIncrement).

HCastano

Okay, so I get how you're trying to solve this. I'm not a huge fan of the blanket impl, but I get why you're doing it. If possible, I'd look for ways to tighten the scope of the implementation up a bit.

Another thing I noticed, there's no more implementation of TraitCallForwarderFor for CallBuilder. I wonder if that's going to have any unintended consequences.

Anyways, I'm okay moving forward with this approach, so feel free to merge master and clean it up once you get a chance

crates/lang/src/reflect/trait_def/info.rs

paritytech-cicd-pr · 2022-02-08T16:13:00Z

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

These are the results when building the examples/* contracts from this branch with cargo-contract 0.17.0-942c50c and comparing them to ink! master:

	Total Optimized Size	Total Used Gas
`accumulator`	1.36 K
`adder`	2.47 K
`contract-terminate`	1.23 K	215_704
`contract-transfer`	8.47 K	15_704
`delegator`	6.86 K	50_833
`dns`	9.70 K	47_112
`erc1155`	28.12 K	94_224
`erc20`	9.34 K	47_112
`erc721`	13.17 K	125_632
`flipper`	1.63 K	15_704
`incrementer`	1.56 K	15_704
`multisig`	26.25 K	102_922
`proxy`	3.36 K	32_147
`rand-extension`	4.64 K	15_704
`subber`	2.48 K
`trait-erc20`	9.64 K	47_112
`trait-flipper`	1.41 K	15_704
`trait-incrementer`	1.54 K	31_408

Link to the run | Last update: Wed Feb 16 23:42:16 CET 2022

HCastano

Couple of questions, but I think it looks alright

HCastano · 2022-02-09T02:05:29Z

crates/lang/codegen/src/enforced_error.rs

-/// We insert markers for these errors in the generated contract code.
-/// This is necessary since we can't check these errors at compile time
-/// of the contract.
-/// `cargo-contract` checks the contract code for these error markers
-/// when building a contract and fails if it finds markers.


We'll need a follow up in cargo-contract to remove these checks

It is used in the implementation of the to+account_id for TraitDefinitionRegistry. So I moved that comment there=)

crates/lang/codegen/src/generator/trait_def/trait_registry.rs

crates/lang/src/reflect/trait_def/info.rs

crates/lang/src/reflect/trait_def/registry.rs

crates/lang/src/reflect/trait_def/info.rs

codecov-commenter · 2022-02-09T17:43:09Z

Codecov Report

Merging #983 (4a7a906) into master (d35b3d7) will decrease coverage by 3.19%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #983      +/-   ##
==========================================
- Coverage   78.72%   75.53%   -3.20%     
==========================================
  Files         253      252       -1     
  Lines        9409     9257     -152     
==========================================
- Hits         7407     6992     -415     
- Misses       2002     2265     +263

Impacted Files	Coverage Δ
crates/lang/codegen/src/generator/arg_list.rs	`100.00% <ø> (ø)`
...odegen/src/generator/as_dependency/call_builder.rs	`100.00% <ø> (ø)`
...odegen/src/generator/as_dependency/contract_ref.rs	`100.00% <ø> (ø)`
crates/lang/codegen/src/generator/mod.rs	`100.00% <ø> (ø)`
.../codegen/src/generator/trait_def/call_forwarder.rs	`100.00% <ø> (ø)`
crates/lang/codegen/src/lib.rs	`100.00% <ø> (ø)`
.../codegen/src/generator/trait_def/trait_registry.rs	`100.00% <100.00%> (+1.19%)`	⬆️
crates/lang/ir/src/ir/contract.rs	`0.00% <0.00%> (-100.00%)`	⬇️
crates/lang/ir/src/ir/ink_test.rs	`0.00% <0.00%> (-87.50%)`	⬇️
crates/lang/ir/src/ir/trait_def/mod.rs	`0.00% <0.00%> (-66.67%)`	⬇️
... and 26 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 d35b3d7...4a7a906. Read the comment docs.

HCastano

Still trying to clarify things around the __ink_enforce_error thing

examples/delegator/accumulator/lib.rs

examples/trait-incrementer/traits/lib.rs

crates/lang/codegen/src/generator/trait_def/trait_registry.rs

crates/lang/src/reflect/trait_def/registry.rs

crates/lang/src/reflect/trait_def/info.rs

HCastano · 2022-02-10T04:24:57Z

Oh, also a couple of thoughs.

Do you mind updating the PR name and description to explain what this change is and why its important?
Can you split out your commits a bit more, makes reviewing a bit easier
Can you avoid force-pushes if possible? Slightly streamlines pulling new changes down. We also squash the PRs anyways, so the commit history doesn't have to be super clean

xgreenx · 2022-02-10T08:37:03Z

Oh, also a couple of thoughs.

Do you mind updating the PR name and description to explain what this change is and why its important?

Can you split out your commits a bit more, makes reviewing a bit easier

Can you avoid force-pushes if possible? Slightly streamlines pulling new changes down. We also squash the PRs anyways, so the commit history doesn't have to be super clean

Done
It is hard to do because I initially did it like one commit. If something is not clear you can ask=)
I'm force-pushing because I'm rebasing it to be actual with master=)

HCastano

Some small doc fixes, but I think this is good to go 😄

crates/lang/src/reflect/trait_def/info.rs

crates/lang/codegen/src/generator/trait_def/trait_registry.rs

crates/lang/src/reflect/trait_def/registry.rs

examples/trait-incrementer/traits/lib.rs

crates/lang/src/reflect/trait_def/registry.rs

HCastano

Looks good, thanks!

Gonna see if we can get another quick look (@ascjones 😃?) before merging

ascjones · 2022-02-14T09:48:51Z

Looks good, thanks!

Gonna see if we can get another quick look (@ascjones smiley?) before merging

Yes I am looking at this today!

ascjones

The main issue I have is that this is quite a significant refactoring to solve this particular issue. To this end I explored a few alternatives to see whether it is possible to fix it with a much smaller change. The one I came up with is #1136. Please have a look and let me know what you think of that. There may be some issues with it that I have not considered.

TriplEight · 2022-02-17T19:23:29Z

@xgreenx how CI team can help in this review?

cmichi · 2022-02-17T19:43:29Z

@TriplEight The CI team was requested automatically for review, since you added yourself as code owners a while ago. I think there were some temporary changes to the GitLab config to comment some stages out, but now no longer. I'm removing the request for you to review.

ascjones · 2022-02-21T09:58:11Z

@xgreenx #1141 is merged now, which fixes the conflicting impls issue which I understand was the main motivator for this change.

However you did say that there were other motivations for this refactor which aren't mentioned here:

The idea of my change also was to reduce the redundant of the code. We have 3 places with the same code that can be covered with blanked implementation. + We don't need an introduction of the utility trait TraitCallForwarderFor.
Also, it was preparation for future change that we want to suggest, but we can refactor it there=)
Also, we planned to use that blanked implementation in OpenBrush to not generate the same code at the fourth time.

So up to you how you want to proceed - I suggest to close this and open a fresh PR when the blanket implementation is actually required for the future change, and we can review it in the context of that and removing code redundancy.

cmichi · 2022-02-22T10:00:23Z

@xgreenx Thanks a lot for finding the bug in the first place, providing a solution and partaking in all the discussions on how to fix it! Even though we ended up closing your PR with the first approach to fix the bug, we wouldn't have gotten to the final solution without you and the discussions we had with you on possible solutions.

We want to give you a tip for all of that. Thank you!

Could you edit your description of this PR and add either one of those?

polkadot address: <SS58 Address>

or

kusama address: <SS58 Address>

cmichi · 2022-02-22T15:44:14Z

/tip medium

shawntabrizi · 2022-02-22T21:10:16Z

/tip medium

substrate-tip-bot · 2022-02-22T21:10:22Z

A medium tip was successfully submitted for xgreenx (1nNaTpU9GHFvF7ZrSMu2CudQjXftR8Aqx58oMDgcuoH8dKe on polkadot).

https://polkadot.js.org/apps/#/treasury/tips

xgreenx mentioned this pull request Oct 27, 2021

New trait_definition fails if to use two traits. #982

Closed

xgreenx mentioned this pull request Nov 25, 2021

Release ink! 3.0.0-rc7 #1043

Merged

HCastano reviewed Jan 26, 2022

View reviewed changes

examples/trait-incrementer/traits/Cargo.toml Outdated Show resolved Hide resolved

examples/trait-incrementer/traits/lib.rs Outdated Show resolved Hide resolved

examples/trait-incrementer/traits/Cargo.toml Outdated Show resolved Hide resolved

HCastano reviewed Feb 3, 2022

View reviewed changes

crates/lang/src/reflect/trait_def/info.rs Outdated Show resolved Hide resolved

xgreenx requested review from ascjones, cmichi, Robbepop and a team as code owners February 8, 2022 14:46

xgreenx force-pushed the two_traits_fails branch 3 times, most recently from a39b980 to d5cecc4 Compare February 8, 2022 15:36

HCastano reviewed Feb 9, 2022

View reviewed changes

xgreenx added 2 commits February 9, 2022 22:50

Fully implemented blanket implementation for CallBuilder

5410a5b

Fixes according comments in PR

bf251aa

xgreenx force-pushed the two_traits_fails branch from d5cecc4 to bf251aa Compare February 9, 2022 21:03

HCastano reviewed Feb 10, 2022

View reviewed changes

xgreenx changed the title ~~Updated example with traits to cause a compilation error~~ Refactor implementation of call builders for traits Feb 10, 2022

Added comments to traits

77ece96

HCastano reviewed Feb 11, 2022

View reviewed changes

Applied suggestions from the PR

e567408

HCastano approved these changes Feb 11, 2022

View reviewed changes

xgreenx and others added 2 commits February 17, 2022 00:03

Merge branch 'master' into two_traits_fails

486d323

Fix after merge with master

4a7a906

xgreenx mentioned this pull request Feb 17, 2022

Release ink! 3.0.0-rc9 #1135

Merged

ascjones mentioned this pull request Feb 17, 2022

Use impl index as const generic parameter for CallBuilder trait impls #1136

Closed

ascjones reviewed Feb 17, 2022

View reviewed changes

cmichi removed the request for review from a team February 17, 2022 19:43

ascjones mentioned this pull request Feb 18, 2022

Unique trait id as a parameter for CallBuilder trait impls #1141

Merged

xgreenx closed this Feb 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor implementation of call builders for traits #983

Refactor implementation of call builders for traits #983

xgreenx commented Oct 25, 2021 •

edited

Loading

xgreenx commented Oct 27, 2021 •

edited

Loading

xgreenx commented Nov 2, 2021 •

edited

Loading

HCastano left a comment

xgreenx commented Jan 26, 2022

HCastano commented Jan 27, 2022

HCastano commented Jan 28, 2022

xgreenx commented Jan 28, 2022

HCastano commented Jan 31, 2022 •

edited

Loading

xgreenx commented Jan 31, 2022

HCastano commented Jan 31, 2022

xgreenx commented Jan 31, 2022 •

edited

Loading

HCastano left a comment

paritytech-cicd-pr commented Feb 8, 2022 •

edited

Loading

HCastano left a comment

HCastano Feb 9, 2022

xgreenx Feb 9, 2022

codecov-commenter commented Feb 9, 2022 •

edited

Loading

HCastano left a comment

HCastano commented Feb 10, 2022

xgreenx commented Feb 10, 2022

HCastano left a comment

HCastano left a comment

ascjones commented Feb 14, 2022

ascjones left a comment

TriplEight commented Feb 17, 2022

cmichi commented Feb 17, 2022

ascjones commented Feb 21, 2022

cmichi commented Feb 22, 2022

cmichi commented Feb 22, 2022

shawntabrizi commented Feb 22, 2022

substrate-tip-bot bot commented Feb 22, 2022

Refactor implementation of call builders for traits #983

Refactor implementation of call builders for traits #983

Conversation

xgreenx commented Oct 25, 2021 • edited Loading

xgreenx commented Oct 27, 2021 • edited Loading

xgreenx commented Nov 2, 2021 • edited Loading

HCastano left a comment

Choose a reason for hiding this comment

xgreenx commented Jan 26, 2022

HCastano commented Jan 27, 2022

HCastano commented Jan 28, 2022

xgreenx commented Jan 28, 2022

HCastano commented Jan 31, 2022 • edited Loading

xgreenx commented Jan 31, 2022

HCastano commented Jan 31, 2022

xgreenx commented Jan 31, 2022 • edited Loading

HCastano left a comment

Choose a reason for hiding this comment

paritytech-cicd-pr commented Feb 8, 2022 • edited Loading

🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑

HCastano left a comment

Choose a reason for hiding this comment

HCastano Feb 9, 2022

Choose a reason for hiding this comment

xgreenx Feb 9, 2022

Choose a reason for hiding this comment

codecov-commenter commented Feb 9, 2022 • edited Loading

Codecov Report

HCastano left a comment

Choose a reason for hiding this comment

HCastano commented Feb 10, 2022

xgreenx commented Feb 10, 2022

HCastano left a comment

Choose a reason for hiding this comment

HCastano left a comment

Choose a reason for hiding this comment

ascjones commented Feb 14, 2022

ascjones left a comment

Choose a reason for hiding this comment

TriplEight commented Feb 17, 2022

cmichi commented Feb 17, 2022

ascjones commented Feb 21, 2022

cmichi commented Feb 22, 2022

cmichi commented Feb 22, 2022

shawntabrizi commented Feb 22, 2022

substrate-tip-bot bot commented Feb 22, 2022

xgreenx commented Oct 25, 2021 •

edited

Loading

xgreenx commented Oct 27, 2021 •

edited

Loading

xgreenx commented Nov 2, 2021 •

edited

Loading

HCastano commented Jan 31, 2022 •

edited

Loading

xgreenx commented Jan 31, 2022 •

edited

Loading

paritytech-cicd-pr commented Feb 8, 2022 •

edited

Loading

codecov-commenter commented Feb 9, 2022 •

edited

Loading