Any token pays access costs in MYB

[mybit-bot]

Introduction: DApps running on the MyBit Network require MYB to be burnt to use the applications. This creates a UX issue where users must manually convert their ERC20 tokens into MYB prior to using the dApps. To streamline user experience we are looking for a solution to be developed where the conversion to MYB is done on the back-end and the user can pay the access cost with any token in their wallet. This should be done in the most streamlined method - as few steps as possible.

We suggest researching Bancors quickConvert() functionality: https://support.bancor.network/hc/en-us/articles/360000878832-How-to-use-the-quickConvert-function

This implementation requires 1 smart contract, with the capability of transferring any ERC20 token to Bancor's contracts and receiving back MYB.

Resources: general - https://files.mybit.io/files/ MyBit SDK - developer.mybit.io

Requirements

1. Pay access costs in any token and have it converted to MYB and burnt on the back-end to enhance user experience.
2. Keep gas costs in mind when developing
3. Options other than Bancor, such as Kyber Network, may be more suitable, but MYB is already integrated with Bancor so please keep that in mind.
4. Should be built to be modular so it is easy to implement across any MyBit Network dApp.

To submit please initiate a pull request.

Disclaimer: All work completed via the MyBit Decentralised Development Fund is property of the MyBit Foundation (CHE-177.186.963) and may be used, re-used, and/or distributed, re-distributed by the MyBit Foundation for financial gain. The contributor holds no warrant nor claims for any future payments resulting from monetisation schemes originating from the MyBit Foundation or any of its affiliates. Where law permits outside of open source regulations, the contributor is not permitted to distribute any work completed via the MyBit Decentralised Development Fund (DDF) for personal gain.

[status-open-bounty]

Balance: 0 ETH Tokens: MYB: 110000.00 Contract address: 0x4980020169b71cea2862d9e095d119693ee06add Network: Mainnet Paid to: uivlis Visit https://openbounty.status.im to learn more.

[uivlis]

Is this a good idea?

pragma solidity 0.4.24;

import "https://raw.githubusercontent.com/bancorprotocol/contracts/master/solidity/contracts/converter/BancorConverter.sol";
import "https://raw.githubusercontent.com/bancorprotocol/contracts/master/solidity/contracts/token/ERC20Token.sol";
import "https://raw.githubusercontent.com/bancorprotocol/contracts/master/solidity/contracts/token/EtherToken.sol";
import "https://raw.githubusercontent.com/bancorprotocol/contracts/master/solidity/contracts/token/interfaces/IERC20Token.sol";

///@title A contract for converting any token into MYB (using Bancor's API)
///@author Vlad Silviu Farcas
contract TokenConversion {

    address bancorConverterAddress;   
    address myBitTokenContractAddress;
    BancorConverter converter; 

    ///@notice initialise addresses needed for conversion
    constructor(
        address _bancorConverterAddress, 
        address _myBitTokenContractAddress
        ) {
        bancorConverterAddress = _bancorConverterAddress;
        myBitTokenContractAddress = _myBitTokenContractAddress;
        converter = BancorConverter(bancorConverterAddress);
    }

    ///@notice convert some tokens
    ///@param _token the contract of the token that is about to be converted
    ///@param _amount the amount of _token that is about to be converted
    ///@param _minimumReturn the minimum return of MyBit token that is about to be received
    function convertTokenToMyBit(
        address _token, 
        uint _amount, 
        uint _minimumReturn
        ) external payable {
        IERC20Token token;
        uint amount = _amount;
        if (msg.value == 0){
            token = ERC20Token(_token);
            token.transferFrom(msg.sender, this, amount);
            token.approve(bancorConverterAddress, amount); 
        } else {
            token = new EtherToken();
            amount = msg.value;
        }

        uint convertedValue = converter.convert(
            token, 
            ERC20Token(myBitTokenContractAddress),
            amount,
            _minimumReturn
        );
        require (convertedValue >= _minimumReturn, "Transaction failed.");
        ERC20Token(myBitTokenContractAddress).transfer(msg.sender, convertedValue);
    }

}

[PeterMPhillips]

Hi @uivlis, thanks for the post. This looks like it could work. Have you done any testing on it?

[uivlis]

No, but I will try now that I have the heads up.

[PeterMPhillips]

ok, next step would be to create a github repository, initialize a truffle environment, and run some tests. post a link to the repository here and i'll clone it and play around with the contracts.

let me know if you have any questions!

[uivlis]

An compilable version is up at https://github.com/uivlis/token-to-mybit-converter

[mybit-bot]

➤ Jose Aguinaga commented:

Kyle Dewhurst please review the proposal and follow-up with Peter Phillips on whether we can integrate the code at its current state. If we can, request a Pull Request to integrate the code to pay out the bounty.

[mybit-bot]

➤ Peter Phillips commented:

I haven't done any testing yet. Not sure how to set up Bancor locally to test against it

[mybit-bot]

➤ Jose Aguinaga commented:

actually me neither, either we poke Kyle or poke around

[mybit-bot]

➤ Peter Phillips commented:

Yeah, I played around with it a bit and did some reading but no testing yet. I can work on it in the morning unless you have anything more pressing

[mybit-bot]

➤ Jose Aguinaga commented:

Sounds good!

[uivlis]

➤ Peter Phillips commented:

I haven't done any testing yet. Not sure how to set up Bancor locally to test against it

As per this https://www.reddit.com/r/Bancor/comments/8f68hx/testing_the_api/, the best way to do it is unfortunately to use low amounts of funds on mainnet.

[mybit-bot]

➤ Peter Phillips commented:

Huh. Ok I'll give it a go in the morning

[0xjjpa]

@uivlis Looking good! I think we need a way to see how we can integrate this with an existing contract or working application/demo. Pinging @kyledewy to also chip in on this to add a couple ideas on that area.

[PeterMPhillips]

While we're waiting for @kyledewy I'll try my best to formalize how I think this functionality can/should be implemented and we can discuss it here on GitHub for @uivlis to take part in.

I think there are two ways we can implement Bancor conversions into our apps:

Web3 way: The front-end can make a call to the Bancor API and get back the raw transaction that is needed to call one of their converter contracts -- API examples and raw tx examples. The user would convert their chosen token into MyB and then approve burning of their MyB.

This means no solidity contracts are needed. However, our service would then be dependent on Bancor servers to pass us raw transaction data and the end user would have to approve transactions for Bancor and MyBit. (MyBit twice in some cases since you have to approve the burn, then approve the main transaction).

Solidity way: When a MyBit dapp seeks burning approval, we can give them a selection of token options to choose from. They then give the ERC20Burner contract approval to withdraw their chosen token. When the burner is called by one of our functions, it looks up the user in our database, gets their token, does a price conversion, and withdraws the correct amount of the chosen token. The burner would then send the tokens to the TokenConverter. The TokenConverter would then convert and burn the tokens.

If we do it this way, the TokenConverter would need to implement some changes.

First, we can't assume that we will know which BancorConverter contract to call. We would need to call BancorNetwork.convert() and pass it a predefined conversion path.

Second, we would need to store the conversion path for each token that we support. This would have to be a mapping(address => IERC20Token[])

Third, this could be pretty gas intensive. We might need to implement functionality where the TokenConverter has approval from the burning contract to transfer tokens from the burner to the TokenConverter for conversion and burning. The converter would then have to be operated manually from an owner address.

This way would reduce the number of approvals a user would have to do, but it would increase gas cost. Also, we would still have to implement a pricing oracle to determine how many tokens to withdraw from the user.

Apologies for the wall of text. Please let me know if there are any questions or if I've just completely missed something. I'm still new to Bancor ;)

[uivlis]

@PeterMPhillips Which solution do you feel more inclined to use? Bancor recommends using web3 (https://www.reddit.com/r/Bancor/comments/8in9ve/convert_using_quickconvert_smart_contract_method/).

[PeterMPhillips]

Web3 is certainly more straightforward from a dev perspective but it has drawbacks from a end user and security perspective. @jjperezaguinaga @kyledewy what are your thoughts on this?

[uivlis]

Hey @mybit-bot, any updates or internal discussions on this one?

[0xjjpa]

@uivlis Yes, we are still seeing the best way to integrate it with our own contracts. Probably worth to have an online demo, or integrate directly in our Trust Contracts (https://github.com/MyBitFoundation/MyBit-Trust.tech). Which one would you prefer to test this on Mainnet?

[uivlis]

Oh ok, so the solidity path is still the vision to go ahead. In that case, I need to change the code according to @PeterMPhillips's recommendations from the wall above. I'll come up with an online demo then.

[0xdewy]

Hey sorry I've been busy with the hackathon. The web3 path is definitely simpler, but I don't see a way to have the received MYB tokens automatically burned, which would defeat the purpose UX wise. @PeterMPhillips any way you can think of doing this?

Otherwise the Solidity contract looks pretty good. I think it needs a few more checks to make sure it has actually received the MYB tokens. (ie.require( balanceAfter == balanceBefore + conversionAmount).

@uivlis It should probably be tested locally before you go burning all your main-net tokens. You can clone the Bancor repository. and run the tests they have setup:

npm install && npm test (make sure you have ganache-cli running on port 7545)

ganache-cli -p 7545

[uivlis]

@kyledewy made a few retouches for https://github.com/uivlis/token-to-mybit-converter, but didn't test it as you said (I'm just too eager to get this running). I feel 98% confident this would work, could you take a last minute look?

[0xdewy]

@uivlis I don't think the contract is ready. I believe the convert() function for Bancor doesn't take a token path directly, but just requires a fromToken, and a toToken, like you had before. Also I made a comment about the token balance check on your github repo.

[uivlis]

I just addressed your comment, can you see if it's fine now? Also,

function convert(IERC20Token[] _path, uint256 _amount, uint256 _minReturn) public payable returns (uint256) {
        return convertFor(_path, _amount, _minReturn, msg.sender);
    }

(snippet from BancorNetwork.sol)

[0xdewy]

Ok can you find a link to the live contracts? I'm seeing this function from the contracts in master branch.

    function convert(IERC20Token _fromToken, IERC20Token _toToken, uint256 _amount, uint256 _minReturn) public returns (uint256) {
        convertPath = [_fromToken, token, _toToken];
        return quickConvert(convertPath, _amount, _minReturn);
    }

[uivlis]

https://etherscan.io/address/0xF20b9e713A33F61fA38792d2aFaF1cD30339126A#code

function convert(IERC20Token[] _path, uint256 _amount, uint256 _minReturn) public payable returns (uint256) {
        return convertFor(_path, _amount, _minReturn, msg.sender);
    }

Are you reading from BancorNetwork.sol, and not from BancorConverter.sol?

[0xdewy]

Ok when i go to use Bancor I'm directed to this contract. The convert function there doesn't appear to receive the path:

convert(IERC20Token _fromToken, IERC20Token _toToken, uint256 _amount, uint256 _minReturn) public returns (uint256);

I'm not sure if Bancor has a way of verifying the address of their current contracts. Can you confirm that Bancor directs you to the same contract? If so, then this should be the version we work with.

It's going to need to be tested locally. You can customize the tests Bancor has written and once you confirm it works the bounty is finished.

[PeterMPhillips]

Different conversions use different converters. @kyledewy, the address you list is for Ether to ERC20 conversions. However, if a convert MyB to Storm i get this address: 0x6B431a7a99780819473722161Ee9145E5649C5e2

I believe it works like this: Each erc20 token has a Smart Token as it relayer. Each relayer token is owned by BancorConverter contract (notice how a BancorConvert is also a SmartTokenController contract). The relayers hold reserves of their connector tokens, e.g. MyBit and Bancor. When you do a conversion, you take the following path: MyBit -> MYBBNT relayer -> Bancor -> STORMBNT relayer -> Storm.

So if you want to quickConvert using a BancorConverter contract, you need to know which converter is needed for each conversion

[0xdewy]

Yaa that makes sense. So it's BancorNetwork.sol it should be calling. Is there a getter function to get the converter address given the token address?

[PeterMPhillips]

i don't think its needed, if you look at BancorNetwork.sol they in their comments "The smart token is only used as a pointer to a converter (since converter addresses are more likely to change)" and in their tests, they just set the path like so: smartToken2BuyPath = [etherToken.address, smartToken1.address, smartToken1.address, smartToken2.address, smartToken2.address];

[PeterMPhillips]

Just a follow up, in BancorNetwork.sol you can see where they get the converter contract in converByPath(). On line 321, converter = IBancorConverter(smartToken.owner());

[uivlis]

Well, this is embarassing, but I keep getting "Error: VM Exception while processing transaction: revert", although I think the test evironment is to blame. In tests, instead of BancorNetwrok.js, I have:

const ethUtil = require('ethereumjs-util');
const web3Utils = require('web3-utils');
const TokenConverter = artifacts.require('TokenConverter.sol');

let etherToken;
let smartToken1;

let defaultGasPriceLimit = BancorGasPriceLimit.class_defaults.gasPrice;
let tokenConverter;
function sign(msgToSign, signerAddress) {

smartToken2SellPath = [smartToken2.address, smartToken2.address, smartToken1.address, smartToken1.address, etherToken.address];

        tokenConverter = await TokenConverter.new(bancorNetwork.address);
    });

    it('verifies that convert function in TokenConverter works', async () => {
        let whitelist = await Whitelist.new();
        await whitelist.addAddress(accounts[0]);
        await converter1.setConversionWhitelist(whitelist.address);
        let balanceBeforeTransfer = await smartToken1.balanceOf.call(accounts[0]);
        await tokenConverter.convertTokenToMyBit(smartToken2.address, 10000, 1, smartToken1.address, etherToken.address, { from: accounts[0], value: 0});
        let balanceAfterTransfer = await smartToken1.balanceOf.call(accounts[0]);
        console.log(balanceAfterTransfer);
        assert.isBelow(balanceAfterTransfer.toNumber(), balanceBeforeTransfer.toNumber(), 'amount transfered');
    });

    it('verifies that quick buy with a single converter results in increased balance for the buyer', async () => {

with a modified TokenConverter.sol for the purpose of testing:

///@notice initialise addresses needed for conversion
    constructor(BancorNetwork _bancorNetwork) {
        bancorNetwork = _bancorNetwork;
    }

    ///@notice convert some tokens
    ///@param _token the contract of the token that is about to be converted
    ///@param _amount the amount of _token that is about to be converted
    ///@param _minimumReturn the minimum return of MyBit token that is about to be received
    function convertTokenToMyBit(
        address _token, 
        uint _amount, 
        uint _minimumReturn,
        address _toToken,
        address _ether
        ) payable {
        IERC20Token token;
        IERC20Token[] memory path = new IERC20Token[](3);
        uint amount = _amount;
        uint value = 0;
        ISmartToken toToken = SmartToken(_toToken);
        if (msg.value == 0){
            require(bancorNetwork.etherTokens(_token) == true, "Token not supported");
            token = SmartToken(_token);
            token.approve(bancorNetwork, amount); 
        } else {
            token = EtherToken(_ether);
            amount = msg.value;
            value = msg.value;
        }
        path[0] = token;
        path[1] = toToken;
        path[2] = toToken;
        uint convertedValue = bancorNetwork.convert.value(value)(
            path,
            amount,
            _minimumReturn
        );
        require (convertedValue >= _minimumReturn, "Transaction failed.");
        require (toToken.balanceOf(this) == convertedValue, "Transaction failed with different return than expected");
        require (toToken.balanceOf(this) <= amount, "Transaction failed without conversion");
        toToken.transfer(msg.sender, convertedValue);
        token.transfer(msg.sender, token.balanceOf(this));
    }

I'm stuck, would you give me some ideas where to look?

[0xdewy]

Hey sorry @uivlis, we don't have the time to clone and debug errors. The revert error is likely from one of those require() statements not being met.

[uivlis]

Thank you @kyledewy for your kind help. Unfortunately, that was not the problem. The tests have somehow succeeded. It seems that in order for a contract on the blockchain to interact with bancorConverter it needs to be whitelisted by the owner of the converter (i.e., by the Bancor team) through the setConversionWhitelist() function. BancorNetwork.sol, line 322:

 checkWhitelist(converter, _for, features);

Also, if that happens, my solution needs to pass through BNT in order to convert to MYB, since there is no converter from, say, ETH to MYB, directly.

[0xdewy]

Ok thats great @uivlis Could you make a pull-request against the MyBit-Network.tech repo on branch baseline with the contract and the test in there? You can call the contract BancorConverter.sol and the test bancorConverter.js. Me and @PeterMPhillips will look at it.

[uivlis]

I did so, however, the files won't work unless you clone bancorprotocol and copy paste them in their appropriate place. The final version is still to be found here. And, don't forget, as I mentioned above, you need the Bancor team to whitelist the TokenConverter (once deployed).

[PeterMPhillips]

As a note, I think we can safely assume that all the converters don't have whitelists set as that would break the usability of their product. For example, if we go to their ethereum/bancor converter you can see that no whitelist is set:

31. conversionWhitelist 0x0000000000000000000000000000000000000000 address

[uivlis]

Oh, okay. So, what's next for me to do, or that's the end of the bounty?

[0xdewy]

@uivlis getting close. The tests are failing though. It seems one of the contract paths are missing. Can you fix it and commit again so the circleCI tests pass ?

[uivlis]

I did so, however, the files won't work unless you clone bancorprotocol and copy paste them in their appropriate place. The final version is still to be found here.

Sadly, I cannot make CircleCI pass... Pardon, but I think I shouldn't, since you shouldn't merge my pull-request, since it was intended to provide you with the tests of the real solution. Now the those tests will work only in bancorprotocol repository, as you requested of me. Instead, shouldn't I make a pull request with this?

[0xdewy]

Yes, but these contracts are going into our SDK so they need to be merged in with all our other contracts and tests. You can import the relevant Bancor contracts so that your tests will pass.

[uivlis]

I'm feeling stupid now, but maybe I miss something. The relevant contracts are just above 90% of all contracts and their artifact builds (due to interdependence) plus some npm packages specific to Bancor Contracts. One cannot just import them all. Again, why would you want to merge it into the SDK, since its only purpose was a hack? Don't get me wrong, but I just don't get it. You were thinking of importing them via https?

[0xdewy]

One can import them all, and we need to, in order to test this locally. This is a hack for now, but it is going to be used by many different people so it has to be properly tested. Just bring in the contracts locally and those npm packages can be added as dependencies.

[uivlis]

Okay, so here's what I did: I put the entire modified bancorprotocol repository as a commit (with a README). Well, at least the checks pass now.

[0xdewy]

@uivlis sorry I don't understand what you did there. It seems you've just deleted the contracts and tests?

[uivlis]

Oh my, that's a big one for me... Big apologies, apparently it was a git folder inside another git folder. I had to delete the .git files from bancor-contracts to make visible the files commited. Hope this commit passes.

[0xdewy]

Ok. I need more reference of what tests you are running for the TokenConverter contract. Is it BancorNetwork.js? If so you only need to include that test, and the 11 contracts it uses. You can put the TokenConverter contract in the ecosystem folder and the BancorNetwork.js test in tests. Keep the necessary bancor contracts in the bancor folder and adjust the relative paths.

[uivlis]

Is it good now?

[0xdewy]

BancorNetwork.js needs to be in the tests folder outside of the contract tests. All the excess files in the bancor folder can be removed. If you can just keep the contracts used in BancorNetwork.js and TokenConverter.sol that would be great. The tests aren't compiling for me. Did you successfully run them?

[uivlis]

The tests aren't compiling for me. Did you successfully run them?

The TokenConverter test is running. For a reason or another, it might interfere with the others. If you put it at the end, all the other tests pass. I saw that the tests interacting with the same tokens as in the TokenConverter test don't pass (coincidentally, the second test is just one of that). I thought it had to do with the converter supply and the modified price after the first conversion, but I did not investigate further. I should see...