Add script to swap ERC-20 token balances in Gitcoin Fee address to ETH using Uniswap #4517
Conversation
|
@owocki @danlipert @SaptakS @thelostone-mc Redo of #4229 and #4383 with clean commit history. Sorry for multiple do-overs. |
Codecov Report
@@ Coverage Diff @@
## master #4517 +/- ##
==========================================
+ Coverage 30.02% 30.08% +0.05%
==========================================
Files 209 214 +5
Lines 16886 17011 +125
Branches 2278 2292 +14
==========================================
+ Hits 5070 5117 +47
- Misses 11619 11694 +75
- Partials 197 200 +3
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #4517 +/- ##
==========================================
+ Coverage 29.83% 30.13% +0.29%
==========================================
Files 233 237 +4
Lines 19453 19590 +137
Branches 2782 2795 +13
==========================================
+ Hits 5804 5903 +99
- Misses 13410 13442 +32
- Partials 239 245 +6
Continue to review full report at Codecov.
|
|
@danlipert @thelostone-mc @SaptakS Checking in on this one. Any chance of getting it reviewed now that I've got the commit history cleaned up? As a reminder, it's the same code as now closed PRs #4229 and #4383. I believe what got mucked up before was my attempt at squashing accidentally included a bunch of commits from the master branch that somehow got roped into my PR when I was rebasing the branch per the contributor guidelines and I could never untangle them in the commit history. |
|
@danlipert Pause on the review of this. I was digging back through this with fresh eyes after a weekend away and noted some improvements and also some new insights for some unit tests. I'll try to knock out the changes in the next couple of days and give you a heads up on Slack when it's ready to revisit. |
acolytec3
changed the title
Sounds good! Appreciate all your hard work on this and the desire for more and better testing and improvements :) |
acolytec3
changed the title
|
@acolytec3 nice! appreciate the extra tests! Can you squash it down to 1 commit and rebase on master? Thanks! |
|
@danlipert Squashed and rebased. |
|
@danlipert @owocki Just checking in on this. Is there anything additional you're looking for on this one? |
|
no i dont think so. we'll get it into the review queue. thanks again for the hard work on this :) |
|
Not trying to be obnoxious but is there any chance of getting this either approved or else any other feedback? It's been sitting for quite a while. |
|
|
||
| # Gitcoin Bounty Funding Fee settings | ||
| FEE_ADDRESS = env('FEE_ADDRESS', default='') | ||
| FEE_ADDRESS_PRIVATE_KEY = env('FEE_ADDRESS_PRIVATE_KEY', default='') |
There was a problem hiding this comment.
is it possible to test this on the mainnet, a la FEE_ADDRESS_NETWORK?
There was a problem hiding this comment.
Sure, though I don't think you need to specify a special network setting in .env for that. You would just need to put an address and key with some mainnet ERC-20 balance in it, set the overall network in the .env for Mainnet, and then run the management command from the docker shell.
|
just left a few comments.. cc @danlipert on final approval |
| "balance":"10000000" | ||
| }, | ||
| { | ||
| "symbol":"WEENUS", |
There was a problem hiding this comment.
if this is a real token I'm giving up 😆
There was a problem hiding this comment.
Oh, it's real which means I'm a rich man. 🤘
|
@octavioamu @thelostone-mc can y'all take a look and lets get this deployed this week! 👍 |
|
@danlipert can we get this on the merge list for the next release pls?? |
|
⚡️ A tip worth 0.30000 ETH (52.95 USD @ $176.5/ETH) has been granted to @acolytec3 for this issue from @owocki. ⚡️ Nice work @acolytec3! Your tip has automatically been deposited in the ETH address we have on file.
|
|
|
hey @acolytec3 i setup the fee swapper on 0xee2303c55f024bf094b2c5ef41002d28f73d144a and deposited 10 STORJ into that account for testing.. but the feeswapper script still reutrns what am i doing wrong? pls advise |
|
storj is def a token supported by uniswap http://bits.owocki.com/aa2e0194b66f/Screen%20Shot%202019-11-01%20at%2010.48.53%20AM.png |
|
Will take a look and get back to you.
…On Fri, Nov 1, 2019 at 12:49 PM Kevin Owocki ***@***.***> wrote:
storj is def a token supported by uniswap
http://bits.owocki.com/aa2e0194b66f/Screen%20Shot%202019-11-01%20at%2010.48.53%20AM.png
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4517?email_source=notifications&email_token=AEENFXAN6A4EOPOLJ64YP7DQRRMYHA5CNFSM4HRDSZ4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC3PNJY#issuecomment-548861607>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEENFXFA4KPQIDF2UNLQXRLQRRMYHANCNFSM4HRDSZ4A>
.
|
|
i just got this email 51 minutes ago (the initial fail was 2 hours ago tho) STORJ conversion to ETH failed previously so no conversion was attempted. |
|
@acolytec3 looks like th escript did at some point send a tx to the mainnet! https://etherscan.io/tx/0xf5b72d8fbee40cd6c5fe883ba32a0b49ba79eaab90c33961e6007dc055dd2efa |
|
ah looks like i got a 'STORJ conversion to ETH failed' email to |
|
Interesting. The timing thing feels like the difference between block
times on testnet vs mainnet.
With regard to the transaction failing, this is the same sort of error I
saw on testnet where the transactions worked with the Rinkeby Uniswap
exchanges for some tokens (namely DAI) and not others (MKR and OMG). I can
look at the construction of the web3 transaction again but would also
welcome any other input on it.
…On Fri, Nov 1, 2019 at 3:01 PM Kevin Owocki ***@***.***> wrote:
ah looks like i got a 'STORJ conversion to ETH failed' email to
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4517?email_source=notifications&email_token=AEENFXBR6DXWGHP6V76JVRTQRR4IFA5CNFSM4HRDSZ4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEC33W2A#issuecomment-548911976>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEENFXHIE2OYBKF57FQSK5DQRR4IFANCNFSM4HRDSZ4A>
.
|
|
any luck? as it stands all this script is doing is sending me an email every hour telling me its not working lol |
|
@owocki ugh, so sorry I haven't gotten to this yet. Been neck deep in a
couple of PRs for the status team so will try and take a look at it this
week. I think I'm going to have to deploy a Uniswap contract with ganache
and step through all the internal transactions to see where it's failing
and that's not something I've done before so make take some more time.
I'll try to make some progress on it this week.
…On Mon, Nov 11, 2019 at 10:11 AM Kevin Owocki ***@***.***> wrote:
any luck? as it stands all this script is doing is sending me an email
every hour telling me its not working lol
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4517?email_source=notifications&email_token=AEENFXFN3KXP4CFOX4QRAD3QTFY2NA5CNFSM4HRDSZ4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDXDXVA#issuecomment-552483796>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEENFXATPR6KRHQYEJZFGJ3QTFY2NANCNFSM4HRDSZ4A>
.
|
|
Initial research indicates that there is indeed something wrong with the parameters I'm using in the Uniswap function call in the script. I also discovered that they released a JavaScript sdk since I wrote the initial code so will do a line by line comparison and see what's different and see if I can find the source of the issue that way. Barring that, will work further on deploying to ganache and trying to recreate. |
|
I've been back over the parameters and they match the Uniswap guidelines exactly as far as I can tell so not seeing anything new. I've also compared how I structure the transaction to the Uniswap Python Wrapper function that calls the TokenToEthSwapInput method on the Uniswap contract and here again, structure appears to match exactly. And, to top it all off, Blockscout discontinued their Rinkeby explorer/API so I can't even run the script in Debug mode anymore so...need a rethink on how to approach this. Any opinions? |
|
@acolytec3 maybe test on mainnet? i can send you a few tokens if that unblocks you |
|
I'm going to try and find some time in the next week to experiment with
that python Uniswap library. That would work on rinkeby. If that works, I'm
going to rip the guts out of the script and replace with that if you guys
are okay with adding the dependency.
…On Wed, Nov 20, 2019, 4:51 PM Kevin Owocki ***@***.***> wrote:
@acolytec3 <https://github.com/acolytec3> maybe test on mainnet? i can
send you a few tokens if that unblocks you
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4517?email_source=notifications&email_token=AEENFXBN6DF6RICXNFKPK7DQUWWP5A5CNFSM4HRDSZ4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEVLGWA#issuecomment-556446552>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AEENFXECO5YQUGUEDE2Q5SDQUWWP5ANCNFSM4HRDSZ4A>
.
|
|
@acolytec3 - the additional dependency is fine with me 👍 |
|
Further experimentation says that this library is out of date (of course). Will see if it's a simple fix to make it work for our purposes. |
|
thanks let me know
…On Sat, Nov 30, 2019 at 8:55 PM acolytec3 ***@***.***> wrote:
Further experimentation says that this library is out of date (of course).
Will see if it's a simple fix to make it work for our purposes.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4517?email_source=notifications&email_token=AAD5PCIO7HOQ6BFFP47TJOTQWMYRLA5CNFSM4HRDSZ4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFQ3BNA#issuecomment-560050356>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD5PCKVNXYYAQNTWBTYLB3QWMYRLANCNFSM4HRDSZ4A>
.
_________________________________________
gitcoin is live and has generated over $2.7mm for Open Source Software - see
our results <https://gitcoin.co/results>
|
|
Sorry for radio silence, holidays and other business got in the way. I've spent a fair bit more time experimenting with this and I confess I'm completely at a loss. The python-uniswap wrapper library is obsolete at this point and didn't provide any insights in its underlying code since it's doing the same thing that my script does when constructing the transactions. I extracted the part of the PR that interacts with the Uniswap exchange into it's own script so I could just run transactions and had the same experience as I reported when building the code base originally (i.e. the script works with the Rinkeby BAT Uniswap exchange, but not other ones).
|
|
@acolytec3 thanks for trying.. if we post another task that moves the token transfers to a diff uniswap wrapper library would that help? have you tried posting a github issue to ask the maintainer for help? https://github.com/shanefontaine/uniswap-python/issues |
Description
Pulls ERC-20 token balances associated with fee address and converts to ETH using Uniswap.
Notes:
Do-over of #4229 and #4383 (cuz my git skillz are lacking)
On Rinkeby testnet, Uniswap trading request only currently works for BAT Uniswap exchange but not OMG or MKR exchanges (using my on test account)
Refers/Fixes
Fixes #4150
Testing
Test included