-
Notifications
You must be signed in to change notification settings - Fork 244
[SUBGRAPH] Add gasUsed
property to all events
#1199
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
Conversation
* bump subgraph package versions * add `gasUsed` property to all event entities * add tests for `gasUsed` property
packages/subgraph/src/utils.ts
Outdated
if (event.receipt) { | ||
// @note if we don't cast event.receipt as ethereum.TransactionReceipt, | ||
// we get a compile error because it thinks event.receipt is: | ||
// ethereum.TransactionReceipt | null |
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.
Normally TS should be able to infer the correct type with this logic:
const receipt = event.receipt;
if (receipt) {
entity.set("gasUsed", Value.fromBigInt(receipt.gasUsed));
} else {
entity.set("gasUsed", Value.fromBigInt(BIG_INT_ZERO));
}
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.
yeah I think someone from their side suggested this too, will make this change 👍🏼
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.
Remove the comment too.
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.
ok
packages/subgraph/src/utils.ts
Outdated
const receipt = event.receipt as ethereum.TransactionReceipt; | ||
entity.set("gasUsed", Value.fromBigInt(receipt.gasUsed)); | ||
} else { | ||
entity.set("gasUsed", Value.fromBigInt(BIG_INT_ZERO)); |
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.
If the receipt should always be present (i.e. specified in the .yaml
) then I'd prefer throwing in general.
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.
true
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.
It's not going to throw ATM as the else-block is just removed? 😛
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.
it will throw because when we go and try to save the entity, this field is non-nullable
* remove casting * let it break if unable to get receipt
XKCD Comic RelifLink: https://xkcd.com/1199 |
gasUsed
property to all event entitiesgasUsed
property