-
Notifications
You must be signed in to change notification settings - Fork 432
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
CallBuilder
delegated calls API
#1133
Conversation
@@ -420,9 +426,28 @@ impl TypedEnvBackend for EnvInstance { | |||
unimplemented!("off-chain environment does not support contract invocation") | |||
} | |||
|
|||
#[allow(clippy::type_complexity)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can create an alias to avoid type_complexity
everywhere=)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, the compiler gives a warning that the type bounds are not checked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can do the following:
type CallParamsTypedef<T, A> =
CallParams<T, DelegateCall<T, <T as Environment>::Hash>, A, ()>;
// -- snip ---
fn invoke_contract_delegate<T, Args>(
&mut self,
params: &CallParamsTypedef<T, Args>,
) -> Result<()>
where
T: Environment,
Args: scale::Encode,
{
let _code_hash = params.code_hash();
unimplemented!("off-chain environment does not support contract invocation")
}
Notice that in the typedef we used fully qualified syntax to specify the trait being used. We should be able to do this everywhere in this PR
Can you please satisfy the CI? |
@athei I will fix the errors I get now, but I remember some tests were failing in the |
The ink waterfall doesn't even start because it is failing before it comes to it.
For the new examples you added we would need manually ad them to the |
Codecov Report
@@ Coverage Diff @@
## master #1133 +/- ##
==========================================
- Coverage 78.43% 78.39% -0.05%
==========================================
Files 231 231
Lines 8760 8760
==========================================
- Hits 6871 6867 -4
- Misses 1889 1893 +4
Continue to review full report at Codecov.
|
@athei, please see that the CI completes before I update to the latest master. |
examples/proxy/lib.rs
Outdated
const PROXY_FIELDS_STORAGE_KEY: [u8; 32] = ink_lang::blake2x256!("ProxyFields"); | ||
|
||
impl SpreadLayout for ProxyFields { | ||
const FOOTPRINT: u64 = | ||
<AccountId as SpreadLayout>::FOOTPRINT + <Hash as SpreadLayout>::FOOTPRINT; | ||
const REQUIRES_DEEP_CLEAN_UP: bool = false; | ||
|
||
fn pull_spread(_: &mut KeyPtr) -> Self { | ||
let mut ptr = KeyPtr::from(Key::from(PROXY_FIELDS_STORAGE_KEY)); | ||
Self { | ||
delegate_to: SpreadLayout::pull_spread(&mut ptr), | ||
admin: SpreadLayout::pull_spread(&mut ptr), | ||
} | ||
} | ||
|
||
fn push_spread(&self, _: &mut KeyPtr) { | ||
let mut ptr = KeyPtr::from(Key::from(PROXY_FIELDS_STORAGE_KEY)); | ||
SpreadLayout::push_spread(&self.delegate_to, &mut ptr); | ||
SpreadLayout::push_spread(&self.admin, &mut ptr); | ||
} | ||
|
||
fn clear_spread(&self, _: &mut KeyPtr) { | ||
let mut ptr = KeyPtr::from(Key::from(PROXY_FIELDS_STORAGE_KEY)); | ||
SpreadLayout::clear_spread(&self.delegate_to, &mut ptr); | ||
SpreadLayout::clear_spread(&self.admin, &mut ptr); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to repeat some docs on this impl why it is needed (to not conflict with the storage of the called into contract).
This is kind of ugly. But it could be resolved by adding a new attribute to the #[ink(storage)]
macro that allows changing the root key, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, but maybe it is okay for now because it will be fixed with #1134
|
||
#[ink(storage)] | ||
pub struct Flipper { | ||
value: Upgradable<bool, NotInitialized>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add docs why and what this Upgradable
does. It is an example after all. The point is teaching things.
The whole point of this example is to teach people how to write upgradeable contracts.
crates/env/src/call/call_builder.rs
Outdated
where | ||
E: Environment, | ||
{ | ||
/// The account ID of the to-be-called smart contract. | ||
callee: E::AccountId, | ||
env: PhantomData<fn() -> E>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please call it _phantom
and move it to the bottom of the struct. Please don't skip doc comments.
crates/env/src/call/call_builder.rs
Outdated
#[allow(dead_code)] // it is used in the `invoke_contract_delegate` / `eval_contract_delegate` functions. | ||
pub(crate) fn exec_input(&self) -> &ExecutionInput<Args> { | ||
&self.exec_input | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this duplication necessary? Couldn't you have a single impl
block where you put the shared functions with a generic CallType
? Same for call_flags
.
crates/env/src/call/call_builder.rs
Outdated
{ | ||
/// The default call type for cross-contract calls. Performs a cross-contract call to `callee` | ||
/// with gas limit `gas_limit`, transferring `transferred_value` of currency. | ||
pub struct Call<E, Callee, GasLimit, TransferredValue> { | ||
env: PhantomData<fn() -> E>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like above should be called _phantom
.
@VargSupercolony Thanks for the PR. The I would prefer if you leave the existing |
@VargSupercolony We're shaping up for a stable ink! release ‒ hopefully this week. It would be awesome to have this PR in there! Do you think you can implement the comments this week? |
hey @cmichi @athei, writing from my personal acc as I don't have much internet access. I will not be able to work with the PR in the coming days because of war, and I parted my ways with Supercolony, so I think someone will pick it up - probably @xgreenx :) but anything you need from me - please mention this acc, this is my personal one |
Yes, I will work on it, I will try to update it today=) |
cc95ca4
to
b42ed34
Compare
@cmichi I returned the |
I'm still looking into it, but AFAIU it has nothing to do with this PR, the tests actually all run through. The CI only complains when shutting down. I currently suspect some combination of protected variables and first time external contribution. Let's ignore the |
e8d1722
to
5d2c14b
Compare
# Needed until https://github.com/paritytech/ink/issues/364 is resolved. | ||
[profile.release] | ||
overflow-checks = false | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Needed until https://github.com/paritytech/ink/issues/364 is resolved. | |
[profile.release] | |
overflow-checks = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No longer needed since #1049.
You will receive the respective `upgradeable_flipper.contract` bundle in the `examples/upgradable-contract/upgradeable_flipper/target/ink/` folder. | ||
|
||
In order to perform migrations and have proxy working for contracts with different storage | ||
layouts, we use the [`Upgradeable`](upgradeable_flipper/upgradeable.rs) type wrapper, which ensures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have the term "upgradeable" and "upgradable" in this PR. @ascjones 🇬🇧 Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will rename everything to upgradeable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've had this debate over on the Substrate side, and American spelling should be used 🇺🇸
🦑 📈 ink! Example Contracts ‒ Changes Report 📉 🦑
These are the results when building the
Link to the run | Last update: Fri Mar 4 09:14:40 CET 2022 |
I've found the issue which caused the (It was because this PR adds a new example, which is not yet tested in the waterfall. The regression bot which posted above went 💥 in that case.) |
fn allocate_packed(&mut self, at: &Key) { | ||
<T as PackedAllocate>::allocate_packed(&mut self.inner, at) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course, we will use only one trait(maybe two), and that trait will be automatically implemented by #[ink::storage_ext]
. All keys will be managed by the user during the definition of the struct, so the code should look tiny=)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah all of this boilerplate is only needed because the current system doesn't allow to change the contract root key without manually implementing SpreadLayout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The boilerplate, oh my 🙈
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only took a quick peek today, will need to spend more time on it later in the week.
One thing that comes to mind is that we should really look for ways to simplify that example contract. If I were a newcomer to ink!
and saw this as the base-case for upgradeable contracts I'd definitely be scared off
fn allocate_packed(&mut self, at: &Key) { | ||
<T as PackedAllocate>::allocate_packed(&mut self.inner, at) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The boilerplate, oh my 🙈
With #1134 we can remove the But with #1134 we need to provide a way how to init the contract's storae if is not initialized. For example, argument in macro |
…invoke_contract`, `invoke_contract_delegate`
Fixes according comments in PR.
91b36cd
to
8057cc6
Compare
@xgreenx can you avoid force-pushing PRs? It makes pulling the changes down easier, and also makes it harder to see what commits are new. It's fine to merge master in order to keep up to date |
I'm only forcing it to rebase to master. I will try to not force for simple conflicts=) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the right direction, but needs a bit of clean-up
@@ -210,7 +202,7 @@ where | |||
/// # type AccountId = <DefaultEnvironment as Environment>::AccountId; | |||
/// # type Balance = <DefaultEnvironment as Environment>::Balance; | |||
/// build_call::<DefaultEnvironment>() | |||
/// .set_call_type(Call::new().set_callee(AccountId::from([0x42; 32])).set_gas_limit(5000).set_transferred_value(10)) | |||
/// .set_call_type(Call::new().callee(AccountId::from([0x42; 32])).gas_limit(5000).transferred_value(10)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bump
@@ -420,9 +426,28 @@ impl TypedEnvBackend for EnvInstance { | |||
unimplemented!("off-chain environment does not support contract invocation") | |||
} | |||
|
|||
#[allow(clippy::type_complexity)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we can do the following:
type CallParamsTypedef<T, A> =
CallParams<T, DelegateCall<T, <T as Environment>::Hash>, A, ()>;
// -- snip ---
fn invoke_contract_delegate<T, Args>(
&mut self,
params: &CallParamsTypedef<T, Args>,
) -> Result<()>
where
T: Environment,
Args: scale::Encode,
{
let _code_hash = params.code_hash();
unimplemented!("off-chain environment does not support contract invocation")
}
Notice that in the typedef we used fully qualified syntax to specify the trait being used. We should be able to do this everywhere in this PR
crates/env/src/call/call_builder.rs
Outdated
pub(crate) fn gas_limit(&self) -> &Gas { | ||
&self.call_type.gas_limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change this to return a reference? It's Copy
anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes sense in this case. We just end up dereferencing Gas
immediately anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we can return E::Balance
from transferred_value
=) It is u128
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but let's leave that for a follow up
examples/upgradeable-contract/upgradeable-flipper/upgradeable.rs
Outdated
Show resolved
Hide resolved
examples/upgradeable-contract/upgradeable-flipper/upgradeable.rs
Outdated
Show resolved
Hide resolved
const REQUIRES_DEEP_CLEAN_UP: bool = <T as SpreadLayout>::REQUIRES_DEEP_CLEAN_UP; | ||
|
||
fn pull_spread(ptr: &mut KeyPtr) -> Self { | ||
if ink_env::get_contract_storage::<T>(ptr.advance_by(0)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ink_env::get_contract_storage::<T>(ptr.advance_by(0)) | |
if ink_env::get_contract_storage::<T>(ptr.key()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
impl<T: SpreadAllocate + PackedLayout> SpreadAllocate for Upgradeable<T, Initialized> { | ||
fn allocate_spread(ptr: &mut KeyPtr) -> Self { | ||
Upgradeable::new(<T as SpreadAllocate>::allocate_spread(ptr)) | ||
} | ||
} | ||
|
||
impl<T: SpreadAllocate + PackedLayout> SpreadAllocate for Upgradeable<T, NotInitialized> { | ||
fn allocate_spread(ptr: &mut KeyPtr) -> Self { | ||
Upgradeable::new(<T as SpreadAllocate>::allocate_spread(ptr)) | ||
} | ||
} | ||
|
||
impl<T: PackedAllocate> PackedAllocate for Upgradeable<T, Initialized> { | ||
fn allocate_packed(&mut self, at: &Key) { | ||
<T as PackedAllocate>::allocate_packed(&mut self.inner, at) | ||
} | ||
} | ||
|
||
impl<T: PackedAllocate> PackedAllocate for Upgradeable<T, NotInitialized> { | ||
fn allocate_packed(&mut self, at: &Key) { | ||
<T as PackedAllocate>::allocate_packed(&mut self.inner, at) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not make these generic over Upgradeable<T, S>
the implementations are the same for Initialized
and NotInitialized
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we can't, because SpreadAllocate
requires SpreadAllocate
. SpreadAllocate
is implemented only for Upgradeable<T, NotInitialized>
and Upgradeable<T, Initialized>
so we can't create a generic implementation.
examples/upgradeable-contract/upgradeable-flipper/upgradeable.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
The failing ink-waterfall
CI is because it tests some additional ink! examples which don't make sense to be committed into the ink! repo, they don't use the new API yet. So the failing CI stage can be ignored here. It's this one: https://github.com/paritytech/ink-waterfall/blob/master/examples/contract-introspection/lib.rs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think the biggest thing left is fixing any merge conflicts from #1165, and we should be good
return_type: RetType, | ||
/// The default call type for cross-contract calls. Performs a cross-contract call to `callee` | ||
/// with gas limit `gas_limit`, transferring `transferred_value` of currency. | ||
pub struct Call<E: Environment> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I like this more actually 👍
crates/env/src/call/call_builder.rs
Outdated
pub(crate) fn gas_limit(&self) -> &Gas { | ||
&self.call_type.gas_limit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it makes sense in this case. We just end up dereferencing Gas
immediately anyways
pub struct EnvAccess<'a, T> { | ||
/// Tricks the Rust compiler into thinking that we use `T`. | ||
marker: PhantomData<fn() -> &'a T>, | ||
pub struct EnvAccess<'a, E> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get why you renamed T
to E
, but in the future can you at least split this out into its own commit? It adds a lot of unnecessary noise. Tbh it should've been done in a follow-up PR
storage locations, while also tracking the initialization status (e.g., we uploaded | ||
the code on chain, but haven't called the constructor). | ||
|
||
3. Build the proxy contract: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you change all of these numbers to 1
Github is smart enough to render the the numbers in the correct order. Makes it easier when change are introduced (no need to update the entire list)
examples/upgradeable-contract/lib.rs
Outdated
struct ProxyFields { | ||
/// The `Hash` of a contract code where any call that does not match a | ||
/// selector of this contract is forwarded to. | ||
delegate_to: Hash, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I should've been more explicit here, I like the forward_to
term more
# Conflicts: # crates/env/src/api.rs # crates/env/src/backend.rs # crates/env/src/call/call_builder.rs # crates/env/src/engine/off_chain/impls.rs # crates/env/src/engine/on_chain/impls.rs # crates/lang/src/env_access.rs # crates/lang/tests/ui/contract/fail/message-input-non-codec.stderr # crates/lang/tests/ui/trait_def/fail/message_input_non_codec.stderr # examples/erc1155/lib.rs # examples/multisig/lib.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I think this is good to go.
There are some things we should improve in follow-ups, but this will at least let the community play with these new mechanisms.
Thanks again Green!
This PR adds the possibility to make delegated calls into ink!. To achieve this, we:
seal_delegate_call
unstable interface importCallBuilder
to two types of calls:Call
(meaning cross-contract call) andDelegateCall
(meaning delegated calls). This is a breaking change, as now the users must specifycall_type
on the builder manually, as opposed to it being alwaysCall
:)invoke_contract_delegate
&eval_contract_delegate
functions to backend trait & impls: we call those functions when making a delegated callset_call_type
)proxy
example, rework it to usebuild_call
withDelegateCall
call type, add upgradeable flipper to showcase simple upgradeable contract example.Massive thanks to @yarikbratashchuk for providing initial PoCs & ideas for the implementation.