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

Finish v0.11 migration #111

Merged
merged 23 commits into from
Oct 10, 2020
Merged

Conversation

ethanfrey
Copy link
Member

This finishes up the great work started in @maurolacy 's branch and makes a final cosmwasm-plus PR for the upgrades

@maurolacy
Copy link
Contributor

maurolacy commented Oct 9, 2020

Ey, thanks for finishing this! I can review it today / tomorrow.

@@ -902,7 +911,7 @@ mod tests {
);

// run the migration
let env = mock_env(HumanAddr::from("admin"), &[]);
let (env, _info) = mock_env_info(HumanAddr::from("admin"), &[]);
Copy link
Member Author

Choose a reason for hiding this comment

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

don't we need to add info to migrate so it compiles in wasm mode?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you are right. I did it locally but I suppose you are working on it don't want to tip toes.

@ethanfrey ethanfrey requested a review from maurolacy October 9, 2020 18:52
@ethanfrey
Copy link
Member Author

@maurolacy can you give a quick review on these changes before merging them into your branch (you should be able to do that). Then I can merge your branch into master. And we have 0.11 support!

I guess I wait til Monday to cut the v0.3.0 release

.collect();

Ok(ProposalListResponse { proposals: props? })
}

fn map_proposal(item: StdResult<(Vec<u8>, Proposal)>) -> StdResult<ProposalResponse> {
fn map_proposal(
block: &BlockInfo,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

Duration::Time(duration) => mock_env_time(VOTER4, env.block.time + duration + 1),
Duration::Height(duration) => mock_env_height(VOTER4, env.block.height + duration + 1),
Duration::Time(duration) => mock_env_time(duration + 1),
Duration::Height(duration) => mock_env_height(duration + 1),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice.

Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Quick review, LGTM.

contracts/cw3-fixed-multisig/src/state.rs Show resolved Hide resolved
contracts/cw721-base/src/contract.rs Outdated Show resolved Hide resolved
})
.filter_map(|item| match item {
Ok((k, expires)) => {
// we remove all expired operators from the result
Copy link
Contributor

Choose a reason for hiding this comment

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

Sure about this? I'm not sure, but listing expired operators can be useful for debugging / accountability issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I can keep them, but then add another flag to the response "is_expired: bool". Is it useful info on the client, without forcing calculations there

Copy link
Contributor

@maurolacy maurolacy Oct 9, 2020

Choose a reason for hiding this comment

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

Good idea.

Another option would be to add a boolean flag to the query, i. e. include_expired or so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be addressed here: 09b7b31

Copy link
Member Author

Choose a reason for hiding this comment

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

Another option would be to add a boolean flag to the query, i. e. include_expired or so.

Just read this. I actually prefer this idea

Copy link
Member Author

Choose a reason for hiding this comment

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

I will undo my change and redo it the other way. Let me know which commit you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I too think the query option is better.

Copy link
Member Author

Choose a reason for hiding this comment

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

Look at 0525a34

info.approvals
.iter()
.filter(|apr| !apr.expires.is_expired(block))
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I didn't update this part... Just the above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed that in 471c86f

@ethanfrey ethanfrey force-pushed the finish-v0.11-migration branch from 09b7b31 to 0525a34 Compare October 9, 2020 20:59
@ethanfrey ethanfrey merged commit 74074a0 into migrate-to-v0.11.0-alpha4 Oct 10, 2020
@ethanfrey ethanfrey deleted the finish-v0.11-migration branch October 10, 2020 00:24
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.

3 participants