-
Notifications
You must be signed in to change notification settings - Fork 92
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
Implemented Merkle Airdrop Support for Initial Token Distribution #73
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@abdelhamidbakhta I have low trust in alexandria libs - can we get someone to review them? |
Resolves #28 @Akashneelesh (dont forget) to link issues to PRs |
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.
LGTM overall but we need to make sure we don't have unused variables, useless comments and have tests organized otherwise maintainance will be a hell
Sorry, I added a lot of minor changes so that the tests are clean and standardized. The biggest issue might be in the claim airdrop function. I'm not sure how the amount is defined or who will call this function |
Firstly to answer about who will call this : The users would call this function. Amount is defined in the list of addressAmountPairs (you could take a look at the scripts/merkletree.ts). So I did take this into consideration, and because the leaf (pedersen hash of the to_address and amount) , The Merkle proof generated for a particular leaf in a Merkle tree changes based on the values in the tree. And hence in the contract, theres an assertion of the hashed_value(to_address,amount) and the leaf. Only if that goes through then the proof is verified. And the amount is minted. |
//Pedersen Hashing of the ContractAddress and Amount | ||
let to_felt252: felt252 = starknet::contract_address_to_felt252(to); | ||
let amount_felt252: felt252 = amount.try_into().unwrap(); | ||
let hashed_value: felt252 = pedersen::pedersen(to_felt252, amount_felt252); |
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.
contractAddress fits into felt252, but not amount. Here you're performing a hash, not on the original data, but on a truncating value. why not hashing the real amount (on 256 bits)?
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.
Yeah true, I had casted amount to felt252 because, the pedersen::pedersend requires (felt252,felt252) as inputs.
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.
I think we should hash the full amount 🤔 I will look into alexandria's merkle tree implementation but I think it can be improved 🤔
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.
imo you should
let mut hash_state = pedersen(to_felt252, amount.low.into())
hash_state = pedersen(hash_state, amount.high.into())
that way you are hashing the correct data
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.
Ohh fine, I'll do this change
@enitrat what is the issue with the alexandria libs? Also I am not sure to understand why we are using pedersen when poseidon is cheaper? EDIT: Is it because starknet.js manage only pedersen hash in its merkle feature? import { merkle, ec } from "starknet"; |
there's pretty much no maintenence of already existing stuff, with the language rapidly evolving I would think some patterns are un-optimized / outdated and some design decisions a bit weird (e.g the way hash function are used in the merkle trees are implemented is super weird) |
|
||
fn get_merkle_root(self: @ContractState) -> felt252 { | ||
//Getting the merkle root | ||
self.ownable.assert_only_owner(); |
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 is there an access control on this view?
I have taken all the description and the changes mentioned in the telegram group into consideration and have done the changes.