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.

Sign up for GitHub

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

Jump to bottom

Cosmwasm 0.16 #338

Merged
merged 11 commits into from
Jul 28, 2021
Merged

Cosmwasm 0.16 #338

merged 11 commits into from
Jul 28, 2021

Conversation

uint
Copy link
Contributor

@uint uint commented Jul 20, 2021

Just your run-of-the-mill cosmwasm update.

We should probably hold off with merging this until we have a full release rather than an RC.

Closes #333

@uint uint requested review from webmaster128 and ethanfrey July 20, 2021 15:33
@uint uint self-assigned this Jul 20, 2021
@webmaster128
Copy link
Member

Nice.

We should probably hold off with merging this until we have a full release rather than an RC.

Agreed.

@ethanfrey
Copy link
Member

It's good to have with an rc to test.

I'd love a review on #335 and #337 if you have time

@ethanfrey
Copy link
Member

Can you rebase this and update to cosmwasm 0.16.0-rc4.

That is integrated in wasmd and close enough to final. Happy to have this merged in, so there are no more merge conflicts moving forward

@ethanfrey
Copy link
Member

Besides the missing rebase (see conflicts), I assume this is ready to merge?

Can you rebase and pull it out of draft mode and I will give a final review

ethanfrey
ethanfrey reviewed Jul 26, 2021
View reviewed changes
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Looks good. Two minor comments.

One push for getting this done pre-0.16.0 final is making sure we shape the API with this feedback. Also, some other crates depend on cosmwasm-plus, so if we could make a 0.8.0-rc1, we can get more feedback of use cases when updating those crates.

Usage is the best way to polish off the API.

.circleci/config.yml Outdated Show resolved Hide resolved
contracts/cw20-ics20/src/ibc.rs
@@ -170,8 +178,9 @@ pub fn ibc_packet_receive(
attributes: vec![
attr("action", "receive"),
attr("success", "false"),
attr("error", err),
attr("error", err.to_string()),
Copy link
Member

Choose a reason for hiding this comment

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

I know you cannot implement attr for both impl Into<String> and impl ToString as some structs implement both and you get a compiler error there.

Is there a way to say K: ToString & !Into<String> ? I think I looked into this some months ago and found it worked in unstable rust (and used to optimise std lib Vec for example)

Copy link
Member

Choose a reason for hiding this comment

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

Seems like this is what I am asking for rust-lang/rfcs#1210

Which is only on nightly (specialization and min_specialization)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ayep. It's probably just something we have to live with.

@uint
Copy link
Contributor Author

uint commented Jul 27, 2021

Rebased and migrated to cosmwasm 0.16.0-rc5

@uint
Copy link
Contributor Author

uint commented Jul 27, 2021

I just want to point out we're removing 250 lines of code in this PR! Ha.

@webmaster128
Copy link
Member

🔥

@ethanfrey
Copy link
Member

ethanfrey commented Jul 27, 2021 •
edited
Loading

and the target branch keeps moving....

let's pull this out of draft and rebase again. I will merge when green

ethanfrey
ethanfrey reviewed Jul 27, 2021
View reviewed changes
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

I'd prefer more idiomatic add_attribute usage, even if it does take a bit more time. These are reference contracts

contracts/cw1-subkeys/src/contract.rs Outdated
};
let res = Response::new()
.add_messages(msgs)
.add_attributes(vec![attr("action", "execute"), attr("owner", info.sender)]);
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't that be simpler with 2 add_attribute calls, since they are different V.

Not to be a stickler, but these contracts are like an example for other devs to follow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done almost everywhere now. We barely ever use attr.

contracts/cw1-subkeys/src/contract.rs Outdated
attr("owner", info.sender),
attr("spender", spender),
attr("denomination", amount.denom),
attr("amount", amount.amount),
Copy link
Member

Choose a reason for hiding this comment

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

same here, I'd only use add_attributes when you don't need to use attr

@uint uint marked this pull request as ready for review July 28, 2021 10:16
@uint uint requested a review from ethanfrey July 28, 2021 12:08
ethanfrey
ethanfrey reviewed Jul 28, 2021
View reviewed changes
contracts/cw1-subkeys/src/contract.rs
attr("denomination", amount.denom),
attr("amount", amount.amount),
]);
let res = Response::new()
Copy link
Member

Choose a reason for hiding this comment

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

🚀

ethanfrey
ethanfrey approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Awesome!

contracts/cw20-atomic-swap/src/contract.rs
@@ -683,7 +682,7 @@ mod tests {
)
.unwrap();
assert_eq!(0, res.messages.len());
assert_eq!(attr("action", "create"), res.attributes[0]);
assert_eq!(("action", "create"), res.attributes[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Good fix as well

contracts/cw3-flex-multisig/src/contract.rs
attr("action", "execute"),
attr("sender", SOMEBODY),
attr("proposal_id", proposal_id.to_string()),
[
Copy link
Member

Choose a reason for hiding this comment

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

Wow, this works over arrays as well? Good stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, yeah. Sometimes if you implement a trait like PartialEq for type T, it turns out some generic implementation for [T; 3]or Vec<T> or something kicks in from Rust's std and cool things happen.

@ethanfrey ethanfrey merged commit 6c091b2 into main Jul 28, 2021
@ethanfrey ethanfrey deleted the cosmwasm-0.16 branch July 28, 2021 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethanfrey ethanfrey ethanfrey approved these changes

@webmaster128 webmaster128 Awaiting requested review from webmaster128

Assignees

@uint uint

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to cosmwasm 0.16
3 participants
@uint @webmaster128 @ethanfrey