-
Notifications
You must be signed in to change notification settings - Fork 12
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
Add support for anchoring with an ERC20 contract with deposit and withdraw #24
Conversation
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.
Some initial thoughts - will come back to solidity code
// CheckHashesValue is a circuit that checks the integrity of transactions of Fungible Tokens | ||
// - check that all output values are positive numbers (within the range of 0 to 2^40) | ||
// - check that the output commitments are the hash of the output values | ||
// - check that the sum of output values equals a total value in the output |
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.
?? we just add them up and return the output from what I can see
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.
yes this is a confusing aspect of ZKP circuits:
- the proof generator calculates the output, which is included in the proof object
- the output is always part of the public input parameters used to verify the proof
- the verifier independently assembles the public input parameters, in this case the expected value from the
amount
parameter of the transaction function - if the proof verification succeeds, then the verifier knows that the values represented by the output UTXOs are exactly equal to the
amount
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.
Awesome - happy about that
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
…rcuit Signed-off-by: Jim Zhang <[email protected]>
Signed-off-by: Jim Zhang <[email protected]>
…hdraw Signed-off-by: Jim Zhang <[email protected]>
829b321
to
0255683
Compare
Signed-off-by: Jim Zhang <[email protected]>
processInputsAndOutputs(inputs, [output, 0]); | ||
} | ||
|
||
function mint(uint256[] memory utxos) public onlyOwner { |
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.
@jimthematrix might need a bit more elaboration on the design intention here.
Is the design intention that the UTXO pool can have both erc20 anchored token and the normal tokens?
But there isn't any flag introduced to distinguish the two types of tokens in the UTXO pools in the PR.
This means anyone who has the UTXO token can use the withdraw function to redeem ERC20 tokens on the anchored contract regardless of whether the input UTXOs are created through anchoring (deposit
function).
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.
it's a fair question. the mint()
method is reserved for the authority that has deployed the token contract as a way to adjust the supply of the currency. since it's a highly privileged operation, per the onlyOwner
modifier, it's at the authority's discretion. I feel most real world currency systems would want controls like this
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.
to fix the erc20 shortage that could be caused due to usage of this mint function, we could bind the invocation of erc20 mint call in this mint function.
Happy for that to be a follow-on PR as it will require some refactor to make erc20 invocation optional when calling mint
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.
thanks for pointing out the scenario @Chengxuan where when an authority mints directly to Zeto UTXOs, but forgets to mint the same amount to ERC20, then an imbalance would be introduced that could cause withdraws to fail since the supplies in the Zeto contract would be larger than the ERC20 balance held by the Zeto contract.
beside providing a sample solution that performs the ERC20 mint by the Zeto contract (which would require a more elaborate ERC20 implementation that allows for additional authorities to mint besides the owner), we should also document that when minting Zeto tokens in a Zeto contract that has an anchored ERC20, the same authority should make sure to mint the same amount in ERC20 and give it to the Zeto contract
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 to me! Great comments across the both the circuits and smart contracts 😃
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!
No description provided.