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

Create ERC20 token out of facet #9

Open
Walodja1987 opened this issue Jan 30, 2021 · 17 comments
Open

Create ERC20 token out of facet #9

Walodja1987 opened this issue Jan 30, 2021 · 17 comments

Comments

@Walodja1987
Copy link

Hi everyone,

I am struggling to create an ERC20 token inside a facet contract. In other words, the following statement is failing (it throws an VM exception error):

ERC20 erc20Token = new ERC20("TestToken","TEST");

I have the suspicion that this operation doesn't work when a delegate call is used. Could someone share an example how to do this operation insde a facet contract?

Best
Wladimir

@mudgen
Copy link
Owner

mudgen commented Jan 31, 2021

Hey @Walodja1987, thanks for making an issue about this. Let's get to the bottom of it.

It should work fine in a facet. delegatecall is okay for this.

Where is the source code for the ERC20 contract? What is the full error message and stack trace?

Thanks
Nick

@Walodja1987
Copy link
Author

Walodja1987 commented Jan 31, 2021

Hi Nick (@mudgen),

I really appreciate your help on this issue. I have spent many hours trying to solve it, but without success. You are my last hope :-)

I have simplified the example, see below the three relevant code snippets. The error message is "Error: Returned error: VM Exception while processing transaction: revert".

Many thanks again!

OptionFacet.sol:

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;
pragma experimental ABIEncoderV2;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";
import "@openzeppelin/contracts/math/SafeMath.sol";
import "@openzeppelin/contracts/math/SignedSafeMath.sol";
import "../DirectionToken.sol";
import "../libraries/LibDiamond.sol";
import "../SafeDecimalMath.sol";

contract OptionFacet {
     function createToken() public returns(bool done_) {
                DirectionToken shortToken = new DirectionToken("TestToken", "TEST", 1, false);
                done_ = true;
            }
}

DirectionToken.sol:

// SPDX-License-Identifier: MIT
pragma solidity ^0.7.6;

import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract DirectionToken is Ownable, ERC20 {
    bool public isLong;
    uint256 public optionId;
    constructor( string memory _name, string memory _symbol, uint256 _optionId, bool _isLong) ERC20(_name, _symbol) public {
        optionId = _optionId;
        isLong = _isLong;
    }
    
    function mintAndReturnToken(uint256 nbrToken) external onlyOwner returns(bool) {
        _mint(tx.origin, nbrToken);
        return true;
    }
    
    function redeemAndBurnToken(address redeemer, uint256 nbrToken) external onlyOwner returns(bool) {
        _burn(redeemer, nbrToken);
        return true;
    }
}

test.js (for truffle test):

const Diamond = artifacts.require('Diamond')
const DiamondCutFacet = artifacts.require('DiamondCutFacet')
const DiamondLoupeFacet = artifacts.require('DiamondLoupeFacet')
const OwnershipFacet = artifacts.require('OwnershipFacet')
const OptionFacet = artifacts.require('OptionFacet')


const ERC20CollateralToken = artifacts.require("ERC20CollateralToken");

const FacetCutAction = {
  Add: 0,
  Replace: 1,
  Remove: 2
}

function getSelectors (contract) {
  const selectors = contract.abi.reduce((acc, val) => {
    if (val.type === 'function') {
      acc.push(val.signature)
      return acc
    } else {
      return acc
    }
  }, [])
  return selectors
}

function removeItem (array, item) {
  array.splice(array.indexOf(item), 1)
  return array
}

function findPositionInFacets (facetAddress, facets) {
  for (let i = 0; i < facets.length; i++) {
    if (facets[i].facetAddress === facetAddress) {
      return i
    }
  }
}

contract('DiamondTest', async (accounts) => {
    let diamondCutFacet
    let diamondLoupeFacet
    let optionFacet
    // eslint-disable-next-line no-unused-vars
    let ownershipFacet
    let diamond
    let result
    let addresses = []
    let erc20CollateralToken

    const zeroAddress = '0x0000000000000000000000000000000000000000'

    before(async () => {
        
      diamond = await Diamond.deployed()
        diamondCutFacet = new web3.eth.Contract(DiamondCutFacet.abi, diamond.address)
        diamondLoupeFacet = new web3.eth.Contract(DiamondLoupeFacet.abi, diamond.address)
        optionFacet = new web3.eth.Contract(OptionFacet.abi, diamond.address)
        // unfortunately this is done for the side affect of making selectors available in the ABI of
        // OwnershipFacet
        // eslint-disable-next-line no-unused-vars
        ownershipFacet = new web3.eth.Contract(OwnershipFacet.abi, diamond.address)
        
        erc20CollateralToken = await ERC20CollateralToken.deployed("DummyCollateralToken", "DCT");
    })

    **it('creates a token', async () => {

    
      const ok_ = await optionFacet.methods
              .createToken()
              .send({from: accounts[1]})
      assert(ok_);
    });**




})

@Walodja1987
Copy link
Author

Anyone who could help with my issue? Unfortunately, there are not that many projects yet that implement the diamond standard that I could use as a potential reference example for my issue :-(

@mudgen
Copy link
Owner

mudgen commented Feb 7, 2021

Hey, sorry, looking now.

@mudgen
Copy link
Owner

mudgen commented Feb 7, 2021

@Walodja1987 thanks for this. Where's your migration script? For example like this: https://github.com/mudgen/diamond-1/blob/master/migrations/2_diamond.js

@Walodja1987
Copy link
Author

@mudgen I am so glad you replied. Really appreciate your help on this.

Here the 2_diamond.js file. It's the standard file with the OptionFacet included.

const Diamond = artifacts.require('Diamond')
const DiamondCutFacet = artifacts.require('DiamondCutFacet')
const DiamondLoupeFacet = artifacts.require('DiamondLoupeFacet')
const OwnershipFacet = artifacts.require('OwnershipFacet')
const OptionFacet = artifacts.require('OptionFacet.sol')

const FacetCutAction = {
  Add: 0,
  Replace: 1,
  Remove: 2
}

function getSelectors (contract) {
  const selectors = contract.abi.reduce((acc, val) => {
    if (val.type === 'function') {
      acc.push(val.signature)
      return acc
    } else {
      return acc
    }
  }, [])
  return selectors
}

module.exports = function (deployer, network, accounts) {

  deployer.deploy(DiamondCutFacet)
  deployer.deploy(DiamondLoupeFacet)
  deployer.deploy(OwnershipFacet)
  deployer.deploy(OptionFacet).then(() => {
    
    const diamondCut = [
      [DiamondCutFacet.address, FacetCutAction.Add, getSelectors(DiamondCutFacet)],
      [DiamondLoupeFacet.address, FacetCutAction.Add, getSelectors(DiamondLoupeFacet)],
      [OwnershipFacet.address, FacetCutAction.Add, getSelectors(OwnershipFacet)],
      [OptionFacet.address, FacetCutAction.Add, getSelectors(OptionFacet)]
    ]    
    return deployer.deploy(Diamond, diamondCut, [accounts[0]])
  })
}

Just for completeness as this is in the test.js file (although you don't really need it for this particular case), here the separate ERC20CollateralToken migration script which I named 3_others.js:

const ERC20 = artifacts.require("ERC20CollateralToken");

module.exports = async function (deployer, network, accounts) {
  //Deploy contracts
  await deployer.deploy(ERC20,"DummyCollateralToken", "DCT", {from: accounts[1]});

  //Get pointer to deplyed instances of contract
  const erc20CollateralToken = await ERC20.deployed();
  console.log("ER20 CollateralToken address:" + erc20CollateralToken.address);
  const erc20CollateralOwner = await erc20CollateralToken.owner();
  console.log("ER20 CollateralToken owner: " + erc20CollateralOwner);
  console.log("Account 1: " + accounts[1])
};

Many thanks for your help again!

@mudgen
Copy link
Owner

mudgen commented Feb 7, 2021

@Walodja1987 Thank you.

One thing that stuck out at me is this line: const OptionFacet = artifacts.require('OptionFacet.sol')
Should it instead be: const OptionFacet = artifacts.require('OptionFacet') ?

If you add another function to OptionFacet and can call it, does it work?

Thanks, Nick

@divaprotocol
Copy link

divaprotocol commented Feb 8, 2021

@mudgen good point about OptionFacet.sol. I fixed that but that didn't seem to cause the issue. I was actually able to fix the above issue in that particular example by replacing .send() with .call() in my test file resulting in the following updated test:

it('creates a token', async () => {
      const ok_ = await optionFacet.methods
              .createToken()
              .call({from: accounts[1]})
      assert(ok_);
    });

However, in my actual code, the ERC20 creation is not part of a view function but of a function that also updates storage variables. So I added a function (setValue) that sets the value of a storage variable b and included the DirectionToken/ERC20 token creation part:

OptionFacet.sol (incl. additional functions):

contract OptionFacet {

            uint256 b;

            function createToken() public returns(bool done_) {
                DirectionToken shortToken = new DirectionToken("TestToken", "TEST", 1, false);
                done_ = true;
            }

            function setValue(uint256 b_) public {                
                DirectionToken shortToken = new DirectionToken("TestToken2", "TEST2", 1, false);
                b=b_;
            }

            function getB() public view returns(uint256) {
                return b;
            }

}

In my test.js file I added the following test:

it('sets value', async () => {
      await optionFacet.methods.setValue(6).send({from: accounts[1], gas: 1000000})
      result = await optionFacet.methods.getB().call();
      assert.equal(result,6);
    });

Interestingly, the test fails with VM Exception error when the DirectionToken part is included in the setValue function. However, when I drop this part / comment it out, then the test goes through.

Conclusion: It looks like creating the ERC20 token within a function that updates variables doesn't work.

@mudgen
Copy link
Owner

mudgen commented Feb 8, 2021

Great you are making progress. It does work, but you can't use state variables declared like b in proxy contracts and diamonds. You need to use diamond storage or AppStorage or other state variable layout strategy.

Here's some links about these:

Understanding Diamonds on Ethereum
https://dev.to/mudgen/understanding-diamonds-on-ethereum-1fb

How Diamond Storage Works
https://dev.to/mudgen/how-diamond-storage-works-90e

AppStorage Pattern for State Variables in Solidity
https://dev.to/mudgen/appstorage-pattern-for-state-variables-in-solidity-3lki

@divaprotocol
Copy link

@mudgen I am aware that I should declarestate variables like b differently when using the diamond struct. However, it still works and I did it like that to demonstrate the issue. The issue is not b as the setValue function works as long as the DirectionToken part is not included. As soon as I include the DirectionToken part, the code fails.

Thanks for sharing the links but unfortunately, I was not able to find a solution to my issue. :-(

@mudgen
Copy link
Owner

mudgen commented Feb 21, 2021

@divaprotocol Okay, I'm not sure what the problem is but it is possible to create new contracts (include ERC20) from within facets in diamonds. It is done successfully here: https://github.com/aavegotchi/aavegotchi-contracts/blob/cbb90bce8a1ef047b376ce7e388ca1c81ac48e00/contracts/Aavegotchi/facets/AavegotchiFacet.sol#L235

I suggest simplifying your code as much as possible to help determine the cause of the problem.

@divaprotocol
Copy link

Thanks for your prompt response. I will have a look at the aavegotchi example. Many thanks!

@mudgen
Copy link
Owner

mudgen commented Feb 21, 2021

@divaprotocol Yes, maybe try creating a new but very simple contract that isn't ERC20 and see if that works.

@divaprotocol
Copy link

Hi @mudgen,

after several hours of testing, we were able to narrow down the error. It looks like there is a limit on how many functions one can use in a contract. We can't explain this behavior and hope for your help.

Let me explain what we did :
In our OptionFacet.sol contract (that I shared above) we import the DirectionToken.sol contract which inherits all the ERC20 functions plus has 2 additional functions defined. To use a simplified example, we replaced the DirectionToken contract with the below a set of simple functions (see below). We noticed that if you include 20 functions + 1 variable, then then it doesn't work, throwing a VM exception error. However, if you exclude returnsTrue7 function, it will work again. It sounds to me like a stack too deep error but the error doesn't say so.

Would you know what could cause this issue? If there is a limitation on the number of functions in a contract, we would need to split the openzeppelin's ERC20 contract to make it work which I think is not an option.

Really appreciate your help on this strange issue.

pragma solidity ^0.7.6;
contract DirectionToken {
    
    string _name;

    constructor()  public {
        _name = "Name";
    }
    
    function name() public view returns (string memory) {
        return _name;
    }

    function name2() public view returns (string memory) {
        return _name;
    }

    function name3() public view returns (string memory) {
        return _name;
    }

    function name4() public view returns (string memory) {
        return _name;
    }

    function name5() public view returns (string memory) {
        return _name;
    }

    function name6() public view returns (string memory) {
        return _name;
    }

    function name7() public view returns (string memory) {
        return _name;
    }

    function name8() public view returns (string memory) {
        return _name;
    }

    function name9() public view returns (string memory) {
        return _name;
    }

    function name10() public view returns (string memory) {
        return _name;
    }

    function name11() public view returns (string memory) {
        return _name;
    }

    function name12() public view returns (string memory) {
        return _name;
    }
 
    function name13() public view returns (string memory) {
        return _name;
    }
    
    function returnsTrue1() external pure returns(bool) {
        return true;
    }

    function returnsTrue2() external pure returns(bool) {
        return true;
    }
    
    function returnsTrue3() external pure returns(bool) {
        return true;
    }
    function returnsTrue4() external pure returns(bool) {
        return true;
    }
    function returnsTrue5() external pure returns(bool) {
        return true;
    }
    
    function returnsTrue6() external pure returns(bool) {
        return true;
    }
    
    /* If you take out this function, it will work again */
    function returnsTrue7() external pure returns(bool) {
        return true;
    }

}

@mudgen
Copy link
Owner

mudgen commented Feb 23, 2021

@divaprotocol There isn't a maximum number of functions you can have. But there is a maximum number of bytes the bytecode of your contract can be which is about 24KB.

The DirectionToken contract is not large enough to hit the 24KB contract size limit by itself. But since you are creating the contract from within the OptionFacet facet the DirectionToken contract bytecode is probably being added to the OptionFacet contract bytecode and that is hitting the maximum size. You can fix this by moving some of the functions or functionality in OptionFacet to another facet. You can test this by measuring the bytecode of the contracts to see if they hit the 24KB limit.

@divaprotocol
Copy link

@mudgen Thanks for getting back. If it was exceeding the 24KB limit, shouldn't it say it in the error message? We had this issue already in the past and that was the main reason why we moved to the diamond standard.

We are using the contract-size plugin by truffle to determine the contract size. Is that a good tool or would you recommend using something else?

@divaprotocol
Copy link

@mudgen To test whether it's a 24KB error, I duplicated the OptionFacet contract within the OptionFacet.sol file and named it OptionFacet2 (same for all the functions in the duplicate). I did this just to increase the size of the contract. If I remove the one function in the DirectionToken that causes the problem, it still runs through. This suggests that it's probably not a 24KB error. It seems to be specific to the one function in the DirectionToken contract. We are really stuck :-(

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

No branches or pull requests

2 participants