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

Fix bugs in two examples + Fix master CI #820

Merged
merged 8 commits into from
Jun 17, 2021
Merged

Fix bugs in two examples + Fix master CI #820

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
09e8ec1
Enable debug output for `contract-transfer` example
cmichi Jun 16, 2021
9b051d2
Enable sending payments to `fn invoke_transaction()`
cmichi Jun 16, 2021
cca614b
Rename `Error::UnknownError` to `Error::Unknown`
cmichi Jun 16, 2021
690bbdd
Revert "Enable sending payments to `fn invoke_transaction()`"
cmichi Jun 17, 2021
2d4368c
Require sending transaction payment to `fn invoke_transaction()`
cmichi Jun 17, 2021
3891d79
Fix interpunctuation
cmichi Jun 17, 2021
e61673a
Introduce `ink-debug` feature
cmichi Jun 17, 2021
9ed01e6
Revert "Introduce `ink-debug` feature"
cmichi Jun 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions crates/engine/src/ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ macro_rules! define_error_codes {
$name = $discr,
)*
/// Returns if an unknown error was received from the host module.
UnknownError,
Unknown,
}

impl From<ReturnCode> for Result {
Expand All @@ -61,7 +61,7 @@ macro_rules! define_error_codes {
$(
$discr => Err(Error::$name),
)*
_ => Err(Error::UnknownError),
_ => Err(Error::Unknown),
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/env/src/engine/experimental_off_chain/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ impl CryptoHash for Keccak256 {
impl From<ext::Error> for crate::Error {
fn from(ext_error: ext::Error) -> Self {
match ext_error {
ext::Error::UnknownError => Self::UnknownError,
ext::Error::Unknown => Self::Unknown,
ext::Error::CalleeTrapped => Self::CalleeTrapped,
ext::Error::CalleeReverted => Self::CalleeReverted,
ext::Error::KeyNotFound => Self::KeyNotFound,
Expand Down
4 changes: 2 additions & 2 deletions crates/env/src/engine/on_chain/ext.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ macro_rules! define_error_codes {
$name = $discr,
)*
/// Returns if an unknown error was received from the host module.
UnknownError,
Unknown,
}

impl From<ReturnCode> for Result {
Expand All @@ -46,7 +46,7 @@ macro_rules! define_error_codes {
$(
$discr => Err(Error::$name),
)*
_ => Err(Error::UnknownError),
_ => Err(Error::Unknown),
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/env/src/engine/on_chain/impls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ impl CryptoHash for Keccak256 {
impl From<ext::Error> for Error {
fn from(ext_error: ext::Error) -> Self {
match ext_error {
ext::Error::UnknownError => Self::UnknownError,
ext::Error::Unknown => Self::Unknown,
ext::Error::CalleeTrapped => Self::CalleeTrapped,
ext::Error::CalleeReverted => Self::CalleeReverted,
ext::Error::KeyNotFound => Self::KeyNotFound,
Expand Down
2 changes: 1 addition & 1 deletion crates/env/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ pub enum Error {
/// The account that was called is either no contract (e.g. user account) or is a tombstone.
NotCallable,
/// An unknown error has occurred.
UnknownError,
Unknown,
/// The call to `seal_debug_message` had no effect because debug message
/// recording was disabled.
LoggingDisabled,
Expand Down
2 changes: 1 addition & 1 deletion examples/contract-transfer/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ edition = "2018"
[dependencies]
ink_primitives = { version = "3.0.0-rc3", path = "../../crates/primitives", default-features = false }
ink_metadata = { version = "3.0.0-rc3", path = "../../crates/metadata", default-features = false, features = ["derive"], optional = true }
ink_env = { version = "3.0.0-rc3", path = "../../crates/env", default-features = false }
ink_env = { version = "3.0.0-rc3", path = "../../crates/env", default-features = false, features = [ "ink-debug" ] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

A better way was to introduce an ink-debug feature and forward that to ink_env.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So the problem with this approach is that cargo-contract builds with --no-default-features. But there is a ticket to forward an ink-debug feature: use-ink/cargo-contract#284.

But for now it means that if somebody builds contract-transfer with cargo-contract right now we need to explicitly enable the feature for ink_env like done in this PR, otherwise the user will not see the debug output.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should raise the priority of the --debug flag on cargo-contract then.

ink_storage = { version = "3.0.0-rc3", path = "../../crates/storage", default-features = false }
ink_lang = { version = "3.0.0-rc3", path = "../../crates/lang", default-features = false }
ink_prelude = { version = "3.0.0-rc3", path = "../../crates/prelude", default-features = false }
Expand Down
7 changes: 6 additions & 1 deletion examples/multisig_plain/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,15 +484,20 @@ mod multisig_plain {

/// Invoke a confirmed execution without getting its output.
///
/// If the transaction which is invoked transfers value, this value has
/// to be sent as payment with this call. The method will fail otherwise,
/// and the transaction would then be reverted.
///
/// Its return value indicates whether the called transaction was successful.
/// This can be called by anyone.
#[ink(message)]
#[ink(message, payable)]
cmichi marked this conversation as resolved.
Show resolved Hide resolved
pub fn invoke_transaction(
&mut self,
trans_id: TransactionId,
) -> Result<(), Error> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As a side note: There is another issue with this method. It internally uses

.map_err(|_| Error::TransactionFailed);

which currently would not revert the transaction. This will only happen once we have implemented error forwarding.
Currently, in this error case, the transaction would be taken (i.e. removed) out of the StorageStash, but not be executed due to the error. It would then not be possible to give the execution another try, since the transaction has been removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah this is a very important point made and acts as yet another motivator to implement storage reversal on error for non-ink-as-dependency calls. In the meantime we are required to panic in these critical cases.

self.ensure_confirmed(trans_id);
let t = self.take_transaction(trans_id).expect(WRONG_TRANSACTION_ID);
assert!(self.env().transferred_balance() == t.transferred_value);
let result = build_call::<<Self as ::ink_lang::ContractEnv>::Env>()
.callee(t.callee)
.gas_limit(t.gas_limit)
Expand Down