Skip to content

Juggernaut/add cw20 support#221

Merged
ethanfrey merged 9 commits intoCosmWasm:masterfrom
juggernaut09:juggernaut/Add-cw20-support
Mar 8, 2021
Merged

Juggernaut/add cw20 support#221
ethanfrey merged 9 commits intoCosmWasm:masterfrom
juggernaut09:juggernaut/Add-cw20-support

Conversation

@juggernaut09
Copy link
Copy Markdown
Contributor

@juggernaut09 juggernaut09 commented Feb 5, 2021

This pull Request is regarding the issue #143

  • used cw20::Denom instead of String in InitMsg::denom and Config::denom
  • Changed handle_bond to accept native of cw20 balance. That means accepting sender: HumanAddr, amount: cw20::Balance instead of info: MessageInfo.
  • handle_bond is now accepting incoming tokens using a switch on config.denom. Kept the current logic for native case.
  • Update handle_claim to release either native token (as now) or cw20-token (used unimplemented!() now, implemented using a match case.
  • @ethanfrey please review these changes.

…:denom

-> Change handle_bond to accept native of cw20 balance. That means accepting sender: HumanAddr, amount: cw20::Balance instead of info: MessageInfo. This conversion should be done in the handle switch just like in cw20-escrow.
-> handle_bond should enforce incoming tokens using a switch on config.denom, inspired by cw20-escrow checks. You will want to keep the current logic for the Balance::Native case (or simplify it to use cw0::must_pay if you know how to extend the custom error type.
->  Update handle_claim to release either native token (as now) or cw20-token (this can be unimplemented!() now, but have the branch based on the cw20::Balance variant)
@CLAassistant
Copy link
Copy Markdown

CLAassistant commented Feb 5, 2021

CLA assistant check
All committers have signed the CLA.

Copy link
Copy Markdown
Contributor

@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.

Nice start. The wiring up of the objects looks good, but I think you made it much too complex for checking the input denom.

Hope these suggestions of simpler data structs help.

@juggernaut09
Copy link
Copy Markdown
Contributor Author

Thank you for reviewing. I will come up with the optimized code in the next commit.

@ethanfrey
Copy link
Copy Markdown
Contributor

I got a notification, but the last commit was 12 days ago.
Happy to review if there is some new code pushed.

Copy link
Copy Markdown
Contributor

@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.

Much nicer.

We will be doing a big overhaul to v0.14 cosmwasm soon.
Can you address the few PR comments I added and we can merge this in (half done), so it gets updated along with the rest of the contracts?

Removed unnecessary imports and tweaked coding conventions.
@juggernaut09 juggernaut09 marked this pull request as ready for review March 4, 2021 16:18
@juggernaut09 juggernaut09 requested a review from ethanfrey March 4, 2021 16:24
Copy link
Copy Markdown
Contributor

@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.

Thanks for the update. The CosmWasm v0.14.0 upgrade happened faster than expected.

Can you rebase and resolve conflicts. Then I am happy to merge. You can look at the migration PR to see some changes needed (in particular, handle -> execute as entry point) and returning a plain Response object (same fields, just a rename)

@juggernaut09 juggernaut09 requested a review from ethanfrey March 8, 2021 07:38
Copy link
Copy Markdown
Contributor

@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.

Thanks for fixing this up.
I'm going to merge this in now. I will be doing some refactoring in another PR, then happy for you to complete the support.

@ethanfrey ethanfrey merged commit f38e805 into CosmWasm:master Mar 8, 2021
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