Skip to content
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

chore: filter out receipt properties on dryrun #1710

Closed
wants to merge 11 commits into from

Conversation

Torres-ssf
Copy link
Contributor

@Torres-ssf Torres-ssf marked this pull request as ready for review February 2, 2024 23:29
@Torres-ssf Torres-ssf enabled auto-merge (squash) February 2, 2024 23:34
arboleya

This comment was marked as outdated.

@arboleya arboleya self-requested a review February 3, 2024 02:00
Copy link
Contributor

github-actions bot commented Feb 3, 2024

Coverage Report:

Lines Branches Functions Statements
78.4%(-0.02%) 68.43%(+0%) 76.65%(+0%) 78.37%(-0.02%)
Changed Files:
Ok File (✨=New File) Lines Branches Functions Statements
🔴 packages/providers/src/generated/operations.ts 90%
(+0.11%)
100%
(+0%)
67.74%
(+0%)
90.38%
(+0.09%)
🔴 packages/providers/src/transaction-response/transaction-response.ts 19.04%
(-0.47%)
12.5%
(+0%)
37.5%
(+0%)
19.04%
(-0.47%)

Copy link
Member

@Dhaiwat10 Dhaiwat10 left a comment

Choose a reason for hiding this comment

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

🔥🔥

gasUsed
data
recipient
contractId
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be redundant given that contract { id } is defined above.

Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

@Torres-ssf Even after my suggestions, this is still a bit off. The dry-run queries will return less data than a few utility methods will require down the road.

I don't think it's worth the risk for this release, so I created another PR here:

This is safer because the removed properties are not used anywhere by any other type. I also fixed a few fragment types, and everything looks okay.

I understand we still have more properties to trim, but I'd suggest doing those in separate PRs with more time to double-check everything.

@Torres-ssf
Copy link
Contributor Author

Closing this if favor of #1715

@Torres-ssf Torres-ssf deleted the st/chore/filter-out-receipt-param-dry-run branch February 26, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Issue is a chore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure dry runs request only necessary data
4 participants