Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Bulk donation support #96
Changes from all commits
bf77409
8b70596
a891025
485cf89
d5ef4a2
a1354ac
4fef97a
6ea6b59
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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
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 theGrantDonation
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.
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:
Here is our current flow:
So once we inherit from SwapRouter, we'll migrate to the top flow, which will reduce approvals and transfers
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
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