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

Clean up CallBuilder return() type #1525

Merged
merged 23 commits into from
Jan 19, 2023
Merged

Conversation

HCastano
Copy link
Contributor

@HCastano HCastano commented Nov 30, 2022

In #1450 I had to change the return type used by CallBuilder callers in order to be
able to encode/decode the new MessageResult return type.

This PR bakes the assumption that all messages must return a MessageResult into the
into the CallBuilder's call stack, allowing callers to specify only the message return
type again. I.e, instead of returns::<Result<R, LangError>>() you can again specify just
returns::<R>().

A breaking change that ended up being introduced here is that ink_env::invoke_contract now
returns a MessageResult<R>, whereas it previously returned just R.

I'm not sure if we need to do anything for for the DelegateCall implementation, but we can
leave that as a follow-up since there are already examples of people stumbling due to (the
lack of) this: use-ink/ink-workshop#31.

@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2023

Codecov Report

Merging #1525 (2c21791) into master (8ed95ea) will decrease coverage by 0.07%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##           master    #1525      +/-   ##
==========================================
- Coverage   71.59%   71.52%   -0.08%     
==========================================
  Files         205      205              
  Lines        6292     6296       +4     
==========================================
- Hits         4505     4503       -2     
- Misses       1787     1793       +6     
Impacted Files Coverage Δ
crates/e2e/src/client.rs 36.36% <ø> (ø)
crates/env/src/api.rs 36.92% <0.00%> (ø)
crates/env/src/backend.rs 78.12% <ø> (ø)
crates/env/src/call/call_builder.rs 0.00% <0.00%> (ø)
crates/env/src/engine/off_chain/impls.rs 47.64% <ø> (ø)
...odegen/src/generator/as_dependency/contract_ref.rs 100.00% <ø> (ø)
crates/ink/src/env_access.rs 9.09% <ø> (ø)
...odegen/src/generator/as_dependency/call_builder.rs 100.00% <100.00%> (ø)
crates/allocator/src/bump.rs 89.07% <0.00%> (-1.01%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@HCastano HCastano marked this pull request as ready for review January 17, 2023 00:31
crates/env/src/call/call_builder.rs Outdated Show resolved Hide resolved
crates/env/src/call/call_builder.rs Outdated Show resolved Hide resolved
Comment on lines -271 to +273
pub fn invoke_contract<E, Args, R>(params: &CallParams<E, Call<E>, Args, R>) -> Result<R>
pub fn invoke_contract<E, Args, R>(
params: &CallParams<E, Call<E>, Args, R>,
) -> Result<ink_primitives::MessageResult<R>>
Copy link
Contributor

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.

Copy link
Contributor Author

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. In
this case, maybe we want to expose even lower level APIs, but I'm not sure we want to go
down that route

Copy link
Contributor

@athei athei Jan 19, 2023

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.

pub fn invoke(&self) -> Result<R, crate::Error> {
crate::invoke_contract(self).map(|inner| {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@HCastano HCastano Jan 18, 2023

Choose a reason for hiding this comment

The 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 CreateBuilder methods can also be included in the clean-up.

Edit: done in #1602

///
/// # Note
///
/// On failure this returns an [`ink_primitives::LangError`] which can be handled by the caller.
Copy link
Collaborator

Choose a reason for hiding this comment

The 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 LangError, and the outer Result is an environmental error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I'll fix it in the follow-up that removes the EnvError from the default execution path

@HCastano HCastano merged commit e2715f7 into master Jan 19, 2023
@HCastano HCastano deleted the hc-clean-up-call-builder-return branch January 19, 2023 22:53
HCastano added a commit that referenced this pull request Jan 23, 2023
* Assume that `CallBuilder`s return a `MessageResult`

* Add `try_*` variants to `CallBuilder`

* Add doc test showing how to handle `LangError` from `build_call`

* Remove TODO related to `delegate_call`

* Account for `LangError` in E2E tests

* Improve `return_value` error message

* Remove extra `expect` from E2E tests

* Add test for checking that `fire` panics on `LangError`

* Fix spelling

* Remove extra `unwrap` in more E2E tests

* Fix ERC-1155 example

* Fix `invoke_contract` doc test

* RustFmt

* Fix `delegator` example

* Update ERC-20 tests

* Indicate that doc test panics in off-chain env

* Forgot some commas 🤦

* Get `Flipper` example compiling again

* Remove more unwraps

* Update UI tests

* Print out `LangError` when panicking after `invoke`

* Bump `scale` to fix UI tests
@HCastano HCastano mentioned this pull request Jan 24, 2023
@ascjones ascjones mentioned this pull request Feb 15, 2023
@DoubleOTheven
Copy link
Contributor

The change mentioned above: returns a MessageResult<R>, whereas it previously returned just R has been incredibly helpful on the front end. It is probably my favorite feature that ink! offers over Solidity. Nice work 👏🏼

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.

5 participants