Internal review - Voucher #30
Conversation
Co-authored-by: Zied Guesmi <[email protected]>
Co-authored-by: Zied Guesmi <[email protected]>
…e to refund tasks fairly (#28)
Co-authored-by: Zied Guesmi <[email protected]>
| _mint(voucherAddress, value); // VCHR | ||
| $._isVoucher[voucherAddress] = true; | ||
| emit VoucherCreated(voucherAddress, owner, voucherType, expiration, value); | ||
| _transferFundsToVoucherOnPoco(voucherAddress, value); // SRLC |
There was a problem hiding this comment.
| _mint(voucherAddress, value); // VCHR | |
| $._isVoucher[voucherAddress] = true; | |
| emit VoucherCreated(voucherAddress, owner, voucherType, expiration, value); | |
| _transferFundsToVoucherOnPoco(voucherAddress, value); // SRLC | |
| if (value > 0) { | |
| _mint(voucherAddress, value); // VCHR | |
| _transferFundsToVoucherOnPoco(voucherAddress, value); // SRLC | |
| } | |
| $._isVoucher[voucherAddress] = true; | |
| emit VoucherCreated(voucherAddress, owner, voucherType, expiration, value); |
There was a problem hiding this comment.
Yes,
- Either add
if (value > 0) - Or maybe
require(value > 0)(like top up)
| sponsoredAmount -= sponsoredAmount % volume; | ||
| if (sponsoredAmount > 0) { | ||
| _burn(msg.sender, sponsoredAmount); | ||
| emit VoucherDebited(msg.sender, sponsoredAmount); |
There was a problem hiding this comment.
These is already a burn event (transfer to 0). Is that not enough?
There was a problem hiding this comment.
It was just here to save a bit of gas to avoid updating balance to 0 and firing event.
Maybe it is a quite not good idea to do so 🤔
There was a problem hiding this comment.
Ok your comment was about the relevance of the event itself!
There was a problem hiding this comment.
The idea is to add a feature-specific event as the burn event occurs on different occasions (drain feature for example).
| * on PoCo from voucher to voucherHub and burn all credits. | ||
| * @param voucher address of the expired voucher to drain | ||
| */ | ||
| function drainVoucher(address voucher) external { |
There was a problem hiding this comment.
there is no access control here. Is that normal? Doesn't feel right
There was a problem hiding this comment.
After check, its ok because Voucher(voucher).drain(amount) checks expiry, and the msg.sender is not used.
|
|
||
| function _getVoucherHubStorage() private pure returns (VoucherHubStorage storage $) { | ||
| //slither-disable-start assembly | ||
| assembly { |
There was a problem hiding this comment.
| assembly { | |
| assembly ("memory-safe") { |
| /// @custom:storage-location erc7201:iexec.voucher.storage.Voucher | ||
| struct VoucherStorage { | ||
| address _owner; | ||
| address _voucherHub; |
There was a problem hiding this comment.
This izs the same for all instances. Maybe it should be an immutable variable
There was a problem hiding this comment.
If it becomes immutable, how to update storage slot if someday VoucherHub address changes? Should we consider we are safe since VoucherHub is UUPSUpgradeable
| * @param dealId id of the task's deal | ||
| * @param taskIndex task's index in the deal | ||
| */ | ||
| function claimBoost(bytes32 dealId, uint256 taskIndex) external { |
There was a problem hiding this comment.
why does this use dealId and taskIndex instead of taskId (like claim) ?
There was a problem hiding this comment.
Because in Boost mode, there is not initialize() so there is not task definition before pushing the result of compute.
Check Boost delegate on PoCo https://github.com/iExecBlockchainComputing/PoCo/blob/develop/contracts/modules/delegates/IexecPocoBoostDelegate.sol#L371
| * Funds are sent to the VoucherHub contract. | ||
| * @param amount amount to be drained | ||
| */ | ||
| function drain(uint256 amount) external onlyVoucherHub onlyExpired { |
There was a problem hiding this comment.
maybe this function should be replaced by:
- doing
ERC20(IVoucherHub(voucherHub).getIexecPoco()).approve(voucherHub, type(uint256).max)at initialization - doing a transferFrom in VoucherHub (instead of calling this)
There was a problem hiding this comment.
The issue with this (if I understand your suggestion correctly) is that anyone could do a transferFrom(voucher => voucherHub) before the expiration date.
There was a problem hiding this comment.
I agree with you Hadrien, it looks cleaner (At the time I did not say anything because it could raise gas costs but did not check how much)
| uint256 dealPrice = datasetOrder.dataset != address(0) | ||
| ? (appPrice + datasetPrice + workerpoolPrice) * volume | ||
| : (appPrice + workerpoolPrice) * volume; |
There was a problem hiding this comment.
| uint256 dealPrice = datasetOrder.dataset != address(0) | |
| ? (appPrice + datasetPrice + workerpoolPrice) * volume | |
| : (appPrice + workerpoolPrice) * volume; | |
| uint256 dealPrice = ( | |
| appPrice + | |
| workerpoolPrice + | |
| (datasetOrder.dataset != address(0) ? datasetPrice : 0) | |
| ) * volume; |
Reminder: PR is for comments only, it won't be merged.