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

Implement MVP for new off-chain testing engine #712

Merged
merged 37 commits into from
May 10, 2021

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Mar 2, 2021

MVP for #565.

ToDo

  • seal_deposit_event
  • seal_set_storage
  • seal_get_storage
  • seal_clear_storage
  • seal_caller
  • seal_address
  • seal_balance
  • seal_value_transferred
  • seal_println
  • seal_terminate
  • seal_transfer

@cmichi cmichi force-pushed the cmichi-implement-new-offchain-engine-mvp branch from df8e55c to def45c2 Compare March 2, 2021 13:16
crates/engine/LICENSE Outdated Show resolved Hide resolved
crates/engine/src/typed_encoded.rs Outdated Show resolved Hide resolved
@cmichi cmichi marked this pull request as draft March 3, 2021 09:26
.gitlab-ci.yml Outdated Show resolved Hide resolved
crates/engine/src/exec_context.rs Outdated Show resolved Hide resolved
crates/engine/src/ext.rs Outdated Show resolved Hide resolved
crates/engine/src/ext.rs Outdated Show resolved Hide resolved
crates/engine/src/ext.rs Outdated Show resolved Hide resolved
crates/env/src/engine/off_chain/mod.rs Outdated Show resolved Hide resolved
crates/env/src/engine/on_chain/mod.rs Outdated Show resolved Hide resolved
crates/env/src/error.rs Outdated Show resolved Hide resolved
crates/env_types/Cargo.toml Outdated Show resolved Hide resolved
crates/storage/src/alloc/init.rs Outdated Show resolved Hide resolved
@cmichi cmichi force-pushed the cmichi-implement-new-offchain-engine-mvp branch 12 times, most recently from eeb68c3 to bc131ca Compare March 17, 2021 05:49
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.

All in all looks good but there is a fundamental problem with ink_engine trying to expose its API as a singleton through free standing methods that require their engine but instead if should expose its API as a single type (the engine) with methods on the engine so that the ink_env can do what it always did and properly turn it into a thread_local and dispatch between engines.

crates/engine/src/types.rs Outdated Show resolved Hide resolved
crates/engine/src/types.rs Outdated Show resolved Hide resolved
crates/engine/src/types.rs Outdated Show resolved Hide resolved
crates/engine/src/types.rs Outdated Show resolved Hide resolved
crates/engine/src/types.rs Outdated Show resolved Hide resolved
crates/env/src/engine/mod.rs Outdated Show resolved Hide resolved
crates/storage/src/collections/binary_heap/tests.rs Outdated Show resolved Hide resolved
crates/storage/src/collections/hashmap/tests.rs Outdated Show resolved Hide resolved
examples/erc20/lib.rs Show resolved Hide resolved
crates/engine/src/ext.rs Outdated Show resolved Hide resolved
crates/engine/src/exec_context.rs Outdated Show resolved Hide resolved
crates/engine/src/ext.rs Outdated Show resolved Hide resolved
crates/engine/src/ext.rs Outdated Show resolved Hide resolved
crates/engine/src/storage.rs Outdated Show resolved Hide resolved
crates/engine/src/test_api.rs Outdated Show resolved Hide resolved
crates/env/src/engine/experimental_off_chain/impls.rs Outdated Show resolved Hide resolved
fn hash(input: &[u8], output: &mut <Self as HashOutput>::Type) {
type OutputType = [u8; 16];
static_assertions::assert_type_eq_all!(
<Blake2x128 as HashOutput>::Type,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<Blake2x128 as HashOutput>::Type,
<Self as HashOutput>::Type,

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately results in this error:

error[E0401]: can't use generic parameters from outer function
  --> crates/env/src/engine/experimental_off_chain/impls.rs:56:14
   |                                                                           
52 | impl CryptoHash for Blake2x128 {                                                                                                                          
   | ---- `Self` type implicitly declared here, by this `impl`                                                                                                 
...
56 |             <Self as HashOutput>::Type,
   |              ^^^^
   |              |                                                            
   |              use of generic parameter from outer function   
   |              use a type here instead

crates/env/src/engine/experimental_off_chain/test_api.rs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2021

Codecov Report

Merging #712 (813301e) into master (5571724) will decrease coverage by 14.96%.
The diff coverage is 80.33%.

❗ Current head 813301e differs from pull request most recent head 8fec9ae. Consider uploading reports for the commit 8fec9ae to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master     #712       +/-   ##
===========================================
- Coverage   83.33%   68.36%   -14.97%     
===========================================
  Files         163      174       +11     
  Lines        7502     7888      +386     
===========================================
- Hits         6252     5393      -859     
- Misses       1250     2495     +1245     
Impacted Files Coverage Δ
crates/env/src/backend.rs 0.00% <ø> (ø)
crates/storage/src/collections/hashmap/tests.rs 100.00% <ø> (ø)
crates/storage/src/collections/smallvec/tests.rs 100.00% <ø> (ø)
crates/storage/src/collections/stash/tests.rs 100.00% <ø> (ø)
crates/storage/src/collections/vec/tests.rs 100.00% <ø> (ø)
crates/storage/src/lazy/lazy_cell.rs 97.40% <ø> (ø)
...tes/env/src/engine/experimental_off_chain/types.rs 21.05% <21.05%> (ø)
crates/engine/src/ext.rs 65.85% <65.85%> (ø)
crates/engine/src/types.rs 66.66% <66.66%> (ø)
...tes/env/src/engine/experimental_off_chain/impls.rs 80.82% <80.82%> (ø)
... and 66 more

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 19200dd...8fec9ae. Read the comment docs.

crates/engine/src/exec_context.rs Show resolved Hide resolved
crates/engine/src/ext.rs Outdated Show resolved Hide resolved
crates/engine/src/ext.rs Show resolved Hide resolved
crates/engine/src/ext.rs Show resolved Hide resolved
crates/engine/src/ext.rs Show resolved Hide resolved
crates/engine/src/test_api.rs Show resolved Hide resolved
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.

Let's ship it!

@cmichi cmichi merged commit 1a19f93 into master May 10, 2021
@cmichi cmichi deleted the cmichi-implement-new-offchain-engine-mvp branch May 10, 2021 11:32
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.

4 participants