-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add additionally functionality to contracts storage interface #10497
Conversation
/benchmark runtime pallet pallet_contracts |
Benchmark Runtime Pallet for branch "at-contract-storage" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
260e75b
to
96dbc43
Compare
/benchmark runtime pallet pallet_contracts |
Benchmark Runtime Pallet for branch "at-contract-storage" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
d9ac33f
to
9de0206
Compare
/benchmark runtime pallet pallet_contracts |
Benchmark Runtime Pallet for branch "at-contract-storage" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
982bb99
to
f5b2cdd
Compare
/benchmark runtime pallet pallet_contracts |
Benchmark Runtime Pallet for branch "at-contract-storage" with command cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_contracts --extrinsic="*" --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/contracts/src/weights.rs --template=./.maintain/frame-weight-template.hbs Results
|
9adb443
to
4ea2fe5
Compare
#[cfg(feature = "unstable-interface")] | ||
fn set_storage_works() { | ||
const CODE: &str = r#" | ||
(module |
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.
Out of curiosity, do you hand-craft this stuff?
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.
Yes all test fixtures are written by hand.
frame/contracts/src/wasm/runtime.rs
Outdated
@@ -632,6 +648,46 @@ where | |||
} | |||
} | |||
|
|||
/// Extracts the size of the overwritten value or `u32::MAX`. |
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'd be explicit about what case returns u32::MAX
here
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.
Makes sense. I changed the doc.
self.read_sandbox_memory_into_buf(key_ptr, &mut key)?; | ||
let value = Some(self.read_sandbox_memory(value_ptr, value_len)?); | ||
self.ext | ||
.set_storage(key, value, false) |
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.
How come we default to take = false
here?
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.
Because we are in seal_set_storage
which should just overwrite the value without reading (taking) the existing value. This is what seal_take_storage
is for.
@@ -2285,7 +2446,7 @@ benchmarks! { | |||
// configured `Schedule` during benchmark development. | |||
// It can be outputed using the following command: | |||
// cargo run --manifest-path=bin/node/cli/Cargo.toml --release \ | |||
// --features runtime-benchmarks -- benchmark --dev --execution=native \ | |||
// --features runtime-benchmarks -- benchmark --extra --dev --execution=native \ |
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.
What's the extra
feature used for?
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.
This was a change introduced to the benchmarking CLI. In order to run benchmarks attributed with#[extra]
you need to supply this flag.
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.
Haven't reviewed it in depth, yet but the new API fixes a lot of issues we have in ink! so I am looking forward a lot to this! Tests look fine. Code looks clean.
bot merge |
…tech#10497) * Add new versions for storage access host functions * Improve docs
…tech#10497) * Add new versions for storage access host functions * Improve docs
seal_contains_storage
which only checks for existence without reading the value. ( fixes Enablecontracts::seal_get_storage
to skip reading the actual value #10490 )seal_take_storage
behaves similarly toseal_get_storage
but also deletes the value.seal_set_storage
returns the size of the overwritten value oru32::MAX
.seal_clear_storage
returns the size of the overwritten value oru32::MAX
.Usually, I try to have a minimal API surface by not exposing functions that can be composed by calling multiple other functions. However, we want contracts that only need a simple mapping to be able to implement that without multiple calls into the contracts pallet.
Note
The weights make no sense because they assume that reading the length or checking for existence is possible in constant time with regard to the item size which is not the case. The benchmarks for
seal_clear_storage
andseal_contains_storage
are using the maximum allowed item size and incur very high weights by doing so for this reason. This will be fixed in a PR that implements #10508.