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

Cache not written to storage after removal of selectors from the last slot #3

Closed
wants to merge 4 commits into from

Conversation

grebnetiew
Copy link

@grebnetiew grebnetiew commented Apr 29, 2020

On removal of a selector from the last slot, the entire operation happens in the cache (named slot.selectorSlot). There is a flag to mark it 'dirty', called slot.newSlot, which is used correctly when adding new selectors. However it is not changed upon removal, and if you only remove a slot, the cache is not written back. This causes the storage to still contain the removed selector.

Final slot before removal:

[ sel2 sel3 sel4 sel5 ]

Final slot after removal of sel3, cache:

[ sel2 sel5 sel4 ]

Final slot in storage after removal:

[ sel2 sel3 sel4 ]

By setting the dirty flag to true, the cache is written back correctly.

The PR adds a test checking if the bug exists.

Erik Weitenberg added 4 commits April 29, 2020 10:07
On removal of a selector from the last slot, the entire operation happens
in the cache (named slot.selectorSlot). There is a flag to mark it 'dirty',
called slot.newSlot, which is used correctly when adding new selectors.
However it is not changed upon removal, and if you only remove a slot,
the cache is not written back. This causes the storage to still contain
the removed selector.

Final slot before removal:
[ sel2 sel3 sel4 sel5 ]
Final slot after removal of sel3, cache:
[ sel2 sel5 sel4 ]
Final slot in storage after removal:
[ sel2 sel3 sel4 ]

By setting the dirty flag to true, the cache is written back correctly.
@mudgen
Copy link
Owner

mudgen commented Aug 11, 2020

I didn't see this until now. Thanks a lot for this! I fixed this bug. The testing switched to Truffle so I modified your test to work with Truffle and added your test to the project. Thanks again.

@mudgen mudgen closed this Aug 11, 2020
@mudgen
Copy link
Owner

mudgen commented Aug 13, 2020

@grebnetiew If you want to contribute more to this project, there's this issue: https://github.com/mudgen/Diamond/issues/7

mudgen pushed a commit that referenced this pull request Sep 1, 2020
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