-
Notifications
You must be signed in to change notification settings - Fork 745
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 READ_ONLY flag to contract call function #4418
Merged
Merged
Changes from 28 commits
Commits
Show all changes
33 commits
Select commit
Hold shift + click to select a range
6847f88
Add static_call flag to contracts call function
smiasojed b19bc77
Refactoring
smiasojed 161d4ac
Unit test added
smiasojed a96a4aa
Cleanup
smiasojed ae1151e
Fmt
smiasojed 41d753e
Add prdoc
smiasojed 5278c7a
Fix prdoc
smiasojed 15fca3c
Update description
smiasojed 5f9d957
Merge remote-tracking branch 'origin/master' into sm/contracts-static…
smiasojed 7169214
Update prdoc
smiasojed cfd4198
Add read-only protection to call_runtime
smiasojed 0a708a0
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
3369de6
Add test
smiasojed 6ebfafc
Fix fixture description
smiasojed 91d145e
Address comments
smiasojed 2eeed62
Fmt
smiasojed fc15cd6
Revert deposit_event implementation
smiasojed 47f9d76
Cleanup
smiasojed 76e50fc
Update chain extension description
smiasojed b6c378b
Improved description
smiasojed 70c654c
Update prdoc
smiasojed f10a0db
Update substrate/frame/contracts/proc-macro/src/lib.rs
smiasojed ac65bd7
Update substrate/frame/contracts/uapi/src/flags.rs
smiasojed b4ad72b
Update substrate/frame/contracts/src/lib.rs
smiasojed fb8b2dc
Update substrate/frame/contracts/src/exec.rs
smiasojed 03a77bb
Update substrate/frame/contracts/src/wasm/runtime.rs
smiasojed 08b4ca7
Address comments
smiasojed b3571d7
Move read-only condition to runtime
smiasojed c7fa16e
Merge remote-tracking branch 'origin/master' into sm/contracts-static…
smiasojed 48e907b
Merge branch 'master' of https://github.com/paritytech/polkadot-sdk i…
2cb8084
".git/.scripts/commands/bench/bench.sh" --subcommand=pallet --runtime…
d5f8d15
Bump API verision for contracts pallet
smiasojed 21aaa70
Merge remote-tracking branch 'origin/master' into sm/contracts-static…
smiasojed File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0 | ||
# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json | ||
|
||
title: "[pallet_contracts] Add `READ_ONLY` flag to contract `call` function" | ||
|
||
doc: | ||
- audience: Runtime User | ||
description: | | ||
This PR implements the `READ_ONLY` flag to be used as a `Callflag` in the contract `call` function. | ||
The flag indicates that the callee is restricted from modifying the state during call execution. | ||
It is equivalent to Ethereum's [STATICCALL](https://eips.ethereum.org/EIPS/eip-214). | ||
|
||
crates: | ||
- name: pallet-contracts | ||
bump: minor | ||
- name: pallet-contracts-uapi | ||
bump: minor | ||
- name: pallet-contracts-proc-macro | ||
bump: minor |
51 changes: 51 additions & 0 deletions
51
substrate/frame/contracts/fixtures/contracts/call_with_flags_and_value.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,51 @@ | ||
// This file is part of Substrate. | ||
|
||
// Copyright (C) Parity Technologies (UK) Ltd. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
//! This fixture calls the account_id with the flags and value. | ||
#![no_std] | ||
#![no_main] | ||
|
||
use common::input; | ||
use uapi::{HostFn, HostFnImpl as api}; | ||
|
||
#[no_mangle] | ||
#[polkavm_derive::polkavm_export] | ||
pub extern "C" fn deploy() {} | ||
|
||
#[no_mangle] | ||
#[polkavm_derive::polkavm_export] | ||
pub extern "C" fn call() { | ||
input!( | ||
256, | ||
callee_addr: [u8; 32], | ||
flags: u32, | ||
value: u64, | ||
forwarded_input: [u8], | ||
); | ||
|
||
api::call_v2( | ||
uapi::CallFlags::from_bits(flags).unwrap(), | ||
callee_addr, | ||
0u64, // How much ref_time to devote for the execution. 0 = all. | ||
0u64, // How much proof_size to devote for the execution. 0 = all. | ||
None, // No deposit limit. | ||
&value.to_le_bytes(), // Value transferred to the contract. | ||
forwarded_input, | ||
None, | ||
) | ||
.unwrap(); | ||
} |
50 changes: 50 additions & 0 deletions
50
substrate/frame/contracts/fixtures/contracts/read_only_call.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// This file is part of Substrate. | ||
|
||
// Copyright (C) Parity Technologies (UK) Ltd. | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS, | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
// This fixture tests if read-only call works as expected. | ||
#![no_std] | ||
#![no_main] | ||
|
||
use common::input; | ||
use uapi::{HostFn, HostFnImpl as api}; | ||
|
||
#[no_mangle] | ||
#[polkavm_derive::polkavm_export] | ||
pub extern "C" fn deploy() {} | ||
|
||
#[no_mangle] | ||
#[polkavm_derive::polkavm_export] | ||
pub extern "C" fn call() { | ||
input!( | ||
256, | ||
callee_addr: [u8; 32], | ||
callee_input: [u8], | ||
); | ||
|
||
// Call the callee | ||
api::call_v2( | ||
uapi::CallFlags::READ_ONLY, | ||
callee_addr, | ||
0u64, // How much ref_time to devote for the execution. 0 = all. | ||
0u64, // How much proof_size to devote for the execution. 0 = all. | ||
None, // No deposit limit. | ||
&0u64.to_le_bytes(), // Value transferred to the contract. | ||
callee_input, | ||
None, | ||
) | ||
.unwrap(); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As stated before: We can't return that early here. This codes runs before we charge the gas for the host function call invocation itself. This has to be charged even if no work has been performed. You have to move down this statement after:
polkadot-sdk/substrate/frame/contracts/proc-macro/src/lib.rs
Lines 708 to 711 in 89604da
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but my understanding is that the gas charge
charge_gas(crate::wasm::RuntimeCosts::HostFn)
is added before function body (mutating check is added to the function body)Expanded:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh yes. Can you add a test that makes sure this weight is charged when erroring out early?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to test it using fixture:
The fixture is called with n equal to 0 and 1. Both calls result in a trap, but the difference is more significant than just the host function call cost because (I assume) the WASMI execution time differs for both cases. The difference is the host function call cost plus 15%.
HostFn ref_time is 72994
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can call the
tokens
function on the gas meter to check which tokens were charged.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in priv, we do not have access to gas_meter in our tests. We could add such functionality to our tests to trace gas metering more deeply during testing. This will be implemented as a separate PR.