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

feat(rpc/v05): add starknet_getTransactionReceipt implementation #1447

Merged
merged 3 commits into from
Oct 20, 2023

Conversation

pierre-l
Copy link
Contributor

This PR adds an implementation for starknet_getTransactionReceipt.

The only change I have found in the spec is, as mentioned in the original issue, the addition of the execution_resources resources field.

The first commit consists in copy-pasting the v0.4 implementation, so I recommend reviewing this PR commit by commit.

Please let me know all that I can improve, including:

  • how far should we go in terms of code reuse/duplication
  • how to test this properly
  • how close should we stay to the spec (using strings because specified like this in the spec when it's just numbers?)

Closes #1429

@pierre-l pierre-l force-pushed the 1429-get-transaction-receipt branch from e343b61 to 4644539 Compare October 18, 2023 11:42
@pierre-l pierre-l marked this pull request as ready for review October 18, 2023 12:17
@pierre-l pierre-l requested a review from a team as a code owner October 18, 2023 12:17
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig left a comment

Choose a reason for hiding this comment

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

ito tests I would add one just to verify the From impl for resources because that is brand new here. So just something that ensures that each field is mapped correctly by using different numbers for each field. Currently the only test exercising this is the json output which is defaulted i.e. all 0 so you won't pick up anything bad.

ito code reuse - I think in the spirit of getting this out we can just copy paste for now to be safer. i.e. to be sure we don't change v0.4 behaviour. We should tackled code reuse in the future though; but we aren't really ready for that yet imo.

@pierre-l pierre-l marked this pull request as draft October 19, 2023 08:35
@pierre-l pierre-l force-pushed the 1429-get-transaction-receipt branch from a269922 to 8ad3801 Compare October 20, 2023 07:59
@pierre-l pierre-l marked this pull request as ready for review October 20, 2023 08:06
Duplicate the v04 implementation before making changes
Move the pending receipt struct next to the regular receipt struct
Add the "execution_resources" field
Introduce a new `ExecutionResourcesProperties` to filter out the unnecessary fields from `ExecutionResources`
@pierre-l pierre-l force-pushed the 1429-get-transaction-receipt branch from 8ad3801 to 762dffe Compare October 20, 2023 08:55
@pierre-l pierre-l merged commit 4a2a176 into eqlabs:main Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

starknet_getTransactionReceipt
2 participants