-
Notifications
You must be signed in to change notification settings - Fork 39
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
Bulk donation support #96
Conversation
- add test that ensures only dust remains after swap - add test to verify donation input ratios - add test for duplicate swap tokens
IERC20 public constant WETH = IERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2); | ||
|
||
/// @notice Scale factor | ||
uint256 internal constant WAD = 1e18; |
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.
decimals here may lead to misleading amounts for tokens like USDC which only go to 1e6
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.
Hm, how so? Even if USDC amount where 1e6 =1 USDC, say you want 25% of 1 USDC, then you'd have 1e6 * 0.25e18 / 1e18 = 250000 = 0.25 USDC
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.
right but what if someone tries to swap 0.2500009 USDC and submits that to the swap method on uni won't it revert?
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.
Do you have an example of how we might get such a value? When we divide by 1e18 at the end, we'll either be left 250,000 (0.25 USDC) or 250,001 (0.250001 USDC) because there's only 6 decimals of precision when do you tokenDecimalsUsdc * xe18 / 1e18
struct Donation { | ||
uint96 grantId; // grant ID to which donation is being made | ||
IERC20 token; // address of the token to donate | ||
uint256 ratio; // ratio of `token` to donate, specified as numerator where WAD = 1e18 = 100% |
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.
for swaps with different decimals we could floor the amount for the tokens with the least amount of significant figures
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.
Seems related to the above comment, but I'm not sure what you mean here and where you need to floor things that isn't already floored by integer division in Solidity?
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.
maybe I'm missing something, but it's not related to the calculation per say but when we transfer an amount, emitting one value but really another has been sent to the contract (even if it doesnt revert)
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 _donationAmount
that is transferred is the same value as we emit in the GrantDonation
method. Sorry, I don't think I understand the issue you are pointing out 😬
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.
@corydickson I'm just running the example in my head and i don't see an issue either.
When donate is invoked for a given token -> sum of ratios should be 1e18.
So if we have a token with 16 decimals and contributions are made to 3 diff grants -> 5 , 5, 10
The ratio would be 25%, 25%, 50%.
The frontend would backfill the last 2 decimals with 0 and send it.
So once we do the swap to 18 decimal token -> you'd still have the same ratio of 25%, 25%, 50% right ?
The only scenario where this might mess things up is if we had a token > 18 decimals (which is not something we need to be bothered about)
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.
There's a good amount of complexity in what we're doing here, so really nice job tackling this 👍
I left some comments and questions and will follow up further based on responses. Thanks!
|
||
// Token approvals of common tokens | ||
// TODO inherit from SwapRouter to remove the need for this approvals and extra safeTransferFrom before swap | ||
IERC20(0x6B175474E89094C44Da98b954EedeAC495271d0F).safeApprove(address(_router), type(uint256).max); // DAI |
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.
Just want to make sure I understand: as currently written, the donate method will fail if it's not using one of these pre-approved tokens, and there is no mechanism for the contract to approve new ones. Correct?
Inheriting from SwapRouter means we'd become a custom v3 router, and users would have to trust (or verify by inspected verified source code) that we did not modify the router functionality in some malicious or unsafe way. Correct?
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.
Correct on all counts! If for whatever reason we don't end up inheriting from SwapRouter, I think we'd want a public method where you pass in a token address and it approves the SwapRouter to spend MaxUint of that token. Otherwise we have to check allowances every donation which would get very costly
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.
Got it, sounds good. I think we want to go this route (ha!). Created this issue to track as a separate PR: #99
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.
Just a clarification -> Even if we inherited the list -> wouldn't we still have to do an approve for every token on the list right ? (the only diff is we won't be hardcoding every token within this 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.
So here is the flow when you normally use Uniswap as a user:
- Approve router to spend your tokens
- Call swap on router, which takes your tokens and executes swap
- (1 total approval, 1 transferFrom)
Here is our current flow:
- Approve manager to spend your tokens
- Manager approves router to spend it's tokens
- Call donate on manager, which takes your tokens, the router takes tokens from manager to execute swap (no more approvals)
- (2 total approvals, 2 transferFroms)
So once we inherit from SwapRouter, we'll migrate to the top flow, which will reduce approvals and transfers
SwapSummary[] calldata _swaps, | ||
uint256 _deadline, | ||
Donation[] calldata _donations | ||
) external payable { | ||
// --- Validation --- |
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 three steps executed in this method are:
- Validate
- Swap
- Donate
These feel like good candidates for 3 distinct internal helper methods. It would add some gas overhead, but what we're doing is already super gassy, so it might be worth it from a code hygiene and comprehensibility standpoint.
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.
Split up in 4fef97a
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.
Hate to nitpick but I'm going to anyway: not a fan of the naming here. What would you think of something more like:
_validateDonations
_executeDonationSwaps
_transferDonations
Then once we have nice verbose names, we don't even need the comments :)
Also, missed this in my initial comments, but we could also extract the last for loop into _clearDonationMappings
or the like. Actually, I wonder if you could make this a modifier? Something like:
modifier clearsTemp(SwapSummary[] calldata _swaps) {
_;
for (uint256 i = 0; i < _swaps.length; i++) {
IERC20 _tokenIn = IERC20(_swaps[i].path.toAddress(0));
swapOutputs[_tokenIn] = 0;
donationRatios[_tokenIn] = 0;
}
}
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.
For context on the function naming, I used the same convention as the app
of naming things <parent-purpose>
to make it easier to understand something's purpose / where it fits in. So here the parent method is donate
and we have 3 sub-methods of validation, swaps, and execution. I don't feel strongly about the naming, so I have no objections to renaming them if that reasoning doesn't convince you
Re: modifier, I'd personally vote against that since it's only used one time, so I'm not sure pulling it out adds any value, and just costs extra gas
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.
Though I do think we should add underscores to prefix internal methods regardless, so good call there, but I'll hold off pending a final decision on naming
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.
Wasn't aware we had that convention on the frontend. Does that come from Vue?
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.
Yea it's based off their style guide: https://vuejs.org/v2/style-guide/#Tightly-coupled-component-names-strongly-recommended
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.
A bit tangential, but @thelostone-mc perhaps worth mentioning that style guide in the CONTRIBUTING doc?
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.
Got it. I can see how that convention makes sense for Components, but not sure it fits well for helper methods. The point of breaking the original function up was for code clarity, so easily grokable names seems pretty important. That said, don't feel strongly enough to block the PR or anything! I think I've made my case and will let you exercise your judgement!
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.
Fair enough! Updated in 6ea6b59
- fix GTC name and symbol in tokens declaration - remove requirement to contribute for a round - split up core donate method into internal methods for readability - clean up lint warnings
886977a
to
6ea6b59
Compare
* @param _donations Array of donations that will be executed | ||
*/ | ||
function _validateDonations(Donation[] calldata _donations) internal { | ||
// TODO consider moving this to the section where we already loop through donations in case that saves a lot of |
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.
are we saying
- keep the function
- let the argument be Donation _donation
- it gets invoked as the first thing in _transferDonations
It does seem a smart way to save gas but this would mean -> we'd do it after the swap happens which is I am not very comfortable about (but the argument could be made -> if you invoke it wrong -> it's not the contract's fault )
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.
Yea pretty much! The benefits of the current approach are that you save gas in the event of a failure (by reverting before the swap), but downside is more gas overall since you repeat a loop. I don't think the extra gas is significant, so I think we should we leave this as-is for readability, and come back to it later when we focus on optimizing gas usage
Closes #76
The
GrantRoundManager
contract, and in particular thedonate
method, feel a bit clunky now, but I'm not totally sure how to clean it up. There's a few things that we could handle in separate issues to make it cleaner, so curious to hear thoughts around this:payee
address the grant ID (Discuss: Use a grant's `payee` address as it's ID #54) removes a validation checkassertValidContribution(address token)
method to theGrantRound
can be used to combine theisActive
anddonationToken
checks into a single callGrantRoundManager
inherit fromSwapRouter
(Update `GrantRoundManager` to inherit from Uniswap's `SwapRouter`, instead of call an external router #74) would remove approvals and transfers that are currently neededCurrent code coverage results are below