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

Ensure that proxies bubble up error messages #102

Open
spalladino opened this issue Apr 21, 2018 · 21 comments
Open

Ensure that proxies bubble up error messages #102

spalladino opened this issue Apr 21, 2018 · 21 comments
Labels
kind:research Research on a topic before implementing it topic:upgradeability Upgreadeability for both kernel and user contracts
Milestone

Comments

@spalladino
Copy link
Contributor

spalladino commented Apr 21, 2018

Solidity 0.4.22 introduced support for error reasons on revert EVM operations, check that proxies correctlly forward the error reason to their clients.

As an example, let's assume we have a logic contract named Reverter:

contract Reverter {
    function willRevert() public {
        require(false, "This is an error message from the contract");
    }
}

If you deploy this contract and attempt to call willRevert, you get the following message. Note that the message includes the revert reason This is an error message from the contract.

transact to Reverter.willRevert errored: VM error: revert.
revert The transaction has been reverted to the initial state.
Reason provided by the contract: "This is an error message from the contract".
Debug the transaction to get more information. 

Now, if you create a Proxy for Reverter via ZeppelinOS, and attempt to call willRevert on the proxy, the returned error message must include the This is an error message from the contract legend. Add a test in zos-lib to ensure it works like this, and make any necessary changes in Proxy if needed.

Depends on zeppelinos/zos-lib#162

@spalladino spalladino added kind:research Research on a topic before implementing it topic:upgradeability Upgreadeability for both kernel and user contracts labels Apr 21, 2018
@facuspagnuolo facuspagnuolo added this to the v2.0.0 milestone Jun 7, 2018
@gitcoinbot
Copy link

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


This issue now has a funding of 0.2 ETH (103.97 USD @ $519.84/ETH) attached to it.

@facuspagnuolo facuspagnuolo added the size:5 Size 5 label Jun 29, 2018
@spalladino
Copy link
Contributor Author

@yjkimjunior thanks for tackling this issue!! Note that the issue is not exactly what you're describing. I've just updated the description to better explain it. Please us me know if it's now clear, and if you're still willing to work on this after the clarification.

@spalladino spalladino added the gitcoin:assigned Under development via gitcoin label Jul 2, 2018
@pmespresso
Copy link

Thanks for the clarification, @spalladino, that makes sense. So make sure that error messages are retrieved/bubbled up from calls made to contracts through the proxies, right? I can modify/add the helper assertRevertWithErrMsg.js for this and assert error.message has a Reason provided by the contract: in the string.

@facuspagnuolo
Copy link
Contributor

Here are my two cents on that, we could add an optional argument to the current assertRevert expecting it to assert a revert with a custom message when given and assert a revert without a message when not.

@gitcoinbot
Copy link

@yjkimjunior Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@pmespresso
Copy link

Looks like this is blocked for now by these issues at Truffle/Ganache:

trufflesuite/truffle#976
trufflesuite/ganache#106

@gitcoinbot
Copy link

@yjkimjunior Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@pmespresso
Copy link

Still blocked by Trufflesuite/truffle#976

@spalladino spalladino added the status:blocked Blocked issue label Jul 19, 2018
@gitcoinbot
Copy link

@yjkimjunior Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

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


@yjkimjunior due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • warning (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@spm32
Copy link

spm32 commented Aug 22, 2018

Hey @yjkimjunior have you looked at trufflesuite/truffle#976 (comment)? It may actually be possible to get around that blocking issue.

@facuspagnuolo facuspagnuolo removed the size:5 Size 5 label Sep 12, 2018
@facuspagnuolo facuspagnuolo modified the milestones: v2.0.0, Backlog Sep 12, 2018
@smitrajput
Copy link

Hey @spalladino are you still looking to get this one fulfilled?

@spalladino
Copy link
Contributor Author

@smitrajput sure! It seems to require a new version of truffle to work, so we may not yet include it in zeppelinos/zos, but it'd be interesting to check if the assembly for delegating calls in the proxy work properly.

Not sure if @yjkimjunior is still working on it though. @yjkimjunior could you confirm if it's ok for @smitrajput to tackle this issue?

@pmespresso
Copy link

Hey all, sorry I've been busy with other things and haven't been working on this. @smitrajput feel free to take the baton!

@frankchen07
Copy link

hey @smitrajput - Frank from Gitcoin here - are you still working on this issue?

@smitrajput
Copy link

smitrajput commented Oct 25, 2018 via email

@frankchen07
Copy link

hey @spalladino - the issue is currently expired on Gitcoin - is it still open to the public?

@spalladino
Copy link
Contributor Author

Hey @frankchen07! I'm not sure about the bounty on gitcoin, but you're welcome to work on this if interested! Note that it may be the case that we don't actually need to code anything, and revert reasons are indeed working on the current implementation.

@spalladino spalladino removed the gitcoin:assigned Under development via gitcoin label Nov 12, 2018
@spalladino spalladino removed the status:blocked Blocked issue label Nov 12, 2018
@frankchen07
Copy link

hey @spalladino - I see. I'm on the Gitcoin team, and just checking in on the issue :). We'll leave it up in open status, but currently it's expired, so whoever is up next to help out on the issue may reach out!

@spalladino
Copy link
Contributor Author

Thanks @frankchen07! I wasn't aware that you were on the gitcoin team, thanks for all the help around here, and for checking in on this issue!

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Cancelled


The funding of 0.2 ETH (35.14 USD @ $175.71/ETH) attached to this issue has been cancelled by the bounty submitter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:research Research on a topic before implementing it topic:upgradeability Upgreadeability for both kernel and user contracts
Projects
None yet
Development

No branches or pull requests

7 participants