Skip to content

impl original_storage in SubstrateStackState#871

Merged
sorpaas merged 6 commits intopolkadot-evm:masterfrom
moonbeam-foundation:jeremy-handle-original-storage
Oct 6, 2022
Merged

impl original_storage in SubstrateStackState#871
sorpaas merged 6 commits intopolkadot-evm:masterfrom
moonbeam-foundation:jeremy-handle-original-storage

Conversation

@nanocryk
Copy link
Copy Markdown
Contributor

@nanocryk nanocryk commented Oct 4, 2022

Implement original_storage which previously returned None everytime.

@nanocryk nanocryk requested a review from sorpaas as a code owner October 4, 2022 13:57
@@ -629,6 +643,15 @@ where
}

fn set_storage(&mut self, address: H160, index: H256, value: H256) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a check beforehand that:

  • If self.original_storage does not contain the key,
  • and the to-set-value is the same as the current value.

Skip inserting into the original_storage.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If self.original_storage does not contain the key,
That's what if let Vacant(e) = self.original_storage.entry(...) does, which was recommanded by clippy instead of contains then insert.
and the to-set-value is the same as the current value.
Yeah, can add this check to not insert for same value as original.

// in the transaction.
use sp_std::collections::btree_map::Entry::Vacant;
if let Vacant(e) = self.original_storage.entry((address, index)) {
let original = <AccountStorages<T>>::get(address, index);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Use self.storage here to keep it consistent.

Suggested change
let original = <AccountStorages<T>>::get(address, index);
let original = self.storage(address, index);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cannot use self.storage as we're already borrowing self with self.original_storage.entry. Seems better to keep <AccountStorages<T>>::get than fetching the value every time even when the entry is not vacant.

Copy link
Copy Markdown
Member

@sorpaas sorpaas left a comment

Choose a reason for hiding this comment

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

Grumble otherwise LGTM.

@nanocryk nanocryk force-pushed the jeremy-handle-original-storage branch from 408fbe3 to 018eb95 Compare October 5, 2022 12:09
@sorpaas sorpaas merged commit 72aeaf6 into polkadot-evm:master Oct 6, 2022
notlesh pushed a commit to moonbeam-foundation/frontier that referenced this pull request Oct 6, 2022
* handle original_storage

* only cache original_storage in set_storage

* test

* clippy

* remove RefCell

* don't write to cache if = to original
abhijeetbhagat pushed a commit to web3labs/frontier that referenced this pull request Jan 11, 2023
* handle original_storage

* only cache original_storage in set_storage

* test

* clippy

* remove RefCell

* don't write to cache if = to original
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