SafeERC20: forward revert reasons instead of "low-level call failed"

[AyushKaul]

I'm using the crowdsale contract. I have deployed a detailedERC20 smart contract. When I send the ether to the crowdsale contract it shows me the following error:
SafeERC20: low-level call failed

When I use transfer() externally it works fine.

💻 Environment
Ganache-cli

[Skyge]

Ohh? Maybe you there is something wrong with the token address you providing.

[AyushKaul]

Are you talking about the ERC20 contract address?
I deployed the ERC-20 token first, then used the same address in the crowdsale contract. I'm positive that's the same.

[nventuro]

Hello @pushcodeveryday, thanks for reporting this!

Indeed, that error message isn't the best. @frangio and I thought about also including the original revert message, but we didn't get around to doing it yet.

Could you share the code for your crowdsale and token contracts? What function of SafeERC20 are you calling?

[AyushKaul]

@nventuro I'm using the openzeppelin crowdsale contract:
https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/crowdsale/Crowdsale.sol
I'm simply sending ether to this contract, So buyTokens(msg.sender) is called.
Token again openzeppelin DetailedERC20:
https://github.com/OpenZeppelin/openzeppelin-solidity/blob/master/contracts/token/ERC20/ERC20Detailed.sol

[nventuro]

Did you give tokens to the Crowdsale contract? The default buyTokens will call transfer on them, so it the contract has no tokens that call will revert.

[AyushKaul]

I deployed the crowdsale contract with the address having ERC20 tokens, I also passed the contract address at the time of deploying the crowdsale contract.

[nventuro]

I'm not sure I fully understand your setup, if you have a deployment script, could you share it?

To clarify, what you'd need to do is:

a) Deploy the token contract
b) Deploy the crowdsale contract, passing it the token contract's address
c) Give tokens to the crowdsale contract

[AyushKaul]

@nventuro , Just to be clear, You are saying that i should transfer erc20 tokens to the crowdsale contract?

[nventuro]

Correct: the base Crowdsale will deliver tokens to buyers by calling transfer. You can read more about it in our crowdsales guide.

[frangio]

We definitely should start forwarding the underlying reason string.

[AyushKaul]

Okay, I'll try that out.

[pengming273]

regarding step 3: c) Give tokens to the crowdsale contract, how to implement it?

[abcoathup]

Hi @pengming273

regarding step 3: c) Give tokens to the crowdsale contract, how to implement it?

The OpenZeppelin Contracts tests are great to see how to implement:
You just need to transfer the supply of the token to the crowdsale

openzeppelin-contracts/test/crowdsale/Crowdsale.test.js

Line 42 in cab26fd

await this.token.transfer(this.crowdsale.address, tokenSupply);

If you need more information on how to implement I suggest you post in the Community Forum: https://forum.openzeppelin.com

[AyushKaul]

@frangio, I agree that the error message is not intuitive. Refactoring the returned errors might be useful in cases like these.

[nventuro]

An issue that came up is that Solidity adds a special marker at the beginning of revert reasons, and this doesn't play nice when re-throwing reverts (i.e. there's some extra bytes at the beginning of the parsed revert reason). Perhaps the marker is being included twice? In any case, this needs some more looking-into.

[AyushKaul]

I'll also look into it.

[frangio]

Let's leave this issue open. We still think it needs to be fixed. The open PR #1943 is dealing with a similar issue with revert reasons.

[AyushKaul]

okay, but I don't see saferc20 being changed.

[frangio]

Once we've figured how to deal with this in the context of #1943 we can apply the same technique to SafeERC20.

[frangio]

Unfortunately the author of #1943 stopped working on it. Does someone want to pick it up and continue the work?

[frangio]

This was fixed in #2264 and it was released as part of 3.1.0!