-
Notifications
You must be signed in to change notification settings - Fork 15
Burn Request Fix #18
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
base: main
Are you sure you want to change the base?
Burn Request Fix #18
Conversation
muskbuster
commented
Nov 20, 2024
- Locks transfers from sender side while waiting for burn callback ( Locks entire balance)
- Allows retry of burn which can be overridden to add some cooldown time.
npasquie
left a comment
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.
The locked token mechanism is correct but the implem has some issues
tbh we should redo the reference implem from scratch or take the one from zama if we agree on design choices (which we don't necessarly)
| Identity public immutable identityContract; | ||
| uint8 private minimumAge; | ||
| uint64 public constant transferLimit = (20000 * 10 ** 6); | ||
| euint64 public TRANSFER_LIMIT = TFHE.asEuint64(transferLimit); // 20,000 tokens with 6 decimals |
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 is it for ?
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.
transferLimit becomes unused
| struct BurnRq { | ||
| address account; | ||
| uint64 amount; | ||
| bool exists; |
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'm not convinced this is useful ?
It only ever has true as a value, "non existant" Burn Requests have address(0) as account and amount 0
| function transfer(address to, euint64 value) public virtual returns (bool) { | ||
| require(TFHE.isSenderAllowed(value)); | ||
| address owner = _msgSender(); | ||
| ebool isTransferable = TFHE.le(value, _balances[msg.sender]); |
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 remove this ?
|
|
||
| ebool canTransfer = TFHE.le(amount, _balances[owner]); | ||
| ebool isTransferable = TFHE.and(canTransfer, allowedTransfer); | ||
| ebool isTransferable = TFHE.le(amount, currentAllowance); |
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.
shouldn't the eamount already containt this check and passed when this func is called ?
| * | ||
| * Does not emit an {Approval} event. | ||
| */ | ||
| function _increaseAllowance(address spender, euint64 addedValue) internal virtual returns (ebool) { |
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 does this function even exist
| _totalSupply += value; | ||
| } | ||
|
|
||
| event BurnRequested(uint256 requestId, address account, uint64 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.
events should be at the top of the file
|
|
||
| error BurnRequestDoesNotExist(uint256 requestId); | ||
|
|
||
| function _cancelBurn(uint256 requestId) internal virtual { |
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.
you must check the calling account is allowed to do this
| function _burnCallback(uint256 requestID, bool decryptedInput) public virtual onlyGateway { | ||
| event InsufficientBalanceToBurn(address account, uint64 burnAmount); | ||
|
|
||
| function _burnCallback(uint256 requestID, bool hasEnoughBalance) public virtual onlyGateway { |
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.
the underlyning token transfer is missing
| */ | ||
| constructor(string memory name_, string memory symbol_) ConfidentialERC20(name_, symbol_) { | ||
| _owner = msg.sender; | ||
| constructor(string memory name_, string memory symbol_) ConfidentialERC20(name_, symbol_)Ownable(msg.sender) { |
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 reintroduce the owner
| } | ||
|
|
||
| function _burnCallback(uint256 requestID, bool decryptedInput) public virtual override onlyGateway { | ||
| function _burnCallback(uint256 requestID, bool hasEnoughBalance) public override onlyGateway { |
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 are we overriding this method if it already does the same thing in confidential erc20 ?