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

implement demo cw1155 contract #251

Merged
merged 3 commits into from
Apr 8, 2021
Merged

Conversation

yihuang
Copy link
Contributor

@yihuang yihuang commented Mar 23, 2021

there are several more unit tests to be done, the overall implementation is good for review now.
There are two FIXME currently:

  • code, we should add overflow error branch in StdError.
  • code, the list returned by query is out of order, I'm not sure if it's an issue of the test storage itself?

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 just got back from vacation and have lots to do before I can take a deeper look, but I did add comments on the 2 FIXMEs. These are subtle points about how cosmwasm-std is designed and actually intended.

}
}

fn checked_add(a: Uint128, b: Uint128) -> StdResult<Uint128> {
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. We define sub for Uint128 to possibly return an error if it underflows (not enough balance). We don't do this for adding as we assumed this would never happen.

We do however, test that this extremely rare condition causes a panic, which is enough to keep the contract secure. It won't provide a pretty error message to the user, but will block the attack vector. I would just use the normal Uint128 + Uint128 approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted back to simple + operator.
Opened issue in cosmwasm-std for the overflow handling: CosmWasm/cosmwasm#850

.unwrap();
}

// // FIXME test failed, query result out of order
Copy link
Member

Choose a reason for hiding this comment

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

They are not "out of order", it is that the keys are stored in CanonicalAddr, which you cannot assume is the same ordering as HumanAddr. For bech32 the order sometimes is the same, sometimes different. For tests, we semi-randomize the mapping to ensure you do not depend on it.

If you hardcode some addresses, just figure out via a few test runs what the ordering is as CanonicalAddr and then assert that one. It will be stable, but it makes you be sure if you are comparing canonical or human addresses.

Discussion here: CosmWasm/cosmwasm#552

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for clarification, FIXME is resolved now.

@yihuang
Copy link
Contributor Author

yihuang commented Mar 29, 2021

I just got back from vacation and have lots to do before I can take a deeper look, but I did add comments on the 2 FIXMEs. These are subtle points about how cosmwasm-std is designed and actually intended.

Thanks, two FIXMEs are resolved now.

@yihuang yihuang force-pushed the cw1155-impl branch 2 times, most recently from a1b7cb0 to 72ea5e8 Compare April 1, 2021 16:08
@yihuang
Copy link
Contributor Author

yihuang commented Apr 1, 2021

Added unit tests to expect panic when minting token amount overflow.

@yihuang yihuang requested a review from ethanfrey April 1, 2021 16:13
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.

Very nice work. And sorry for the delay in reviewing, between vacation and Easter and end-of-the-month, my work weeks have been full.

I made some stylistic comments on a few places, but quite impressed with the quality of the code. And I learned a few techniques I'd like to emulate for other contracts. Especially the event system and inlining those query functions.

If you want to take a pass and address some of the minor comments, I am happy to merge it. Very nice addition to cosmwasm-plus

contracts/cw1155-base/Cargo.toml Outdated Show resolved Hide resolved
contracts/cw1155-base/src/lib.rs Outdated Show resolved Hide resolved
contracts/cw1155-base/src/state.rs Outdated Show resolved Hide resolved
contracts/cw1155-base/src/state.rs Show resolved Hide resolved
packages/cw1155/src/event.rs Show resolved Hide resolved
contracts/cw1155-base/src/contract.rs Show resolved Hide resolved
contracts/cw1155-base/src/contract.rs Show resolved Hide resolved
contracts/cw1155-base/src/contract.rs Show resolved Hide resolved
contracts/cw1155-base/src/contract.rs Show resolved Hide resolved
.unwrap(),
Response {
attributes: vec![
attr("action", "transfer"),
Copy link
Member

Choose a reason for hiding this comment

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

This is interesting. I am not sure how this ends up when it comes out through tendermint (I think it does some "collapsing" of events).

It totally makes sense. I wonder if you had a wish on a cleaner way to encode these or pass them through to the client? Although I guess just as it is is perfect for searching.

Copy link
Contributor Author

@yihuang yihuang Apr 8, 2021

Choose a reason for hiding this comment

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

I'm not sure how it's going to be indexed by tendermint though, need to look into details in tendermint I guess.

EDIT:
Yeah, i guess it would be fine, since tendermint will index attributes independently?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, they should all match

test approval expires
- Use entry_point macro
- bump dependencies to alpha3
- Remove unused struct
- Remove guard helper
- Other misc changes
rsp.messages = if let Some(msg) = msg {
vec![Cw1155ReceiveMsg {
if let Some(msg) = msg {
rsp.messages = vec![Cw1155ReceiveMsg {
Copy link
Member

Choose a reason for hiding this comment

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

This could also be rsp.add_message(Cw1155ReceiveMsg { .. }.into_cosmos_msg(to)?);

But I don't think that is any clearer in this case.

// insert if not exist
let key = TOKENS.key(&token_id);
if deps.storage.get(&key).is_none() {
// insert an empty entry so token enumeration can find it
Copy link
Member

Choose a reason for hiding this comment

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

thanks for the comment

@@ -42,6 +44,7 @@ pub struct ExecuteEnv<'a> {
info: MessageInfo,
}

#[cfg_attr(not(feature = "library"), entry_point)]
Copy link
Member

Choose a reason for hiding this comment

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

Nice update

if !check_can_approve(deps.as_ref(), &env, &from, &info.sender)? {
return Err(ContractError::Unauthorized {});
}
guard_can_approve(deps.as_ref(), &env, &from, &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.

Yeah, this is a nicer way to inline the common check.

.unwrap(),
Response {
attributes: vec![
attr("action", "transfer"),
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, they should all match

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.

Good stuff.

@ethanfrey
Copy link
Member

Created #261 inspired by your comments

@ethanfrey ethanfrey merged commit 7244314 into CosmWasm:master Apr 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.

2 participants