Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Allow strings and bigints instead of numbers in decoder input fields #6131

Merged
merged 3 commits into from
Jun 29, 2023

Conversation

haltman-at
Copy link
Contributor

PR description

Addresses #6080. This makes it so that fields in Log or Transaction that previously were number are now number | string | bigint, and fields that were number | null are now number | string | bigint | null.

Most of these fields are ignored, so the type is changed and that's all.

The one changed field that's actually used is blockNumber, so I just changed things so that if we get that and it's not null, we convert it to a number.

I do have to also discuss what happened to the DecodedLog type. For simplicity, DecodedLog extends Log. However, Log has just had its type broadened. But while that's fine for Log, because it's an input type, DecodedLog is an output type. So to broaden its type would be a breaking change. So I re-narrowed the types of the appropriate fields to prevent that.

Anyway that's it (aside from tests); this is pretty simple.

Testing instructions

I altered some decoder tests so that they pass in strings for the block number (the only part that matters), I think that should be sufficient.

@haltman-at haltman-at changed the title Allow strings instead of numbers in decoder input fields Allow strings and bigints instead of numbers in decoder input fields Jun 28, 2023
Copy link
Contributor

@gnidan gnidan left a comment

Choose a reason for hiding this comment

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

Small docs thing

Comment on lines 137 to 145
/**
* Index of the log within the block.
*/
logIndex?: number;
/**
* Index within the block of the emitting transaction; null if
* block is pending.
*/
transactionIndex?: number | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these optional fields need explanation on why they might be undefined. Probably warrants a note in the DecodedLog docstring, to say that @truffle/decoder re-emits everything given, and unspecified optional input fields will thus not appear in output

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, that's not how this works! Which raises a good point: These fields shouldn't be optional. I guess I should just not make this an extension of Log, huh? Because it doesn't save me any work if everything is mandatory. :-/

To explain: If you use decodeLog, passing in your own logs, you just get back the decodings, you don't get DecodedLog. You only get DecodedLog if you call events(), to which one does not pass in their own logs. So actually these fields will always be present in this type.

...so do you agree that's what I should do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can this emit the same info in both situations? It's weird that passing in a Log will remove fields, while events() will return everything log-related

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What? Passing in a Log doesn't remove fields. What I'm saying is that decodeLog doesn't use the DecodedLog type at all. It returns the decodings, they're not inside a DecodedLog container. Only events uses that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ohh, sorry to misunderstand. Yeah, let's not extend Log

@haltman-at haltman-at requested a review from gnidan June 29, 2023 19:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants