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

/{runtime}/events: Include eth_tx_hash in response #419

Merged
merged 1 commit into from
May 17, 2023

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented May 16, 2023

Closes: #363

@ptrus ptrus closed this May 16, 2023
@ptrus ptrus reopened this May 16, 2023
@ptrus ptrus requested a review from abukosek May 16, 2023 14:17
Copy link
Collaborator

@mitjat mitjat left a comment

Choose a reason for hiding this comment

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

Thank you! Have you tested the JSON response for an event with a corresponding tx, and for an event with no tx? (Events with no tx include one of deposit/withdrawal, IIRC. You can find them with make psql, then select ... from chain.runtime_events where "type"=...)

I can't wait for #410 to land so we get these tests/comparisons mostly automatically :)

storage/client/queries/queries.go Outdated Show resolved Hide resolved
api/spec/v1.yaml Outdated
@@ -1912,6 +1914,12 @@ components:
Hash of this event's originating transaction.
Absent if the event did not originate from a transaction.
example: *tx_hash_1
eth_tx_hash:
type: string
nullable: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this is not needed and has a slightly different meaning: That the value might be an explicit null (as opposed to being omitted, which is what we're doing). It's not 100% clear to me from the docs though.

Anyway, what we've been doing elsewhere (except tx_hash apparently ... that surprises me) was to just omit the field from required (line 1897), which is the standard openapi way of saying that this is an optional field.

Copy link
Member Author

@ptrus ptrus May 17, 2023

Choose a reason for hiding this comment

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

You are right, after testing it works as you expected. Updated as you suggested.

Not sure why it's like that for tx_hash, I guess it was preferred to return explicitly tx_hash=null field for events not tied to a transaction. I kept that as is in this PR, and can be updated in a separate PR.


Note there is now some inconsistency, where eth_tx_hash will either be present or not (depending on if it originates from an ETH transaction), while the tx_hash will always be present, and explicitly null for block events. Can be updated in a follow-up PR.

@ptrus ptrus force-pushed the ptrus/feature/events-eth-hash branch from fbea296 to 746a934 Compare May 17, 2023 11:55
@ptrus ptrus force-pushed the ptrus/feature/events-eth-hash branch from 746a934 to 75dc9f6 Compare May 17, 2023 11:57
@ptrus
Copy link
Member Author

ptrus commented May 17, 2023

Have you tested the JSON response for an event with a corresponding tx, and for an event with no tx?

Tested now (after addressing review comments) and it works as expected in all cases (runtime event, evm runtime event, block event). There is a minor inconsistency between tx_hash and eth_tx_hash, see the other comment.

@ptrus ptrus merged commit e8be16c into main May 17, 2023
5 checks passed
@ptrus ptrus deleted the ptrus/feature/events-eth-hash branch May 17, 2023 12:11
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.

/{runtime}/events: Include eth_tx_hash in response
3 participants