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

Add example which uses ext_transfer + ext_terminate #554

Merged
merged 20 commits into from
Oct 30, 2020

Conversation

cmichi
Copy link
Collaborator

@cmichi cmichi commented Oct 29, 2020

Closes #355 and #356.

@cmichi cmichi requested a review from Robbepop October 29, 2020 14:01

let contract_id = self.account_id::<T>().expect("could not decode account id");
self.accounts.remove_account::<T>(contract_id);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What the on-chain impl would do here is set a tombstone with a code hash and remove storage. AFAIU both is not easily achievable with our current off-chain env, hence I left it out here for the moment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you maybe provide a small dev. note comment explaining exactly this?

crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/env/src/backend.rs Outdated Show resolved Hide resolved
crates/env/src/backend.rs Outdated Show resolved Hide resolved
Comment on lines 405 to 418
use std::panic;

let value_any = panic::catch_unwind($should_terminate)
.expect_err("contract did not terminate");
let encoded_input: &Vec<u8> =
value_any.downcast_ref::<Vec<u8>>().expect("must work");
let info: ink_env::test::ContractTerminationResult<AccountId, Balance> =
scale::Decode::decode(&mut &encoded_input[..]).expect("must work");

let expected_beneficiary: AccountId = $beneficiary;
assert_eq!(info.beneficiary, expected_beneficiary);

let expected_balance: Balance = $balance;
assert_eq!(info.transferred, expected_balance);
Copy link
Collaborator

Choose a reason for hiding this comment

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

very clever and creative solution! might be acceptable. however, needs further macro hygiene improvements since this is going to be exposed to the users.

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

codecov-io commented Oct 30, 2020

Codecov Report

Merging #554 into master will increase coverage by 0.33%.
The diff coverage is 20.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #554      +/-   ##
==========================================
+ Coverage   67.94%   68.28%   +0.33%     
==========================================
  Files         155      155              
  Lines        6811     6811              
==========================================
+ Hits         4628     4651      +23     
+ Misses       2183     2160      -23     
Impacted Files Coverage Δ
crates/env/src/engine/off_chain/db/accounts.rs 85.71% <ø> (+10.38%) ⬆️
crates/env/src/engine/off_chain/impls.rs 55.07% <0.00%> (ø)
crates/env/src/engine/off_chain/test_api.rs 70.73% <ø> (ø)
crates/env/src/engine/off_chain/typed_encoded.rs 75.67% <ø> (ø)
crates/lang/ir/src/ir/item/event.rs 68.65% <0.00%> (ø)
crates/lang/ir/src/ir/item_impl/mod.rs 68.14% <0.00%> (ø)
crates/lang/src/env_access.rs 0.00% <ø> (ø)
crates/lang/ir/src/ir/attrs.rs 85.43% <100.00%> (ø)
...ates/storage/src/collections/hashmap/fuzz_tests.rs 94.52% <0.00%> (-5.48%) ⬇️
...rates/storage/src/collections/binary_heap/tests.rs 92.00% <0.00%> (-2.00%) ⬇️
... and 6 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 aedd756...688faab. Read the comment docs.

Comment on lines +37 to +39
pub fn terminate_me(&mut self) {
self.env().terminate_contract(self.env().caller());
}
Copy link
Collaborator Author

@cmichi cmichi Oct 30, 2020

Choose a reason for hiding this comment

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

The correct version of this would be:

Suggested change
pub fn terminate_me(&mut self) {
self.env().terminate_contract(self.env().caller());
}
pub fn terminate_me(&mut self) -> ! {
self.env().terminate_contract(self.env().caller())
}

We then run into this though:

error[E0277]: the trait bound `!: scale_info::TypeInfo` is not satisfied

Wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for the explanation!


let contract_id = self.account_id::<T>().expect("could not decode account id");
self.accounts.remove_account::<T>(contract_id);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you maybe provide a small dev. note comment explaining exactly this?

crates/env/src/engine/off_chain/test_api.rs Outdated Show resolved Hide resolved
crates/env/src/engine/off_chain/test_api.rs Outdated Show resolved Hide resolved
Comment on lines +37 to +39
pub fn terminate_me(&mut self) {
self.env().terminate_contract(self.env().caller());
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks for the explanation!

examples/contract-terminate/lib.rs Outdated Show resolved Hide resolved
Comment on lines 42 to 46
impl Default for JustTerminate {
fn default() -> Self {
Self::new()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why? And why not flag it as #[ink(constructor)]?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clippy made me do it!

Copy link
Collaborator

Choose a reason for hiding this comment

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

And what did clippy say exactly? 🙃

Copy link
Collaborator Author

@cmichi cmichi Oct 30, 2020

Choose a reason for hiding this comment

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

It said https://www.youtube.com/watch?v=ZXsQAXx_ao0 🙃.

Okay, actually it was this one.

examples/contract-transfer/lib.rs Outdated 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.

LGTM

Thanks for the new example!

///
/// See `examples/contract-terminate` for a complete usage example.
#[cfg(feature = "std")]
pub fn assert_contract_termination<T, F>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

So much better as a function compared to a macro!

@cmichi cmichi merged commit 89620e7 into master Oct 30, 2020
@cmichi cmichi deleted the cmichi-add-terminate-transfer-example branch October 30, 2020 19:15
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.

Create example contract that uses ext_transfer
3 participants