Implement cw20-merkle-airdrop #364

orkunkl · 2021-07-29T11:41:31Z

This contract implements merkle airdrops for efficient token distribution.

hashedone

Just left some Rust-idioms related comments, coupled with very general stuff (like files which I think shouldn't be there). I didn't review helper code at all as I have literally no knowledge of Node ecosystem (neither JS nor TS), so someone should probably take a look at this. Later I would also do a second round with attempt to understand the contract logic and verify if I can spot any bugs.

contracts/cw20-merkle-airdrop/.editorconfig

contracts/cw20-merkle-airdrop/helpers/.editorconfig

contracts/cw20-merkle-airdrop/helpers/.eslintignore

contracts/cw20-merkle-airdrop/helpers/.eslintrc

contracts/cw20-merkle-airdrop/rustfmt.toml

hashedone · 2021-07-29T12:48:17Z

contracts/cw20-merkle-airdrop/src/contract.rs

+
+    // validate owner change
+    let new_owner = owner
+        .ok_or(ContractError::InvalidInput {})


I don't understand how owner can be Optional in this message, if setting it to None always result in an error? Wouldn't it be cleaner to just make owner a String?

Also this can be done easier to read like:

let new_owner = owner.ok_or(ContractError::InvalidInput {})?; let new_owner = deps.api.addr_validate(&new_owner)?;

I see you changed it to what I proposed, but I still don't get - why owner is optional in the message in the fist place? For me it looks like if it is not set it is an error, so why not to make it required?

I see you changed it to what I proposed, but I still don't get - why owner is optional in the message in the fist place? For me it looks like if it is not set it is an error, so why not to make it required?

I agree, the Msg types should capture requirements, and will enforce them in parsing without you writing any code. if we require a number to be present AND between 1 and 100, well, I would just use serde to enforce it is a value u64, and then add business logic for 1-100. But the types should be captured in the Msg classes and the rustdoc there

contracts/cw20-merkle-airdrop/src/contract.rs

orkunkl · 2021-07-29T13:52:48Z

@hashedone here is an explanation on how merkle airdrop works for better context Medium Merkle Airdrop: the Basics

hashedone · 2021-07-30T07:57:11Z

@ethanfrey please take additional look at contract details itself as we spoke yesterday. I do my best here, but I am not yet confident enough about smart contract logic to approve/merge it even if it is ok for me.

This reverts commit 6c8c4c7.

contracts/cw20-merkle-airdrop/src/state.rs

contracts/cw20-merkle-airdrop/src/contract.rs

contracts/cw20-merkle-airdrop/src/msg.rs

ethanfrey

Was doing a review untl a big diff came in. Will continue later

contracts/cw20-merkle-airdrop/NOTICE

contracts/cw20-merkle-airdrop/README.md

ethanfrey · 2021-07-30T13:17:10Z

contracts/cw20-merkle-airdrop/README.md

+
+This contract works with multiple airdrop rounds, meaning you can execute several airdrops using same instance.
+
+Uses **SHA3 Keccak 256** for merkle root tree construction.


Why this choice?
To be compatible with Ethereum obviously, but do we need to?
Are there frontend tools we can reuse by doing so?

I would use sha2-256 or sha3-256 (non-keccak) or maybe blake2. Keccak is only used in Ethereum, the rest are more standard libs.

I pulled this code base from anchor protocol(Apache 2.0) licence.
I believe developers come from solidity background.
I thought of changing it but left it for your decision. I will apply it!

Let's leave it.

contracts/cw20-merkle-airdrop/README.md

contracts/cw20-merkle-airdrop/src/msg.rs

ethanfrey · 2021-07-30T13:39:41Z

contracts/cw20-merkle-airdrop/src/contract.rs

+        return Err(ContractError::Unauthorized {});
+    }
+
+    // check merkle root length


This is a proper decode, but does it assert the length?
I would just manually assert the input is 64 chars and assert it explicitly

let mut root_buf: [u8; 32] = [0; 32];

I thought 32 runs assertion

contracts/cw20-merkle-airdrop/src/contract.rs

ethanfrey · 2021-07-30T13:45:57Z

Please remove node_modules (and include that in .gitignore) and use git rebase -i to splice out that commit from the git history (splitting it into 2 parts first if you want to save something).

node_modules is HUUUGGGEEE and should never be committed anywhere (there is already discussion on yarn.lock)

ethanfrey

Finished looking through it.
The actual hashing and verify logic should be in a separate function and heavily tested for compatibility with TS. Otherwise, looks good.

ethanfrey · 2021-07-30T14:27:44Z

contracts/cw20-merkle-airdrop/src/contract.rs

+    let merkle_root = MERKLE_ROOT.load(deps.storage, stage.into())?;
+
+    let user_input = format!("{}{}", info.sender, amount);
+    let hash = sha3::Keccak256::digest(user_input.as_bytes())


These lines (142/143-157) should be pulled out into a separate function to heavily unit test it and compare with TS test vectors. They are simple function on basic data structures and easier to heavily test than the execute methods

contracts/cw20-merkle-airdrop/src/contract.rs

ethanfrey · 2021-07-30T14:35:43Z

contracts/cw20-merkle-airdrop/src/contract.rs

+        };
+        let _res = execute(deps.as_mut(), env, info, msg).unwrap();
+
+        let msg = ExecuteMsg::Claim {


It would be good to document where these come from. Or even better, have a script that generates many of these examples, so we can use testvectors.

You can store as json files and include in the tests with include_bytes!()

ethanfrey · 2021-07-30T14:37:19Z

contracts/cw20-merkle-airdrop/src/state.rs

+pub const CONFIG_KEY: &str = "config";
+pub const CONFIG: Item<Config> = Item::new(CONFIG_KEY);
+
+pub const STAGE_KEY: &str = "stage";


I avoid supporting raw queries, except in a few special cases.
cw4 interface is one and has a very well define raw query support for ONE use case - is_member.

But if you prefer them, your style is not wrong, just a bit more verbose. And yes, does allow raw queries.

ethanfrey

Good steps

ethanfrey · 2021-07-30T15:41:22Z

contracts/cw20-merkle-airdrop/Cargo.toml

-cw20 = { path = "../../packages/cw20", version = "0.7.0" }
-cosmwasm-std = { version = "0.15.0", features = ["iterator"] }
-cw-storage-plus = { path = "../../packages/storage-plus", version = "0.7.0", features = ["iterator"] }
+cw0 = { path = "../../packages/cw0", version = "0.8.0-rc1" }


ethanfrey

I will merge this in, so it doesn't rot as we prepare a release.

The Rust contract code is fine. However, I really would like to see some unit tests in TS land showing compatibility with the Rust hashing algorithm. That can be a follow up PR (and maybe by a JS dev if you can convince one to do so)

ethanfrey · 2021-08-03T20:38:07Z

contracts/cw20-merkle-airdrop/README.md

+
+This contract works with multiple airdrop rounds, meaning you can execute several airdrops using same instance.
+
+Uses **SHA3 Keccak 256** for merkle root tree construction.


Let's leave it.

orkunkl · 2021-08-04T09:48:52Z

I've finished sha256 refactor was writing tests. I will make it another PR

orkunkl force-pushed the cw20-merkle-airdrop branch from 6d98e81 to e4ae969 Compare July 29, 2021 11:49

hashedone reviewed Jul 29, 2021

View reviewed changes

orkunkl requested a review from hashedone July 29, 2021 13:50

orkunkl added 25 commits July 30, 2021 14:19

Copy airdrop contract from anchor to cw-plus

d28452f

state, msgs done; insantiate done

622b5f5

update_config and register_merkle_root done

ff00722

Finish execute

93a2641

Finish query

cf6c6e9

Run fmt

bec5be0

First test works; clippy passes

27c1f59

Add update test

043779b

Start helper client

9f62f8e

Generate root command works

908ebc5

Register merkle root test

64a4a99

Move tests

d0788af

Fmt, clippy

10dcb1e

Remove helpers

ab08d33

Airdrop cli boilerplate

30887a0

Helpers.ts

3d0b822

Revert "Remove helpers"

736b2f0

This reverts commit 6c8c4c7.

cli generate root works

2f2501e

Generate proofs works

6b06bcc

Tests pass; cli readme

9a623ac

Update schema

f1c41c7

Add specs

1b7db54

Update cargo lock

c7ac243

Add contract to CI

62ca9bd

Remove left over code

8ce59b7

orkunkl added 3 commits July 30, 2021 14:22

Remove item change

7d7af42

sort() => sort_unstable()

18d68f3

Clippy happy

f62727f

hashedone reviewed Jul 30, 2021

View reviewed changes

contracts/cw20-merkle-airdrop/src/state.rs Show resolved Hide resolved

hashedone reviewed Jul 30, 2021

View reviewed changes

contracts/cw20-merkle-airdrop/src/contract.rs Outdated Show resolved Hide resolved

hashedone reviewed Jul 30, 2021

View reviewed changes

contracts/cw20-merkle-airdrop/src/contract.rs Outdated Show resolved Hide resolved

hashedone reviewed Jul 30, 2021

View reviewed changes

contracts/cw20-merkle-airdrop/src/msg.rs Show resolved Hide resolved

Apply review recommendations

eb753c4

orkunkl force-pushed the cw20-merkle-airdrop branch from 47fd8bc to eb753c4 Compare July 30, 2021 13:43

ethanfrey reviewed Jul 30, 2021

View reviewed changes

orkunkl added 4 commits July 30, 2021 17:01

Remove node_modules

fb5011e

Add tfl to licence

062152f

Update to 0.8.0-rc1

b905a15

Update licence

ad5e351

ethanfrey reviewed Jul 30, 2021

View reviewed changes

orkunkl added 3 commits July 30, 2021 18:13

Apply review recommendations

0086d03

Apply review recommendations

b3f1a5f

Add schema details

f7f5b75

ethanfrey reviewed Jul 30, 2021

View reviewed changes

orkunkl added 6 commits July 30, 2021 18:54

Apply review recommendations

3dde3ed

Apply review recommendations

8e3e50a

Apply review recommendations

1361827

Apply review recommendations

9a16a0e

Apply review recommendations

a3e555f

Apply review recommendations

9c9dff9

ethanfrey approved these changes Aug 3, 2021

View reviewed changes

ethanfrey merged commit d88742e into main Aug 3, 2021

ethanfrey deleted the cw20-merkle-airdrop branch August 3, 2021 20:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cw20-merkle-airdrop #364

Implement cw20-merkle-airdrop #364

orkunkl commented Jul 29, 2021

hashedone left a comment

hashedone Jul 29, 2021

hashedone Jul 29, 2021

ethanfrey Jul 30, 2021

orkunkl commented Jul 29, 2021

hashedone commented Jul 30, 2021

ethanfrey left a comment

ethanfrey Jul 30, 2021

orkunkl Jul 30, 2021

ethanfrey Aug 3, 2021

ethanfrey Jul 30, 2021

orkunkl Jul 30, 2021 •

edited

Loading

ethanfrey commented Jul 30, 2021

ethanfrey left a comment

ethanfrey Jul 30, 2021

ethanfrey Jul 30, 2021

ethanfrey Jul 30, 2021

ethanfrey left a comment

ethanfrey Jul 30, 2021

ethanfrey left a comment

ethanfrey Aug 3, 2021

orkunkl commented Aug 4, 2021


		This contract works with multiple airdrop rounds, meaning you can execute several airdrops using same instance.

		Uses SHA3 Keccak 256 for merkle root tree construction.

Implement cw20-merkle-airdrop #364

Implement cw20-merkle-airdrop #364

Conversation

orkunkl commented Jul 29, 2021

hashedone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orkunkl commented Jul 29, 2021

hashedone commented Jul 30, 2021

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orkunkl Jul 30, 2021 • edited Loading

Choose a reason for hiding this comment

ethanfrey commented Jul 30, 2021

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orkunkl commented Aug 4, 2021

orkunkl Jul 30, 2021 •

edited

Loading