-
Notifications
You must be signed in to change notification settings - Fork 424
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
Conversation
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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>> |
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. In
this 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.
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 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.
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 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. |
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.
It would be more accurate to clarify that the inner Result will be a LangError
, and the outer Result is an environmental error.
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, I'll fix it in the follow-up that removes the EnvError
from the default execution path
* 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
The change mentioned above: |
In #1450 I had to change the return type used by
CallBuilder
callers in order to beable to encode/decode the new
MessageResult
return type.This PR bakes the assumption that all messages must return a
MessageResult
into theinto the
CallBuilder
's call stack, allowing callers to specify only the message returntype again. I.e, instead of
returns::<Result<R, LangError>>()
you can again specify justreturns::<R>()
.A breaking change that ended up being introduced here is that
ink_env::invoke_contract
nowreturns a
MessageResult<R>
, whereas it previously returned justR
.I'm not sure if we need to do anything for for the
DelegateCall
implementation, but we canleave that as a follow-up since there are already examples of people stumbling due to (the
lack of) this: use-ink/ink-workshop#31.