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

runtime-sdk/modules/contracts: Propagate errors in WASM lib calls #894

Merged
merged 1 commit into from
Apr 21, 2022

Conversation

kostko
Copy link
Member

@kostko kostko commented Apr 20, 2022

No description provided.

@kostko kostko added the m:contracts Module: contracts label Apr 20, 2022
@codecov
Copy link

codecov bot commented Apr 20, 2022

Codecov Report

Merging #894 (2651c5a) into main (43fe1f9) will decrease coverage by 0.03%.
The diff coverage is 53.33%.

@@            Coverage Diff             @@
##             main     #894      +/-   ##
==========================================
- Coverage   67.60%   67.57%   -0.04%     
==========================================
  Files         122      123       +1     
  Lines        9678     9708      +30     
==========================================
+ Hits         6543     6560      +17     
- Misses       3110     3123      +13     
  Partials       25       25              
Impacted Files Coverage Δ
...time-sdk/modules/contracts/src/abi/oasis/crypto.rs 4.93% <0.00%> (-0.33%) ⬇️
runtime-sdk/modules/contracts/src/lib.rs 68.22% <32.00%> (-1.64%) ⬇️
...ime-sdk/modules/contracts/src/abi/oasis/storage.rs 57.69% <54.54%> (-1.08%) ⬇️
runtime-sdk/modules/contracts/src/abi/mod.rs 100.00% <100.00%> (ø)
runtime-sdk/modules/contracts/src/abi/oasis/mod.rs 64.54% <100.00%> (+1.58%) ⬆️
...untime-sdk/modules/contracts/src/abi/oasis/test.rs 100.00% <100.00%> (ø)
runtime-sdk/modules/contracts/src/results.rs 80.23% <100.00%> (+0.23%) ⬆️

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 43fe1f9...2651c5a. Read the comment docs.

Copy link
Contributor

@pro-wh pro-wh left a comment

Choose a reason for hiding this comment

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

I haven't worked with the contracts module, probably gonna rely on someone else to review.

question about this PR though: we do a lot of error propagation through Results, whereas here it's an Option in a context structure. Is it better this way? Maybe are there limitations with having it as part of an error type? It looks like, for one thing, we're locked into a wasm3-specific error enum.

@kostko
Copy link
Member Author

kostko commented Apr 20, 2022

question about this PR though: we do a lot of error propagation through Results, whereas here it's an Option in a context structure. Is it better this way? Maybe are there limitations with having it as part of an error type? It looks like, for one thing, we're locked into a wasm3-specific error enum.

The problem is that this is about propagating errors from inside functions that are called from within WASM (e.g. imported WASM modules provided by the Oasis ABI). You cannot directly propagate an error as you can only trigger an interpreter abort. So we need to store the error in the execution context structure that can be inspected after the overall call into WASM returns.

@kostko kostko force-pushed the kostko/feature/wasm-aborts branch from 36e212e to 2651c5a Compare April 21, 2022 08:56
@kostko kostko enabled auto-merge April 21, 2022 08:59
@kostko kostko merged commit 668a3fc into main Apr 21, 2022
@kostko kostko deleted the kostko/feature/wasm-aborts branch April 21, 2022 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
m:contracts Module: contracts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants