search

AmbireTech / adex-protocol-eth

Ethereum implementation of the Ambire Protocol: Ambire Wallet contracts and AdEx payment channels
https://www.ambire.com
MIT License
50 stars 24 forks source link

Ethereal Hackathon Bounty: AdEx: Uniswap fork with SafeERC20 #63

Closed Ivshti closed 5 years ago

Ivshti commented 5 years ago

Challenge description:

Uniswap is revolutionary - it showed us that it's possible to build an efficient and user friendly exchange on Ethereum by making the right design decisions.

However, it's smart contracts were built in vyper - which is not a bad thing, but coupled with the a popular bug in many erc20 contracts, means it's not usable for a lot of 2017 erc20 tokens, including ADX

The challenge is to build a uniswap version in solidity (perhaps based on https://github.com/PhABC/uniswap-solidity) that uses our own https://github.com/AdExNetwork/adex-protocol-eth/blob/master/contracts/libs/SafeERC20.sol library

This would come in handy to exchange ADX to ETH and vice versa; it should be able to interoperate with Uniswap itself, probably by wrapping the factory+registry - using the solidity-based market contracts for buggy ERC20 tokens and using the uniswap markets for non-buggy

More links:

https://twitter.com/phabcd/status/1101672268189581312?s=21 https://github.com/PhABC/uniswap-solidity/issues/1 https://medium.com/coinmonks/missing-return-value-bug-at-least-130-tokens-affected-d67bf08521ca

Submission requirements:

Submission deadline:

Judging criteria:

Judging date:

Bounty:

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 300.0 DAI (300.0 USD @ $1.0/DAI) attached to it as part of the AdExNetwork fund.

ccashwell commented 5 years ago

@Ivshti, @haydenadams recommended is to simply wrap your ERC20 (and others like it) to return a bool for success. That's a much smaller task which accomplishes the seemingly primary goal of your request, probably with a much lower additional gas overhead. Is there a deeper reason for wanting to rewire everything in Solidity and make them interoperable?

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 1 week, 6 days from now. Please review their action plans below:

1) constantinkloecker has started work.

Hey, looking forward to port Uniswap from Vyper to Solidity

Learn more on the Gitcoin Issue Details page.

2) dannielboy58 has started work.

a port of the vyper smart contract to the solidity language by using the original erc20safe.sol libraries and communication with Uniswap to ensure good integration practices.

Learn more on the Gitcoin Issue Details page.

3) okwme has started work.

I will modify Philippe's solidity implementation to utilize SafeERC20 then write tests to confirm the functions work even when using AdEx's BadToken and WorstToken.

Learn more on the Gitcoin Issue Details page.

rmshea commented 5 years ago

feel free to use this channel to discuss, all! https://discord.gg/qj8A2DS

Ivshti commented 5 years ago

@ccashwell that doesn't work if the token is already on the mainnet, which is the case for many 2017 ERC20 tokens; AdEx in particular is, other than being held by many accounts, also traded on various centralized exchanges (binance, bittrex, etc.) which makes it even harder to migrate

It's easy enough to say "fix your bug" but that isn't so easy when so many things are interconnected on an immutable ledger

proxying/wrapping is also not an option, since users have to wrap/unwrap to trade on uniswap

As in why Solidity: no particular reason to choose solidity over vyper, it's just that there's already SafeERC20 in solidity and a solidity implementation of uniswap, so it seems the fastest way forward

haydenadams commented 5 years ago

@Ivshti

Here's the branch for no_bool tokens:

https://github.com/Uniswap/contracts-vyper/tree/no_bool_transfer

Although that version of the contract will only be safe on no_bool tokens and should not be used for tokens that return false instead of throwing on fail.

Ivshti commented 5 years ago

@haydenadams that's exactly the point of the SafeERC20 library - it handles every possible scenario and is well tested;

That means, if the token doesn't return a value (checked by returnvaluesize), it treats this as a success

If it does return a value, it throws on false

references: https://github.com/AdExNetwork/adex-protocol-eth/blob/master/test/TestSafeERC20.sol https://github.com/AdExNetwork/adex-protocol-eth/blob/master/contracts/libs/SafeERC20.sol

it's even adopted by aragon: https://github.com/aragon/aragonOS/blob/5c61d10ad6b20e64f662154cc456f451656a2618/contracts/common/SafeERC20.sol

btw we can also consider providing a bounty for anyone who implements safeerc20 (with the tests) on vyper

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 300.0 DAI (300.0 USD @ $1.0/DAI) has been submitted by:

  1. @okwme

@Ivshti please take a look at the submitted work:

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 300.0 DAI (300.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @okwme.