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

Store orderUid in event data #36

Merged
merged 1 commit into from
Nov 15, 2022
Merged

Store orderUid in event data #36

merged 1 commit into from
Nov 15, 2022

Conversation

fedgiac
Copy link
Contributor

@fedgiac fedgiac commented Nov 15, 2022

An event that contains an indexed parameter with a reference types (like bytes memory) doesn't include the full data in the event data but only the hash (source).

We are however interested in reading orderUid from onchain data. We do this by not indexing orderUid.

Some other discarded options:

  1. We could emit the order hash instead, since validity and order owner are always the same. This leads to gas savings of ~900/1100. We decided these events aren't expected to fire often, so we prefer to keep the event more generic.
  2. We could keep an index for orderUid (e.g., event OrderInvalidation(bytes indexed _orderUid, bytes orderUid)). This costs ~500 gas. We decided against it because (1) it looks weird and (2) there is no concrete use case for selecting an event based on orderUid, since the backend will process every cancellation and show the result on the interface.

Test plan

Existing unit tests.

@fedgiac fedgiac requested a review from a team November 15, 2022 12:11
Copy link

@bh2smith bh2smith left a comment

Choose a reason for hiding this comment

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

Seems alright to me, the info will still be part of the event logs.

I am not sure how EVM indexers like dune handle indexed vs non-indexed event data. I am also not exactly sure what is meant by this at the EVM level (perhaps you could remind me).

@fedgiac
Copy link
Contributor Author

fedgiac commented Nov 15, 2022

Seems alright to me, the info will still be part of the event logs.

This is the issue: if the parameter is indexed, then the info is not part of the event log at an EVM level but only its hash is. Alex and Nick noticed here.

@bh2smith
Copy link

Right but dune decodes event data so it may actually recover the value we care about regardless of whether it is indexed.

@fedgiac fedgiac merged commit 22e3b4a into main Nov 15, 2022
@fedgiac fedgiac deleted the orderuid-not-indexed branch November 15, 2022 14:27
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.

3 participants