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

Call a contract #165

Merged
merged 18 commits into from
Sep 24, 2020
Merged

Call a contract #165

merged 18 commits into from
Sep 24, 2020

Conversation

ascjones
Copy link
Contributor

@ascjones ascjones commented Sep 16, 2020

Make the contract call extrinsic work, and update the contracts tests so they can run in parallel and all succeed.

Note: the contracts tests require a node-template based runtime with contracts enabled running.

@dvdplm
Copy link
Contributor

dvdplm commented Sep 18, 2020

This looks interesting. I get Metadata(ModuleNotFound("Contracts")) when I try to run these tests, likely because of how I run the node (client/run.sh).

@ascjones
Copy link
Contributor Author

Yeah the contracts module is not included in the test runtime at the moment. We could consider adding it, at the cost of increasing complexity of the test node.

But it does beg the question as to whether we should add all modules to that, in which case it would just become node-runtime. I don't know the answer to that but we need to figure it out

@dvdplm
Copy link
Contributor

dvdplm commented Sep 21, 2020

But it does beg the question as to whether we should add all modules to that, in which case it would just become node-runtime. I don't know the answer to that but we need to figure it out

Agreed. I'm fine with a heavy-handed interim solution. Can you spell out the pros/cons to using the whole node-runtime?

@ascjones
Copy link
Contributor Author

Well now that there exists tagged releases of substrate on docker: https://hub.docker.com/r/parity/substrate/tags we could use one of those on CI.

So pros: don't need a dependency, or to compile the maintain the whole node runtime. Cons: some CI setup.

I think it could work: #169.

@ascjones ascjones marked this pull request as ready for review September 22, 2020 15:32
@ascjones ascjones requested a review from dvdplm September 22, 2020 15:33
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

Good stuff. I think the account generation and nonce handling stuff is useful enough to warrant making it available to other code as well; can be done in a diff PR though.

@@ -115,44 +116,164 @@ pub struct InstantiatedEvent<T: Contracts> {
pub contract: <T as System>::AccountId,
}

/// Contract execution event.
///
/// Raised upon successful executionFailing of a contract call
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Raised upon successful executionFailing of a contract call
/// Emitted upon successful execution/failure of a contract call

pub struct ContractExecutionEvent<T: Contracts> {
/// Caller of the contract.
pub caller: <T as System>::AccountId,
/// Raw contract event data
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this SCALE encoded data that is runtime dependent or can we say anything at all about the content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The data is the encoded contract event, I will update the comment

.unwrap()
.nonce;
let local_nonce = STASH_NONCE
.fetch_update(Ordering::SeqCst, Ordering::SeqCst, |x| Some(x + 1))
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not fetch_add()?

}

impl TestContext {
async fn init() -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is useful enough that it might be worth making it generic over the runtime and put it in a test-utils.rs for use in other places.

}

/// generate a new keypair for an account, and fund it so it can perform smart contract operations
async fn generate_account(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above: consider making this available to other code.

src/frame/contracts.rs Show resolved Hide resolved
@ascjones
Copy link
Contributor Author

the account generation and nonce handling stuff is useful enough to warrant making it available to other code as well; can be done in a diff PR though.

#176

@dvdplm dvdplm merged commit 385d214 into master Sep 24, 2020
dvdplm added a commit that referenced this pull request Sep 24, 2020
dvdplm added a commit that referenced this pull request Sep 24, 2020
@ascjones ascjones deleted the aj-contract-call branch September 25, 2020 07:14
dvdplm added a commit that referenced this pull request Oct 5, 2020
* Upgrade to substrate 2.0.0

* WIP implement Subcommand manually (see paritytech/substrate#6894 (comment))

* Add pallet-staking/std to the std feature

* Sort out the subcommand impl

* Sort out the module index (ty @ascjones)
Sort out the RefCount type (ty @dvc94ch)
Random tweaks to make test-node more similar to the vanilla node-template

* obey the fmt

* Add changelog and bump versions

* Merge #165 and update CHANGELOG

* Update test-node/runtime/src/lib.rs

Co-authored-by: Demi Marie Obenour <demiobenour@gmail.com>

* Update CHANGELOG.md

Co-authored-by: Demi Marie Obenour <demiobenour@gmail.com>

Co-authored-by: Demi Marie Obenour <demiobenour@gmail.com>
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.

2 participants