-
Notifications
You must be signed in to change notification settings - Fork 310
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
Storage status #52
Storage status #52
Conversation
size_t data_size, | ||
const struct evmc_uint256be topics[], | ||
size_t topics_count) | ||
static void emit_log(struct evmc_context* context, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack this change. Needs an ABI bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ABI is bumped in this PR already, but that's not needed in this case, because this is a static function in a example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right I thought it was renaming the member, but that was renamed before.
include/evmc/evmc.h
Outdated
* @param address The address of the contract. | ||
* @param key The index of the storage entry. | ||
* @param value The value to be stored. | ||
* @return The effect on the storage item. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should have @see ::evmc_storage_status
include/evmc/evmc.h
Outdated
EVMC_STORAGE_UNCHANGED = 0, /**< The storage item value unchanged. */ | ||
EVMC_STORAGE_MODIFIED = 1, /**< The storage item value modified. */ | ||
EVMC_STORAGE_ADDED = 2, /**< The storage item added. */ | ||
EVMC_STORAGE_DELETED = 3, /**< The storage item deleted. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this needs to expose special cases about 0?
Adding explanation above may be good. We have the following cases, please map them to codes:
- zero -> zero (UNCHANGED, because value is always the same)
- zero -> non-zero (ADDED)
- non-zero -> zero (DELETED)
- non-zero -> non-zero (same value: UNCHANGED)
- non-zero -> non-zero (different value: MODIFIED)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the special case for zero -> zero
is needed, this can be reported together with X -> X
. It will not be needed in EIP-1087 either.
I will add more documentation. Thanks for suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the caller can deduce that by value == 0 && unchanged.
Please review again. |
Extend set_storage() by reporting the storage status.
Surprisingly, this was quite easy and aleth-interpreter is simpler and works without any test failure.