Skip to content
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

Test for multiple writes/reads to the same storage slot #2874

Merged

Conversation

beeguy74
Copy link
Contributor

@beeguy74 beeguy74 commented Jan 24, 2025

Closes #1559

Introduced changes

I modified locally :
storage_view field to map only StorageKey to felt

Screenshot of modified `crates/runtime/src/starknet/state.rs` state modified locally

And created two test cases which ensure that writes to the storage across multiple contracts with same ClassHash and storage key is not possible inside crates/cheatnet/tests/cheatcodes/multiple_writes_same_storage.rs:

  • same_storage_access_call_contract - Verifies that the two deployed contracts with same class_hash have different addresses and demonstrates modification and access to storage variable balance with call_contract method
  • same_storage_access_store - does same routine but uses custom implementations for storage operations store and load instead of call_contract to access to storage

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

@franciszekjob
Copy link
Collaborator

Hi @beeguy74 👋,
let me if you're ready for review or need any assistance 😄

@beeguy74 beeguy74 marked this pull request as ready for review January 27, 2025 16:39
@beeguy74
Copy link
Contributor Author

beeguy74 commented Jan 27, 2025

hi @franciszekjob ! I think i finished here. But I'm not sure it's necessary to change the documentation and changelog. Thank you in advance!

Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

Overall looks good 😄 , please see my nits.

Copy link
Member

@kkawula kkawula left a comment

Choose a reason for hiding this comment

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

Please, show that these tests fail in the modified state

@beeguy74
Copy link
Contributor Author

beeguy74 commented Jan 28, 2025

Hi @kkawula! As far the tests pass in both states: modified or not
image

@beeguy74
Copy link
Contributor Author

So, guys. You want me to achieve a test failing?

@franciszekjob
Copy link
Collaborator

Hi @beeguy74 , let me check why these tests don't fail. Will update you.

@beeguy74
Copy link
Contributor Author

It would help me a lot if you gave me another assignment, sir, while I'm blocked here. For example, I applied for another test issue. Can you assign me please?

@franciszekjob
Copy link
Collaborator

@beeguy74 in fact, the repro steps in issue description were a bit misleading. The tests should pass in both cases (so it's ok as it is now), apologies for little inconvenience.

Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

Please fix formatting 🙏

@beeguy74 beeguy74 force-pushed the test/multiple-writes-same-storage branch from 5bca80a to 826ed59 Compare January 29, 2025 12:44
@beeguy74
Copy link
Contributor Author

formatted
image

@franciszekjob franciszekjob added this pull request to the merge queue Jan 31, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 31, 2025
@beeguy74
Copy link
Contributor Author

beeguy74 commented Feb 3, 2025

Can you merge it, please, to finish my ODboost contribution 🙏

@franciszekjob franciszekjob added this pull request to the merge queue Feb 3, 2025
github-merge-queue bot pushed a commit that referenced this pull request Feb 3, 2025
<!-- Reference any GitHub issues resolved by this PR -->

Closes #1559

## Introduced changes

<!-- A brief description of the changes -->
I  modified locally :
`storage_view` field  to map only StorageKey to felt

<details>
<summary>Screenshot of modified
`crates/runtime/src/starknet/state.rs`</summary>
<img
src="https://github.com/user-attachments/assets/6bcb98e5-9ccd-4064-a5af-56ea300a9c7d"
alt="state modified locally" </img>
</details>

And created two test cases which ensure that writes to the storage
across multiple contracts with same ClassHash and storage key is not
possible inside
[`crates/cheatnet/tests/cheatcodes/multiple_writes_same_storage.rs`](diffhunk://#diff-e17b72d677be53df13b4896cb6770dc9f012711c5f02ac3e6dd7f7238d8ef32fR1-R46):
* `same_storage_access_call_contract` - Verifies that the two deployed
contracts with same class_hash have different addresses and demonstrates
modification and access to storage variable `balance` with
`call_contract` method
* `same_storage_access_store` - does same routine but uses custom
implementations for storage operations `store` and `load` instead of
`call_contract` to access to storage
## Checklist

<!-- Make sure all of these are complete -->

- [x] Linked relevant issue
- [ ] Updated relevant documentation
- [x] Added relevant tests
- [x] Performed self-review of the code
- [ ] Added changes to `CHANGELOG.md`

---------

Co-authored-by: Franciszek Job <[email protected]>
@franciszekjob
Copy link
Collaborator

franciszekjob commented Feb 3, 2025

Can you merge it, please, to finish my ODboost contribution 🙏

Yes, added to merge queue :) Thank you for contribution 😄

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 3, 2025
@cptartur cptartur added this pull request to the merge queue Feb 3, 2025
Merged via the queue into foundry-rs:master with commit 2cdbf6d Feb 3, 2025
38 checks passed
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.

Test for multiple writes/reads to the same storage slot
4 participants