-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
fix: add missing receipts properties #3156
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
CodSpeed Performance ReportMerging #3156 will improve performances by 81.8%Comparing Summary
Benchmarks breakdown
|
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.
Nice work @Torres-ssf Given that this method is used in dryRun
, estimateTxDependencies
, estimateMultipleTxDependencies
and simulate
Provider methods, as well as the transaction summary methods , should we add fuel-gauge
tests to capture this change in the receipts?
@maschad I have updated the unit test where parsed receipts properties are validated 8cdfd23 |
@Torres-ssf I observed that, but that relies on mock data, do you not feel that our end-to-end / fuel-gauge tests would capture this type of inconsistency more readily given it's closer to a consumer API interaction? |
Why is this PR causing such a significant performance regression? 👀 |
IMO as these coders are due for deprecation, mocked data is sufficient for now. |
Good point @petertonysmith94 |
Coverage Report:
Changed Files:
|
Checklist