-
Notifications
You must be signed in to change notification settings - Fork 180
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
v0.17 upgrade logic #112
v0.17 upgrade logic #112
Conversation
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.
LGTM
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.
Nice improvement. Thanks!
@larry0x sorry for being so slow to review. should be able to rebase on main and merge this. |
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.
Seems good.
Just little comment about the cw2 library, I think version assertion is something custom to each contract (why cw2 didn't create a function for it?), would defo suggest just copy paste it as a helper.
# FIXME: change back to the "official" crates.io release after this PR is merged: | ||
# https://github.com/CosmWasm/cw-plus/pull/858 | ||
cw2 = { git = "https://github.com/mars-protocol/cw-plus", rev = "1a3a944" } |
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.
Seems to be an overkill just to get the assert_contract_version
helper: https://github.com/CosmWasm/cw-plus/pull/858/files#diff-a35715fa1fde56c42db0d8f2845ebc6a571a801699c0705f0080237820d96ee6R59-R83
Would suggest just to just put it in this contract until we have an "official" version of it.
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.
@Art3miX My PR to cw2 referenced in the comment has been merged. Now we can use the main branch of cw2 instead of mars-protocol fork. Imo this is more elegant than including extra code in this repo which we need to remove later.
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.
Oh ok, so this can be changed to the official cw2 now?
I just didn't like the using some fork for a helper function just to revert back later to the previous package.
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.
We can change it to the main branch (use the latest commit) of the official cw-plus repo
upgrades/v0_16.rs
toupgrades/v0_17.rs
(I think the filename should be the version being migrated to, not the version being migrated from)cw2::assert_contract_version
; have to use our fork of cw2 before our PR is merged)