Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implementation of EIP-1153: Transient Storage using Disk Persistence and Lifecycle Management #1588
base: master
Are you sure you want to change the base?
Implementation of EIP-1153: Transient Storage using Disk Persistence and Lifecycle Management #1588
Changes from 4 commits
801dc0c
ef679de
a98c058
9f3e90d
2ee1d9d
31180a1
56d2458
312299f
757131f
eb894ae
3dfe377
ab808a2
20aefb5
99c1e6b
2b811de
89311f9
af6bb99
661755a
d93167b
0ceb2d8
250d133
3052b34
e614a36
978b49d
a47c21e
e26e519
4feec0f
3d5deba
22963df
41c41de
837a003
3c6d9a7
037939a
dddf19c
646ac96
074e16f
7a99e10
fb8025d
2c8febf
f1ac779
7c6ba37
a005cf7
8eed0c9
133f7b5
1b57940
685fd63
ff09cfa
b96dbe5
b7ac7ba
a43f34a
260f401
a2e4b37
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 interested to know what these errors are and how you end up diagnosing the problem, for my own educational purposes because your commented code looks like it should work.
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.
It's because
self.rt.store()
doesn't implement clone, while&...
does, so rust is helpfully creating a reference for you.Note: the
new
function explicitly requires that the store be clonable.Options are:
impl
level.self.transient_slots.into_store()
to "take" the existing store.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.
@Stebalien I've tried a few different variations of the "taking" option that you are suggesting. I didn't quite get anything to compile however.
Could you share a deeper explanation on the two options to either lift the requirement to the
impl
level (I'm not sure what you mean by this; which requirement? whichimpl
?) and also if you can help me with the second option about taking the ownership of the datastore in order to satisfy the requirements of new_with_config. I'm still learning the ins and outs of this in rust, and would be very grateful. I would definitely not mind a code commit either if it's a simple lift for you. Thanks in advanced!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.
Ah, so, this is going to be a bit of a pain. The issue is that you can't leave
transient_slots
uninitialized so you need something to put there while you construct the newKamt
. E.g., you can haveOption<StateKamt<...>>
and put none there while you replace it.But... doing that will also help facilitate lazy loading (you can leave it as
None
until the user first tries to load a transient slot). So it's not the end of the world.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.
If you're stuck, I'd address the rest of the feedback first. Then I'll see what I can do to fix this part.
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.
Thanks! I'm now returning from the thanksgiving break and will look into everything this week.
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 have learned many things since starting this :) I see that my previous attempt was very much against the RAII ideas that rust enforces. I didn't realize that was the issue. We don't need to destroy and create a new KAMT object but simply need to clear the one we have.
As a placeholder I have implemented a loop over keys and delete implementation of
clear
for now until an efficientclear()
can be used. I opened a PR filecoin-project/ref-fvm#2092 for an efficient implementation ofclear
in KAMT that I hope can be approved and it will be easy for me from there to upgrade kamt in builtin-actors to the future version that supports clear.