Add Giver #338
Add Giver #338
Conversation
CodeSandwich
force-pushed
the
igor/giver
branch
4 times, most recently
from
b6104b4 to
7f21685
Compare
| /// and each `Giver` has a single account ID assigned. | ||
| /// Any ERC-20 tokens or native tokens sent to `Giver` will | ||
| /// eventually be `give`n to the account assigned to it. | ||
| contract GiversRegistry is Managed { |
There was a problem hiding this comment.
I think there pros and cons are making it upgradeable. I get that everything is upgradeable so you are following the pattern here.
However, it is also kind of nice, if the giver can only do one thing using the Drips functionality.
There was a problem hiding this comment.
I think that it's risky to make GiversRegistry and by extension Giver unupgradeable. This is all the users can do with their Giver-based addresses, and we may want to add features later. Both Drips and AddressDriver are upgradeable too, and we may want to use some future parts of their APIs.
| /// If it's the zero address, `Giver` wraps all the native tokens it holds using | ||
| /// `nativeTokenWrapper`, and then `give`s to the account all the wrapped tokens it holds. | ||
| /// @param amt The amount of tokens that were `give`n. | ||
| function give(uint256 accountId, IERC20 erc20) public whenNotPaused returns (uint256 amt) { |
There was a problem hiding this comment.
I am just checking here to see if it would revert if the implementation contract is called directly for this function.
This would revert because implementation() on the implementation contract would return address 0.
Maybe consider adding the onlyProxy modifier here. Alternatively, maybe an init method with initializer where you create the first
new Giver(); for the logic address?
There was a problem hiding this comment.
This function is not callable on a non-proxied instance of GiversRegistry because it's whenNotPaused and it's always paused on deployment, see https://github.com/drips-network/contracts/blob/main/src/Managed.sol#L79C22-L79C22.
An init function doing just new Giver(); sounds like a smart move, why waste gas on checking if the giver logic is there?
There was a problem hiding this comment.
Added initialize()
| /// `nativeTokenWrapper`, and then `give`s to the account all the wrapped tokens. | ||
| /// @param amt The amount of tokens that were `give`n. | ||
| function giveImpl(uint256 accountId, IERC20 erc20) public returns (uint256 amt) { | ||
| require(address(this) == _giver(accountId, msg.sender), "Caller is not GiversRegistry"); |
There was a problem hiding this comment.
| require(address(this) == _giver(accountId, msg.sender), "Caller is not GiversRegistry"); | |
| // address(this) in this context should be the giver contract | |
| require(address(this) == _giver(accountId, msg.sender), "Not Delegate Call from Giver"); |
I think it would be nice to add some comments here.
| } | ||
| (uint128 streamsBalance, uint128 splitsBalance) = _drips.balances(erc20); | ||
| uint256 maxAmt = _maxTotalBalance - streamsBalance - splitsBalance; | ||
| amt = erc20.balanceOf(address(this)); |
There was a problem hiding this comment.
| amt = erc20.balanceOf(address(this)); | |
| // balance of the giver contract | |
| amt = erc20.balanceOf(address(this)); |
| // slither-disable-next-line unused-return | ||
| if (!Address.isContract(giver_)) Clones.cloneDeterministic(giverLogic, bytes32(accountId)); | ||
| bytes memory delegateCalldata = abi.encodeCall(this.giveImpl, (accountId, erc20)); | ||
| bytes memory returned = Giver(payable(giver_)).delegate(implementation(), delegateCalldata); |
There was a problem hiding this comment.
From the perspective here the naming Giver(payable(giver_)).delegate might be misleading. Since, we are not calling giver contract with a delegate call. It is a regular call to a function which performs a delegate call. Might be easier to call it give or something.
There was a problem hiding this comment.
delegate only delegates, it makes no assumptions on what function it delegates to. If we call it give and then later we use it to delegate to a function that doesn't actually give, that will be really misleading. I think that the current name is correct and precise.
| /// @return ret The data returned from the delegation. | ||
| function delegate(address target, bytes memory data) | ||
| public | ||
| payable |
There was a problem hiding this comment.
Why payable for gas optimization?
There was a problem hiding this comment.
It's because this is the non-upgradeable part of Giver, and it adds a tiny bit of flexibility allowing delegation to a payable function. It probably will never be helpful, but it doesn't hurt either.
| /// If it's the zero address, `Giver` wraps all the native tokens it holds using | ||
| /// `nativeTokenWrapper`, and then `give`s to the account all the wrapped tokens it holds. | ||
| /// @param amt The amount of tokens that were `give`n. | ||
| function give(uint256 accountId, IERC20 erc20) public whenNotPaused returns (uint256 amt) { |
There was a problem hiding this comment.
Okay, since it is the full account Id and not only the subAccount you see it kind of as a global giver system. It can be used for all accountId in Drips, correct?
CodeSandwich
force-pushed
the
igor/giver
branch
from
bc440ea to
102bea8
Compare
Adds a mechanism for
giveing without interacting with the protocol. The users only need to send tokens to the address derived from the account ID, and then somebody needs to callGiversRegistry.giveto actuallygiveall the tokens.