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

accounts/abi/bind, eth: add contract non-existent error #2496

Merged
merged 1 commit into from
Apr 27, 2016

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Apr 27, 2016

This PR adds a non-existent contract error message to the eth API's call and estimateGas methods to properly distinguish this specific error message. It further adds this error to the Go ABI binding simulated backend as well as implements detecting this in the remote backend too.

This PR is specifically important to make it both obvious if a native dapp call fails due to a bad contract address and/or not being in sync, but also so that we can programatically gracefully handle these scenarios (i.e. don't crash, just postpone the call (e.g. versions check) until the contract becomes available).

Edit: Long term we probably would want to assign a JSON RPC error code specifically to this failure so we don't have to rely on the error string contents to convert the textual failure into our specific internal Go failure type.

@robotally
Copy link

robotally commented Apr 27, 2016

Vote Count Reviewers
👍 0
👎 0

Updated: Wed Apr 27 14:24:06 UTC 2016

@karalabe karalabe added this to the 1.4.1 milestone Apr 27, 2016
@karalabe
Copy link
Member Author

@fjl Please review

// If there's no code to interact with, respond with an appropriate error
if args.To != nil {
if code := stateDb.GetCode(*args.To); len(code) == 0 {
return "0x", nil, bind.ErrNonExistentContract
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the cross-dependency on accounts/abi/bind. Maybe the error can live in package eth instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had that originally, but I need to return this same error from bind/backends, which would create a dependency on eth and then I couldn't any more use native daps within eth due to circular dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, perhaps it's still possible since eth wouldn't depend on the backends package, only on bind. But the tests would still need to go then into eth_test to be able to pull in the simulator (or we don't test the contract integration :P)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all it seemed a bit large of a dependency to pull in the entire eth protocol just to handle a 3 word error string.

@fjl
Copy link
Contributor

fjl commented Apr 27, 2016

Maybe the error should be "no code" instead, given that it checks for that. Contracts with empty code can exist and would trigger the error otherwise.

@karalabe
Copy link
Member Author

// ErrNoContractCode is returned by call and transact operations for which the
// requested recipient contract to operate on does not exist in the state db or
// does not have any code associated with it (i.e. suicided).
var ErrNoContractCode = errors.New("no contract code")

like this?

@fjl
Copy link
Contributor

fjl commented Apr 27, 2016

would prefer ErrNoCode but that's a subjective thing.

@karalabe karalabe force-pushed the abibind-missing-contract-error branch from b810006 to d30ba7b Compare April 27, 2016 14:13
@karalabe karalabe force-pushed the abibind-missing-contract-error branch from d30ba7b to cdcbb2f Compare April 27, 2016 14:15
@karalabe
Copy link
Member Author

@fjl PTAL

@fjl
Copy link
Contributor

fjl commented Apr 27, 2016

👍 for now, but we should really add the error code

@karalabe
Copy link
Member Author

I agree. Looked at it but it would overlap with som changes Bas is making currently (like renaming RPCError to Error) and other parts of the code. Also it would require us defining some proper application level error codes and messages, which is not a quick fix. I'm all in for doing that, but that can wait for 1.5.

@codecov-io
Copy link

Current coverage is 50.84%

Merging #2496 into develop will decrease coverage by -0.00%

@@            develop      #2496   diff @@
==========================================
  Files           209        209          
  Lines         29521      29525     +4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits          15019      15010     -9   
- Misses        13396      13408    +12   
- Partials       1106       1107     +1   
  1. 3 files (not in diff) in p2p were modified. more
    • Misses +8
    • Partials +1
    • Hits -9
  2. File accounts/watch.go (not in diff) was modified. more
    • Misses -2
    • Partials -1
    • Hits +3
  3. File ...saction_test_util.go (not in diff) was modified. more
    • Misses +1
    • Partials 0
    • Hits -1
  4. File ...loader/downloader.go (not in diff) was modified. more
    • Misses +1
    • Partials +1
    • Hits -2

Sunburst

Powered by Codecov. Last updated by b810006

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

Successfully merging this pull request may close these issues.

5 participants