Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

contracts: Refactor the runtime API in order to simplify node integration #7409

Merged
13 commits merged into from
Oct 29, 2020

Conversation

athei
Copy link
Member

@athei athei commented Oct 26, 2020

Currently, in order for a node to implement the contracts runtime api it is necessary to copy paste some conversion logic:

impl pallet_contracts_rpc_runtime_api::ContractsApi<Block, AccountId, Balance, BlockNumber>
 		for Runtime
{
	fn call(
		origin: AccountId,
		dest: AccountId,
		value: Balance,
		gas_limit: u64,
		input_data: Vec<u8>,
	) -> ContractExecResult {
		let (exec_result, gas_consumed) =
			Contracts::bare_call(origin, dest.into(), value, gas_limit, input_data);
		match exec_result {
			Ok(v) => ContractExecResult::Success {
				flags: v.flags.bits(),
				data: v.data,
				gas_consumed: gas_consumed,
			},
			Err(_) => ContractExecResult::Error,
		}
	}
}

This PR reduces it to:

impl pallet_contracts_rpc_runtime_api::ContractsApi<Block, AccountId, Balance, BlockNumber>
 		for Runtime
{
	fn call(
		origin: AccountId,
		dest: AccountId,
		value: Balance,
		gas_limit: u64,
		input_data: Vec<u8>,
	) -> pallet_contracts_primitives::ContractExecResult {
		Contracts::bare_call(origin, dest, value, gas_limit, input_data)
	}
}

It does this by moving common types to the pallet-contracts-primitives crate and by that removing the need for intermediate types that made those conversions necessary.

I think this is important because this conversion logic is duplicated in every runtime and is also part of a tutorial that can be simplified by doing so (@danforbes @wheresaddie).

Because this modifies a runtime API a client update is necessary. This API is only used by a the contracts_call RPC and is therefore not critical: It is only relevant for public RPC nodes. Nonetheless I bumped the priority to medium to signal that a client update is necessary to restore this RPC functionality.

@athei athei added A0-please_review Pull request needs code review. B3-apinoteworthy C3-medium PR touches the given topic and has a medium impact on builders. labels Oct 26, 2020
@athei athei requested a review from pepyakin October 26, 2020 14:17
@bkchr
Copy link
Member

bkchr commented Oct 26, 2020

Because this modifies a runtime API a client update is necessary. This API is only used by a the contracts_call RPC and is therefore not critical: It is only relevant for public RPC nodes. Nonetheless I bumped the priority to medium to signal that a client update is necessary to restore this RPC functionality.

If the runtime upgrade and the client upgrade are done together, there shouldn't be any problem. Otherwise, there will be always a period of time until both are in sync to make it work again.

frame/contracts/common/src/lib.rs Outdated Show resolved Hide resolved
frame/contracts/rpc/src/lib.rs Show resolved Hide resolved
Co-authored-by: Andrew Jones <ascjones@gmail.com>
Copy link
Contributor

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

LGTM

wheresaddie and others added 2 commits October 29, 2020 14:56
Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
wheresaddie and others added 2 commits October 29, 2020 14:57
Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
Co-authored-by: Nikolay Volf <nikvolf@gmail.com>
Copy link
Contributor

@NikVolf NikVolf left a comment

Choose a reason for hiding this comment

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

Other than some minor formatting grumbles, looks great!

frame/contracts/src/wasm/runtime.rs Outdated Show resolved Hide resolved
@athei
Copy link
Member Author

athei commented Oct 29, 2020

bot merge

@ghost
Copy link

ghost commented Oct 29, 2020

Waiting for commit status.

Copy link
Contributor

@wheresaddie wheresaddie left a comment

Choose a reason for hiding this comment

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

👍

@ghost
Copy link

ghost commented Oct 29, 2020

Checks failed; merge aborted.

@athei
Copy link
Member Author

athei commented Oct 29, 2020

bot merge

@ghost
Copy link

ghost commented Oct 29, 2020

Waiting for commit status.

@ghost ghost merged commit 30ecb9b into master Oct 29, 2020
@ghost ghost deleted the at-contract-rpc branch October 29, 2020 14:57
ascjones added a commit to paritytech/canvas that referenced this pull request Oct 29, 2020
ascjones added a commit to paritytech/canvas that referenced this pull request Oct 30, 2020
* Use substrate dependencies from master

* Cargo update

* Fix up compiler errors

* Update Cargo.lock

* Update contract call rpc api boilerplate. Rel paritytech/substrate#7409

* Fix up service

* Update runtime

* Update README
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. C3-medium PR touches the given topic and has a medium impact on builders.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants