-
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
Clean up CallBuilder
return()
type
#1525
Changes from all commits
b211af1
5b9b1f1
65edbfe
caba49c
6e00bae
a258897
d8fb710
cb22885
313780d
2710f17
f03b929
5a6934d
1716f88
1b6beec
b3f8888
09e1b53
a662d99
782ed2e
ffd3926
84b5924
2c21791
63d7b1c
51af07e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -109,7 +109,27 @@ where | |
/// Invokes the contract with the given built-up call parameters. | ||
/// | ||
/// Returns the result of the contract execution. | ||
/// | ||
/// # Panics | ||
/// | ||
/// This method panics if it encounters an [`ink_primitives::LangError`]. If you want to handle | ||
/// those use the [`try_invoke`][`CallParams::try_invoke`] method instead. | ||
pub fn invoke(&self) -> Result<R, crate::Error> { | ||
crate::invoke_contract(self).map(|inner| { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should take the chance to also unwrap the error from pallet-contracts while we are breaking things. Let's just make the default path as safe and frictionless as possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah good idea. I'll do this in a small follow-up so that the Edit: done in #1602 |
||
inner.unwrap_or_else(|lang_error| { | ||
panic!("Cross-contract call failed with {:?}", lang_error) | ||
}) | ||
}) | ||
} | ||
|
||
/// Invokes the contract with the given built-up call parameters. | ||
/// | ||
/// Returns the result of the contract execution. | ||
/// | ||
/// # Note | ||
/// | ||
/// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller. | ||
pub fn try_invoke(&self) -> Result<ink_primitives::MessageResult<R>, crate::Error> { | ||
crate::invoke_contract(self) | ||
} | ||
} | ||
|
@@ -173,7 +193,7 @@ where | |
/// ) | ||
/// .returns::<()>() | ||
/// .fire() | ||
/// .unwrap(); | ||
/// .expect("Got an error from the Contract's pallet."); | ||
/// ``` | ||
/// | ||
/// ## Example 2: With Return Value | ||
|
@@ -209,7 +229,7 @@ where | |
/// ) | ||
/// .returns::<i32>() | ||
/// .fire() | ||
/// .unwrap(); | ||
/// .expect("Got an error from the Contract's pallet."); | ||
/// ``` | ||
/// | ||
/// ## Example 3: Delegate call | ||
|
@@ -237,7 +257,47 @@ where | |
/// ) | ||
/// .returns::<i32>() | ||
/// .fire() | ||
/// .unwrap(); | ||
/// .expect("Got an error from the Contract's pallet."); | ||
/// ``` | ||
/// | ||
/// # Handling `LangError`s | ||
/// | ||
/// It is also important to note that there are certain types of errors which can happen during | ||
/// cross-contract calls which can be handled know as [`LangError`][`ink_primitives::LangError`]. | ||
/// | ||
/// If you want to handle these errors use the [`CallBuilder::try_fire`] methods instead of the | ||
/// [`CallBuilder::fire`] ones. | ||
/// | ||
/// **Note:** The shown examples panic because there is currently no cross-calling | ||
/// support in the off-chain testing environment. However, this code | ||
/// should work fine in on-chain environments. | ||
/// | ||
/// ## Example: Handling a `LangError` | ||
/// | ||
/// ```should_panic | ||
/// # use ::ink_env::{ | ||
/// # Environment, | ||
/// # DefaultEnvironment, | ||
/// # call::{build_call, Selector, ExecutionInput} | ||
/// # }; | ||
/// # use ink_env::call::Call; | ||
/// # type AccountId = <DefaultEnvironment as Environment>::AccountId; | ||
/// # type Balance = <DefaultEnvironment as Environment>::Balance; | ||
/// let call_result = build_call::<DefaultEnvironment>() | ||
/// .call_type( | ||
/// Call::new() | ||
/// .callee(AccountId::from([0x42; 32])) | ||
/// .gas_limit(5000) | ||
/// .transferred_value(10), | ||
/// ) | ||
/// .try_fire() | ||
/// .expect("Got an error from the Contract's pallet."); | ||
/// | ||
/// match call_result { | ||
/// Ok(_) => unimplemented!(), | ||
/// Err(e @ ink_primitives::LangError::CouldNotReadInput) => unimplemented!(), | ||
/// Err(_) => unimplemented!(), | ||
/// } | ||
/// ``` | ||
#[allow(clippy::type_complexity)] | ||
pub fn build_call<E>() -> CallBuilder< | ||
|
@@ -597,9 +657,23 @@ where | |
E: Environment, | ||
{ | ||
/// Invokes the cross-chain function call. | ||
/// | ||
/// # Panics | ||
/// | ||
/// This method panics if it encounters an [`ink_primitives::LangError`]. If you want to handle | ||
/// those use the [`try_fire`][`CallBuilder::try_fire`] method instead. | ||
pub fn fire(self) -> Result<(), Error> { | ||
self.params().invoke() | ||
} | ||
|
||
/// Invokes the cross-chain function call. | ||
/// | ||
/// # Note | ||
/// | ||
/// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be more accurate to clarify that the inner Result will be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, I'll fix it in the follow-up that removes the |
||
pub fn try_fire(self) -> Result<ink_primitives::MessageResult<()>, Error> { | ||
self.params().try_invoke() | ||
} | ||
} | ||
|
||
impl<E> | ||
|
@@ -626,9 +700,23 @@ where | |
R: scale::Decode, | ||
{ | ||
/// Invokes the cross-chain function call and returns the result. | ||
/// | ||
/// # Panics | ||
/// | ||
/// This method panics if it encounters an [`ink_primitives::LangError`]. If you want to handle | ||
/// those use the [`try_fire`][`CallBuilder::try_fire`] method instead. | ||
pub fn fire(self) -> Result<R, Error> { | ||
self.params().invoke() | ||
} | ||
|
||
/// Invokes the cross-chain function call and returns the result. | ||
/// | ||
/// # Note | ||
/// | ||
/// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller. | ||
pub fn try_fire(self) -> Result<ink_primitives::MessageResult<R>, Error> { | ||
self.params().try_invoke() | ||
} | ||
} | ||
|
||
impl<E, Args, R> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,39 +1,39 @@ | ||
error[E0277]: the trait bound `NonCodec: WrapperTypeEncode` is not satisfied | ||
--> tests/ui/trait_def/fail/message_output_non_codec.rs:6:26 | ||
| | ||
6 | fn message(&self) -> NonCodec; | ||
| ^^^^^^^^ the trait `WrapperTypeEncode` is not implemented for `NonCodec` | ||
| | ||
= help: the following other types implement trait `WrapperTypeEncode`: | ||
&T | ||
&mut T | ||
Arc<T> | ||
Box<T> | ||
Cow<'a, T> | ||
Rc<T> | ||
String | ||
Vec<T> | ||
parity_scale_codec::Ref<'a, T, U> | ||
= note: required for `NonCodec` to implement `Encode` | ||
--> tests/ui/trait_def/fail/message_output_non_codec.rs:6:26 | ||
| | ||
6 | fn message(&self) -> NonCodec; | ||
| ^^^^^^^^ the trait `WrapperTypeEncode` is not implemented for `NonCodec` | ||
| | ||
= help: the following other types implement trait `WrapperTypeEncode`: | ||
&T | ||
&mut T | ||
Arc<T> | ||
Box<T> | ||
Cow<'a, T> | ||
Rc<T> | ||
String | ||
Vec<T> | ||
parity_scale_codec::Ref<'a, T, U> | ||
= note: required for `NonCodec` to implement `Encode` | ||
note: required by a bound in `DispatchOutput` | ||
--> src/codegen/dispatch/type_check.rs | ||
| | ||
| T: scale::Encode + 'static; | ||
| ^^^^^^^^^^^^^ required by this bound in `DispatchOutput` | ||
--> src/codegen/dispatch/type_check.rs | ||
| | ||
| T: scale::Encode + 'static; | ||
| ^^^^^^^^^^^^^ required by this bound in `DispatchOutput` | ||
|
||
error[E0599]: the method `fire` exists for struct `CallBuilder<E, Set<Call<E>>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<NonCodec>>>`, but its trait bounds were not satisfied | ||
--> tests/ui/trait_def/fail/message_output_non_codec.rs:5:5 | ||
| | ||
1 | pub struct NonCodec; | ||
| ------------------- doesn't satisfy `NonCodec: parity_scale_codec::Decode` | ||
--> tests/ui/trait_def/fail/message_output_non_codec.rs:5:5 | ||
| | ||
1 | pub struct NonCodec; | ||
| ------------------- doesn't satisfy `NonCodec: parity_scale_codec::Decode` | ||
... | ||
5 | #[ink(message)] | ||
| ^ method cannot be called on `CallBuilder<E, Set<Call<E>>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<NonCodec>>>` due to unsatisfied trait bounds | ||
| | ||
= note: the following trait bounds were not satisfied: | ||
`NonCodec: parity_scale_codec::Decode` | ||
5 | #[ink(message)] | ||
| ^ method cannot be called on `CallBuilder<E, Set<Call<E>>, Set<ExecutionInput<ArgumentList<ArgumentListEnd, ArgumentListEnd>>>, Set<ReturnType<NonCodec>>>` due to unsatisfied trait bounds | ||
| | ||
= note: the following trait bounds were not satisfied: | ||
`NonCodec: parity_scale_codec::Decode` | ||
note: the following trait must be implemented | ||
--> $CARGO/parity-scale-codec-3.2.1/src/codec.rs | ||
| | ||
| pub trait Decode: Sized { | ||
| ^^^^^^^^^^^^^^^^^^^^^^^ | ||
--> $CARGO/parity-scale-codec-3.2.2/src/codec.rs | ||
| | ||
| pub trait Decode: Sized { | ||
| ^^^^^^^^^^^^^^^^^^^^^^^ |
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 know not related to this PR but do we really need these standalone functions? I don't think we want multiple ways to do the same thing.
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'm not sure how much use external developers are making of these specific APIs, but it's
probably important to leave them in. This at least gives developers a chance to optimize
their contracts, and third-party tooling to improve upon our abstractions.
A counter-argument here might be that this API is already opinionated (e.g the use of
CallParams
) and doesn't give developers enough room to optimize or improve upon it. Inthis case, maybe we want to expose even lower level APIs, but I'm not sure we want to go
down that route
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 can't see how those stand alone function are more low level. Both operate on the same type
CallParams
. The difference is just inherent vs free function. This is just a mechanical change.