Add return code to get_block_hash_fn #73
Conversation
| typedef void (*evmc_get_block_hash_fn)(struct evmc_uint256be* result, | ||
| struct evmc_context* context, | ||
| int64_t number); | ||
| typedef int (*evmc_get_block_hash_fn)(struct evmc_uint256be* result, |
There was a problem hiding this comment.
Should it return evmc_status_code (https://github.com/ethereum/evmc/blob/master/include/evmc/evmc.h#L191)?
There was a problem hiding this comment.
I don't think so, but we could consider having another enum for more regular cases.
Success/failure/invalid args.
There was a problem hiding this comment.
@chfast do we want an enum instead?
There was a problem hiding this comment.
I don't know yet. I have the review all similar cases and create the superset of possible error codes. If we end up with up to 4 items we can use it in all cases.
There was a problem hiding this comment.
We can merge it with int, but you have to improve documentation of @result and @param number.
There was a problem hiding this comment.
Should we wait for #116 first?
There was a problem hiding this comment.
The #116 is ready, just needs review.
|
Quick analysis in #117, the Should I take it over? |
|
Still waiting for an answer on |
bindings/go/evmc/host.go
Outdated
| r := C.int(0) | ||
| if blockhash != (common.Hash{}) { | ||
| // All zeroes hash is considered a failure in lookup. | ||
| // FIXME: should we instead adjust `ctx.GetBlockHash`? |
There was a problem hiding this comment.
I think this is the main question. Leaning towards this.
There was a problem hiding this comment.
@chfast if you think that's a better idea, can you please push a new commit which changes it? (just leave the current one as it is)
There was a problem hiding this comment.
This should be changed to blockhash, err := ctx.GetBlockHash(number). I can push the change later on.
axic
changed the title
include/evmc/evmc.h
Outdated
| @@ -160,10 +160,11 @@ typedef struct evmc_tx_context (*evmc_get_tx_context_fn)(struct evmc_context* co | |||
| * @param context The pointer to the Host execution context. | |||
| * @param number The block number. Must be a value between | |||
| * (and including) 0 and 255. | |||
| * @return 1 if exists, 0 otherwise. | |||
There was a problem hiding this comment.
This comment is wrong. It should be something like 1 if the information is available, 0 otherwise or something better.
Also:
- The comment for
numberis wrong. In this case leave "The block number" only and explain when the information is available in the main description. - In the
resultexplain that result is written only if the function returns 1 i.e. when information is available.
bindings/go/evmc/host.go
Outdated
| r := C.int(0) | ||
| if blockhash != (common.Hash{}) { | ||
| // All zeroes hash is considered a failure in lookup. | ||
| // FIXME: should we instead adjust `ctx.GetBlockHash`? |
There was a problem hiding this comment.
This should be changed to blockhash, err := ctx.GetBlockHash(number). I can push the change later on.
I do the first fix, but you should do this one. Will let you know when I pushed my fix. |
|
@chfast updated, you can take it over now. |
Fixes #63.