Simplify signatures of Host methods #154
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chfast
force-pushed
the
simplify-host
branch
from
335da8b
to
40b0886
Compare
|
ABI stays v6? |
I think there are still some changes. I will compare later on wit v5.0.0. |
gumb0
reviewed
Sep 10, 2018
gumb0
reviewed
Sep 10, 2018
chfast
force-pushed
the
simplify-host
branch
from
40b0886
to
4da1c68
Compare
Oh, you mean it should be v7? No, we bump it after the previous release if breaking changes are planned and it stays 6 until 6.0.0 is released. |
chfast
force-pushed
the
simplify-host
branch
from
4da1c68
to
3eb7b05
Compare
gumb0
approved these changes
Sep 13, 2018
axic
reviewed
Sep 17, 2018
| * @return The storage value at the given storage key or null bytes | ||
| * if the account does not exist. | ||
| */ | ||
| typedef evmc_bytes32 (*evmc_get_storage_fn)(struct evmc_context* context, |
There was a problem hiding this comment.
I'm not sold this change makes sense.
I can agree with the account lookups, but storage is different.
There was a problem hiding this comment.
I partly agree, but also this case is exotic at the moment.
I can do the following for now:
- extract this commit to a separate PR for a reference (not to be merge soon),
- add a comment to
evmc_get_storage_fnandevmc_set_storage_fnthat the account MUST exist.
axic
reviewed
Sep 17, 2018
| /** | ||
| * An attempt to modify storage of an non-existing account. | ||
| */ | ||
| EVMC_STORAGE_NON_EXISTING_ACCOUNT = 5 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts most of the Host method changes introduced recently.
Should I also change evmc_get_block_hash_fn?
I tried the previous API in Aleth and it is very messy. Here is an example of trying to expose the information if an account exists when checking balance: ethereum/aleth#5261
This information is not useful in any of the use cases (no place where you would not to fold it to 0 balance).
Reverts #146, #140, #135.