Skip to content
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

Improve gas estimates on all-Dai carts and specific tokens #6918

Closed
wants to merge 1 commit into from

Conversation

mds1
Copy link
Contributor

@mds1 mds1 commented Jun 17, 2020

This PR has two changes in regards to gas estimates

Changes for Carts with ANT, aDAI, or cDAI

The current default estimate for token donations is 100k per donation, but this is insufficient for some tokens:

  • A standard ANT transfer costs up to 131k gas, so for ANT donations we add some margin and now use a value of 170k gas
  • An aDAI transfer can vary in gas cost depending on your address status in the platform (e.g. if you are borrowing it might be more costly than if you're not borrowing). The Aave team suggested a gas limit of 500k per transfer which they say should be more than enough
  • If transferring cDAI in MetaMask, the gas limit is automatically set to 400k. So we add a bit of margin here and use 450k gas per transfer

Changes for Carts that Only Use DAI

If a cart is all Dai (which is about 75% of checkouts), we use a linear regression to estimate the gas price based on actual transaction data.

The raw data was obtained using the script located at this repo: https://github.com/mds1/Gitcoin-Checkout-Gas-Analysis

That script does the following:

  1. Finds all cart checkout transactions that only use Dai
  2. For a given number of donations, generate a data set that only keeps the transactions that used the most gas
  3. Plot those results in Plotly

We then used the Plotly Chart Studio to generate a linear best fit line, then tweak the result so the results are more conservative than a typical best fit line (because we'd rather overestimate gas costs than underestimate). This chart can be found in Plotly Chart Studio here: https://plotly.com/~mds1/7/number-of-donations-vs-gas-used-all/

This gave us a new curve of gasLimit = 25000 * n + 125000; where n is number of donations in a transaction. For 1 donation we use a special case and set the estimate to 80k to avoid overestimating too much (in practice this will not really be seen since the default contribution to Gitcoin means there's two donations when you choose one grant).

Some comparisons are shown below:

# of Donations Per Tx Old Estimate New Estimate True Value
1 100k 80k 44k
2 200k 175k 84k
3 300k 200k 122k
5 500k 250k 181k
9 900k 350k 296k
15 1.5M 500k 396k
25 2.5M 750k 658k
40 4.0M 1.125M 1.04M

cc @owocki @apbendi

@mds1 mds1 changed the title Improve gas estimates on all-Dai carts Improve gas estimates on all-Dai carts and specific tokens Jun 19, 2020
@mds1 mds1 force-pushed the improve-gas-limit-estimate branch 2 times, most recently from df483d3 to 5e621b9 Compare June 19, 2020 14:56
@apbendi
Copy link
Contributor

apbendi commented Jun 22, 2020

Great work on this @mds1, it looks good to me. Would love to see this merged today— the high gas estimations can be scary when you're doing a large cart. Bet some folks are abandoning carts on this.

@danlipert danlipert changed the base branch from stable to master June 23, 2020 10:18
@danlipert danlipert changed the base branch from master to stable June 23, 2020 10:50
@danlipert
Copy link
Contributor

Since this was created against stable and I dont have edit permissions to rebase it properly we'll have to deploy it separately

@mds1 mds1 changed the base branch from stable to master June 23, 2020 13:37
@mds1 mds1 force-pushed the improve-gas-limit-estimate branch from 88d1393 to 6f85d00 Compare June 23, 2020 13:48
@mds1
Copy link
Contributor Author

mds1 commented Jun 23, 2020

@danlipert Just changed the base branch to master and squashed the commits. Let me know if there's anything else you want me to change here

@mds1
Copy link
Contributor Author

mds1 commented Jun 23, 2020

Ah looks like this branch it out of date, I'll just create a clean PR branched off the current master

@mds1
Copy link
Contributor Author

mds1 commented Jun 23, 2020

@danlipert Cherry picked the commit to an updated branch, so this PR is now replaced by #6961

@mds1 mds1 closed this Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants