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

Cw20 atomic swaps #72

Merged
merged 15 commits into from
Sep 8, 2020
Merged

Cw20 atomic swaps #72

merged 15 commits into from
Sep 8, 2020

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Sep 3, 2020

Cw20 swap functionality.

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.

I think some of the helpers to combine native and cw20 coins can be extended to make this even cleaner. But that is really for #73 - happy to see how you combined them already and I think we could do something similar with cw20-escrow as well.

I want to go through the test cases again to make sure it is all covered, but in general looks quite good.

contracts/cw20-atomic-swap/Cargo.toml Show resolved Hide resolved
HandleMsg::Create(msg) => try_create(deps, env, msg),
HandleMsg::Create(msg) => try_create(
deps,
env.clone(),
Copy link
Member

Choose a reason for hiding this comment

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

Why clone? We never use it elsewhere

Copy link
Contributor Author

@maurolacy maurolacy Sep 7, 2020

Choose a reason for hiding this comment

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

It's needed because of the use of env.message.sent_funds, just below. I think it's because Coin (or the whole Env) doesn't currently implement the Copy trait.

I improved it a bit by cloning only the needed struct member.

I'll create an issue to implement the Copytrait for Coin/ Vec<Coin>.

Copy link
Member

Choose a reason for hiding this comment

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

Coin contains strings, and cannot be Copy, only Clone.
I missed the other usage below... my bad. It was fine as it was, I just needed ☕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem. I'll just close the issue I wrote regarding Copy coins then.

contracts/cw20-atomic-swap/src/contract.rs Outdated Show resolved Hide resolved
}
}

pub fn try_receive<S: Storage, A: Api, Q: Querier>(
Copy link
Member

Choose a reason for hiding this comment

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

Nice

contracts/cw20-atomic-swap/src/contract.rs Outdated Show resolved Hide resolved
contracts/cw20-atomic-swap/src/contract.rs Show resolved Hide resolved
contracts/cw20-atomic-swap/src/contract.rs Show resolved Hide resolved
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 and thanks for the code cleanup

amount: cw20_coin.amount,
msg: Some(to_binary(&HandleMsg::Create(create)).unwrap()),
};
let token_contract = cw20_coin.address; // TODO: Confirm
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is correct

recipient: cw20_rcpt,
amount: cw20_coin.amount,
};
assert_eq!(
Copy link
Member

Choose a reason for hiding this comment

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

Yup. I wonder if some of these asserts could be made easier with a few helpers, but not sure

let env = mock_env("other_somebody", &[]);

// First, let's obtain the preimage from the logs of the release() transaction on Y
let preimage_log = &res.log[2];
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 merged commit a35cfbe into master Sep 8, 2020
@ethanfrey ethanfrey deleted the cw20-atomic-swaps branch September 8, 2020 20:15
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