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.

Sign up for GitHub

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

Jump to bottom

Update seal_call to __unstable__ version #960

Merged
merged 13 commits into from
Nov 9, 2021
Merged

Update seal_call to __unstable__ version #960

merged 13 commits into from
Nov 9, 2021

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Oct 15, 2021 •
edited
Loading

@cmichi cmichi requested review from athei and Robbepop October 15, 2021 09:56
@codecov-commenter
Copy link

codecov-commenter commented Oct 15, 2021 •
edited
Loading

Codecov Report

Merging #960 (2c05061) into master (1393502) will increase coverage by 0.02%.
The diff coverage is 73.52%.

❗ Current head 2c05061 differs from pull request most recent head e89f464. Consider uploading reports for the commit e89f464 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #960      +/-   ##
==========================================
+ Coverage   78.80%   78.83%   +0.02%     
==========================================
  Files         244      244              
  Lines        9188     9222      +34     
==========================================
+ Hits         7241     7270      +29     
- Misses       1947     1952       +5
Impacted Files Coverage Δ
crates/env/src/call/call_builder.rs 0.00% <0.00%> (ø)
crates/env/src/call/execution_input.rs 76.92% <ø> (ø)
crates/env/src/call/selector.rs 100.00% <ø> (ø)
...tes/env/src/engine/experimental_off_chain/impls.rs 54.71% <0.00%> (-0.53%) ⬇️
crates/env/src/engine/off_chain/impls.rs 42.60% <0.00%> (-0.38%) ⬇️
crates/env/src/backend.rs 80.64% <100.00%> (+80.64%) ⬆️
...ates/storage/src/collections/hashmap/fuzz_tests.rs 100.00% <0.00%> (+4.25%) ⬆️

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 1393502...e89f464. Read the comment docs.

@cmichi
Copy link
Collaborator Author

cmichi commented Oct 15, 2021 •
edited
Loading

Merging #960 (4bde1c3) into master (284e4e5) will decrease coverage by 14.21%.

Not sure what's going on there or how this can possibly be true.

The ink-waterfall CI failure is expected, since our master is broken with the UI's.

@Robbepop
Copy link
Collaborator

Robbepop commented Oct 15, 2021 •
edited
Loading

Not sure what's going on there or how this can possibly be true.

The coverage reports are broken since quite a while ...

Robbepop
Robbepop requested changes Oct 15, 2021
View reviewed changes
Copy link
Collaborator

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

Not a fan of the introduction of Option<ExecutionInput> since we can do better here with a system that relies on already existing compile time information. Also we could better protect against invalid flags.

crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/env/src/call/call_builder.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/impls.rs Show resolved Hide resolved
athei
athei approved these changes Oct 18, 2021
View reviewed changes
Copy link
Contributor

@athei athei left a comment •
edited
Loading

Choose a reason for hiding this comment

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

To me it looks like it uses the seal_call API correctly. Cannot really comment on the Option situation Robin is raising.

I guess code gen will be changed in a later step to allow for actually setting those flags without going through the CallBuilder?

AFAIK this will change the default behavior of calls to disallow reentering the calling contract. Something to keep in mind.

crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/ext.rs
Comment on lines -222 to -233
pub fn seal_call(
callee_ptr: Ptr32<[u8]>,
callee_len: u32,
gas: u64,
transferred_value_ptr: Ptr32<[u8]>,
transferred_value_len: u32,
input_ptr: Ptr32<[u8]>,
input_len: u32,
output_ptr: Ptr32Mut<[u8]>,
output_len_ptr: Ptr32Mut<u32>,
) -> ReturnCode;

Copy link
Contributor

Choose a reason for hiding this comment

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

We drop support for any stable contracts pallet (without having unstable-interface) set. Not saying this is bad. Just raising awareness.

@cmichi cmichi mentioned this pull request Nov 9, 2021
Open
@cmichi cmichi merged commit 6f3d7c0 into master Nov 9, 2021
@cmichi cmichi deleted the cmichi-implement-unstable-seal-call branch November 9, 2021 07:25
@cmichi cmichi self-assigned this Nov 15, 2021
xgreenx pushed a commit to Supercolony-net/ink that referenced this pull request Feb 8, 2022
@cmichi @xgreenx
Update seal_call to __unstable__ version (use-ink#960)
2dc377e 
* Update `seal_call` to `__unstable__` version

* Add `Debug` derive

* Make `cargo-spellcheck` happy

* Make `into_u32` take `self`

* Update release notes

* Fix crate docs link

* Use type-system encoded version of empty `ExecutionInput`

* Add `const` for getters and setters

* Shorten if-branches logic

* Update release notes

* Remove dead code

* Improve release notes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Robbepop Robbepop Robbepop requested changes

@athei athei athei approved these changes

Assignees

@cmichi cmichi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cmichi @codecov-commenter @Robbepop @athei