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

Storage status #52

Merged
merged 3 commits into from
Aug 9, 2018
Merged

Storage status #52

merged 3 commits into from
Aug 9, 2018

Conversation

chfast
Copy link
Member

@chfast chfast commented Aug 8, 2018

Extend set_storage() by reporting the storage status.

Surprisingly, this was quite easy and aleth-interpreter is simpler and works without any test failure.

size_t data_size,
const struct evmc_uint256be topics[],
size_t topics_count)
static void emit_log(struct evmc_context* context,
Copy link
Member

@axic axic Aug 8, 2018

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.

Copy link
Member Author

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.

Copy link
Member

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.

* @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.
Copy link
Member

@axic axic Aug 8, 2018

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

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. */
Copy link
Member

@axic axic Aug 8, 2018

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)

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 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.

Copy link
Member

@axic axic Aug 9, 2018

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.

@chfast
Copy link
Member Author

chfast commented Aug 9, 2018

Please review again.

@axic axic merged commit ee4bcd9 into master Aug 9, 2018
@axic axic deleted the storage-status branch August 9, 2018 10:14
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.

2 participants