Skip to content
This repository has been archived by the owner on Dec 15, 2023. It is now read-only.

Bounce back invalid transactions #518

Merged
merged 9 commits into from
Aug 11, 2023
Merged

Bounce back invalid transactions #518

merged 9 commits into from
Aug 11, 2023

Conversation

FabijanC
Copy link
Collaborator

@FabijanC FabijanC commented Aug 9, 2023

Usage related changes

  • Properly bounce-back invalid transactions (according to the first four steps here)

Development related changes

  • Fix nonce specification in invoke testing utility
  • Refactor mapping of gateway to RPC errors
  • Support some errors from RPC specification 0.4.0 for proper mapping
  • Move internal tx creation into TransactionHandler
  • Some tests required max_fee to be reduced because, although sufficient, it was exceeding the available balance (mostly replaced 1e18 with 1e15)
    • test_fork.py::test_forking_testnet_from_too_early_block was failing, but due to an unexpected cause (insufficient balance), it should be failing due to contract not being deployed yet (now it does after a fix)
  • Some error expectations had to be changed: instead of checking the reason of being reverted in get_tx response (no longer available for validation failures), we now expect an immediate error (manifested in interaction with Starknet CLI - relying on ErrorExpector for this)

Checklist:

  • Applied formatting - ./scripts/format.sh
  • No linter errors - ./scripts/lint.sh
  • Performed code self-review
  • Rebased to the last commit of the target branch (or merged it into my branch)
  • Documented the changes
  • Linked the issues which this PR resolves
  • Updated the tests
  • All tests are passing - ./scripts/test.sh

@FabijanC FabijanC marked this pull request as ready for review August 10, 2023 14:39
@FabijanC FabijanC requested a review from mikiw August 10, 2023 14:39
Copy link
Contributor

@mikiw mikiw left a comment

Choose a reason for hiding this comment

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

looks legit to me

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants