Refactor fuel-core to use version of StorageRead::read with offset #2438
Conversation
acerone85
requested review from
AurelienFT,
xgreenx,
Dentosal and
MitchTurner
as code owners
|
I find it suspicious that I never had to hardcode the offset value to |
acerone85
changed the title
| @@ -546,7 +546,8 @@ impl TryFrom<VmBench> for VmBenchPrepared { | |||
| } | |||
| let storage_diff = vm.storage_diff(); | |||
| let mut vm = vm.remove_recording(); | |||
| let mut diff = start_vm.diff(&vm); | |||
| // TODO: Check if this is the intended use of rollback_to. | |||
There was a problem hiding this comment.
@xgreenx please check that this is how we intend to use the new rollback_to funciton, in which case we can remove the TODO
| @@ -4084,6 +4084,7 @@ dependencies = [ | |||
| "sha3", | |||
| "static_assertions", | |||
| "strum 0.24.1", | |||
| "substrate-bn", | |||
There was a problem hiding this comment.
Check, do we want this?
There was a problem hiding this comment.
i think it is a side-effect of bumping fuel-vm. the ecc operations under the hood use substrate-bn
| dependencies = [ | ||
| "spin 0.9.8", | ||
| ] |
There was a problem hiding this comment.
same: do we need this?
There was a problem hiding this comment.
it comes from lazy static from the substra-bn
| .map_err(|e| DatabaseError::Other(anyhow::anyhow!(e)))?; | ||
| StorageResult::Ok(read) | ||
| if let Some(read) = value.len().checked_sub(offset) { | ||
| std::io::Write::write_all(&mut buf, value[offset..].as_ref()) |
There was a problem hiding this comment.
this can panic if offset is out of bounds maybe we should use .get ?
There was a problem hiding this comment.
If offset is out of bounds, then value.len().checked_sub(offset) should return None and this code won't be executed.
But I agree that it is not immediate from the code, so maybe there's a better way to write this? 🤔
There was a problem hiding this comment.
yeah i missed it a comment is fine
Linked Issues/PRs
Related to FuelLabs/fuel-vm#681
Description
In order to load contracts from a given offset efficiently, we have changed the interface of
StorageRead::readto accept a n offset in input (see FuelLabs/fuel-vm#863). This PR refactors the fuel-core repo to make use of the new StorageRead trait.Do not merge
we should first wait for the changes to
StorageReadto be released infuel-vm, after which we can update the dependencies in this PRChecklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]