optimism icon indicating copy to clipboard operation
optimism copied to clipboard

add `shouldValidate` to Receipt Fetchers; Introduce more tests

Open axelKingsley opened this issue 1 year ago • 2 comments

Building off: https://github.com/ethereum-optimism/client-pod/issues/526

We fixed the validation of receipts, and along the way noted two open areas of improvement:

shouldValidate

Receipt Providers' FetchReceipts function should be modified to include a boolean shouldValidate in the signature. Receipt Providers now have everything they need to handle validation, but we don't want to do multiple redundant rounds of it. Instead, fetchers should consult the boolean to either do the validation directly, or ask an inner scope fetcher to do so.

Unit Tests

We have unit tests which confirm correct behavior of the validation function, and we also have a test at the EL Client level confirming that validation is used by at least one of the fetchers. However, we would also like tests which confirm the following things:

  • At the client level, if a receipt fetching fails validation, the invalid data is not returned later from the cache
  • At each fetcher level, if shouldValidate is true, validation returns an error, and in the case of the caching fetcher, the cache is not populated.

These tests will help us confirm that beyond validation being used and being correct, that each fetcher treats validation appropriately.

axelKingsley avatar Feb 08 '24 15:02 axelKingsley

@axelKingsley is this still something we plan to do or we can close this one?

BlocksOnAChain avatar Mar 29 '24 16:03 BlocksOnAChain

Yes, this is architectural/tech-debt. No real expiration to the need.

axelKingsley avatar May 10 '24 19:05 axelKingsley