-
Notifications
You must be signed in to change notification settings - Fork 362
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
feat: r/demo/foo20airdrop - Merkle Airdrop contract example #906
base: master
Are you sure you want to change the base?
Conversation
2060eff
to
2675ea9
Compare
|
||
func TestClaimAirdrop(t *testing.T) { | ||
contractAddr := std.DerivePkgAddr("gno.land/r/demo/foo20-airdrop") | ||
std.TestSetOrigCaller(contractAddr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you still need that ?
p/demo/foo20
uses std.PrevRealm()
now right ?
"crypto/sha256" | ||
"encoding/hex" | ||
"errors" | ||
"fmt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't work yet
@@ -0,0 +1,88 @@ | |||
package airdrop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p/demo/airdrop
seems to be too generic IMO, so I would either name it accurately like p/demo/grc20_merkle_airdrop
or use a subpaths, maybe p/demo/airdrop/grc20_merkle
.
type AirdropData struct { | ||
Address std.Address | ||
// TODO: use std.Coin | ||
Amount uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better control and data integrity, keep the fields in the Airdrop
struct unexported.
To maintain easy marshaling with the Bytes
helper, consider this approach:
type Airdrop struct {
data struct {
Address std.Address
Amount uint64
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 💯
Please see @moul's comments, otherwise we should be good to go
ma.claimed.Iterate("", "", func(k string, v interface{}) bool { | ||
claimed += v.(uint64) | ||
return false | ||
}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, do you forsee this method being called often? If not we can leave the iteration, otherwise we might want to consider keeping a claimed counter in MerkleAirdrop
proofs, err := tree.Proof(leaf) | ||
if err != nil { | ||
t.Fatalf("failed to generate proof, %v", err) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant return here
var ( | ||
token grc20.IGRC20 = foo20.GRC20() | ||
|
||
// admin std.Address = "g1sw5xklxjjuv0yvuxy5f5s3l3mnj0nqq626a9wr" // albttx.gno |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover?
err := foo20airdrop.Claim(data, proofs) | ||
if err != nil { | ||
panic(err.Error()) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: you can inline this
} | ||
|
||
// for tests purpose | ||
func reset() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you modify foo20airdrop
from the test directly, since they are in the same package? You can then drop this method entirely
}, | ||
} | ||
|
||
func TestRegisterMerkle(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about completely removing this test?
You verify it works in TestClaimAirdrop
regardless (happy path)
// instantiate foo20 airdrop contract | ||
tree := merkle.NewTree(leaves) | ||
RegisterMerkleRoot(tree.Root()) | ||
defer reset() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the need for a defer
(reset at all)?
ttClaimed := TotalClaimed() | ||
if ttClaimed != sumClaimed { | ||
t.Fatalf("expected: %d", sumClaimed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: you can inline this
proofs, err := tree.Proof(leaf) | ||
if err != nil { | ||
t.Fatalf("failed to generate proof, %v", err) | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redundant return
"gno.land/r/demo/users" | ||
) | ||
|
||
var leaves []merkle.Hashable = []airdrop.AirdropData{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving this to TestClaimAirdrop
? (see my next comment about removing TestRegisterMerkle
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just blocking since there are several changes. Let’s make an additional round of reviews.
There is tests issue once rebased with master. There is a situation to figure out, with When Possible fix could be to update in |
Related (and depending?) on #952 |
Hey @albttx, What's the status of this PR? |
7c384db
to
2588613
Compare
2588613
to
6b06c76
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #906 +/- ##
==========================================
- Coverage 47.84% 47.54% -0.31%
==========================================
Files 369 367 -2
Lines 62764 62307 -457
==========================================
- Hits 30028 29622 -406
+ Misses 30308 30265 -43
+ Partials 2428 2420 -8
☔ View full report in Codecov by Sentry. |
Description
This PR bring an example for a merkle airdrop contract.
p/demo/airdrop
r/demo/foo20airdrop
How it's works ?
A merkle airdrop contract works by saving off-chain the elements of the merkle tree. ONLY the Merkle root is saved on the realm.
To confirm an element is present, we just need to send to the contract a merkle proof.