-
Notifications
You must be signed in to change notification settings - Fork 586
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
Add snapshotter extension #4723
Conversation
hi @damiannolan, now this PR allows edit from maintainers. |
This code looks familiar ;-) |
full review pending on merge of #4292 |
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.
Reviewed everything except the iterate code. Thanks @trinitys7 for getting this work started and @DimitrisJim for pushing it along 🙏
}, | ||
|
||
{ | ||
name: "multiple contract", |
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.
duplicate contract test case? Or are we assuming a node would never be provided such a snapshot?
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.
duplicate contract test would fail with the current test structure on the initial store code call in 08-wasm here: https://github.com/cosmos/ibc-go/blob/feat/wasm-clients/modules/light-clients/08-wasm/keeper/keeper.go#L103-L105
We might be able to work around it tho.
But that being said, all calls to the vm StoreCode
are idempotent so it should just behave as if its a no-op if its ever reached. I think it would be quite hard for a node to obtain a snapshot with duplicate contracts, impossible even?
Hey @trinitys7, apologies that this PR has been waiting in the wings so long! Finally we can pick up the remaining work on this. I will resolve the conflicts and can contribute some commits to pushing this forward. Thank you for getting this started ❤️ |
…ry wasm code from vm
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.
Amazing work! Everything looks great to me, only comment would be to add an assertion that the dest wasm app has no contracts before the snapshots are restored
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.
Dropping my approval on this now, I think its looking pretty good. I'd like to try to improve the tests in future but I think this is good to move forward with for now.
Thank you for collaboration on this PR @trinitys7 @DimitrisJim, and @colin-axner for reviews ❤️ ❤️ ❤️
Description
This PR is to add the snapshotter extension to sync wasm directory :
ibc_08-wasm_client_data
will be subfolder of ~/.simappcloses: #4275
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/
) or specification (x/<module>/spec/
).godoc
comments.Files changed
in the Github PR explorer.Codecov Report
in the comment section below once CI passes.