Fix hive getLogs #9889
Fix hive getLogs #9889
Conversation
deffrian
requested review from
LukaszRozmej,
benaadams and
smartprogrammer93
as code owners
alexb5dh
left a comment
alexb5dh
left a comment
There was a problem hiding this comment.
Mostly worried about replacing BlockParameterConverter with LongConverter.
|
|
||
| private static AddressFilter GetAddress(object? address) => | ||
| address switch | ||
| private static AddressFilter GetAddress(AddressAsKey[]? address) |
There was a problem hiding this comment.
Parameter should probably be named addresses, just to avoid confusion.
| else | ||
| { | ||
| FromBlock = ToBlock = BlockParameterConverter.GetBlockParameter(blockHash); | ||
| FromBlock = hasFromBlock ? new BlockParameter(LongConverter.FromString(fromBlockElement.ToString())) : BlockParameter.Earliest; |
There was a problem hiding this comment.
BlockParameterConverter also handled all non-integer values like earliest, latest, etc. I don't see this functionality in LongConverter, are these values still handled correctly?
In case if not - would be good to add some unit tests on these values specifically after a fix.
There was a problem hiding this comment.
I also see that earliest/latest params were removed/replaced in many tests, was this intentional?
| } | ||
|
|
||
| public AddressFilter(HashSet<AddressAsKey> addresses) | ||
| public AddressFilter(AddressAsKey[] addresses) |
There was a problem hiding this comment.
What was the reason behind replacing HashSet with an array?
| ToBlock = BlockParameterConverter.GetBlockParameter(toBlockElement.ToString()); | ||
| if (hasFromBlock || hasToBlock) | ||
| { | ||
| throw new ArgumentException("either (fromBlock and toBlock) or blockHash have to be specified"); |
There was a problem hiding this comment.
Honestly, geth message looks better to me, but it may be subjective:
cannot specify both BlockHash and FromBlock/ToBlock
Fixes Closes Resolves #
Changes
Types of changes
What types of changes does your code introduce?