Skip to content

Conversation

@GabrielMartinezRodriguez
Copy link
Contributor

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented Feb 11, 2025

Linked Issues

Description

This PR addresses the problem we were experiencing in production, where there was a nonce that had been used, but we did not have the corresponding attempt recorded in our system. This was causing infinite retries or cancellations if the transaction had a deadline.

Now, we have implemented a new status, Interrupted, where transactions meeting this criteria are moved. The user can decide to include the transaction again by sending it through the originator function.

  • Include all relevant context (but no need to repeat the issue's content).
  • Draw attention to new, noteworthy & unintuitive elements.
Toggle Checklist

Checklist

Basics

  • B1. I have applied the proper label & proper branch name (e.g. norswap/build-system-caching).
  • B2. This PR is not so big that it should be split & addresses only one concern.
  • B3. The PR targets the lowest branch it can (ideally master).

Reminder: PR review guidelines

Correctness

  • C1. Builds and passes tests.
  • C2. The code is properly parameterized & compatible with different environments (e.g. local,
    testnet, mainnet, standalone wallet, ...).
  • C3. I have manually tested my changes & connected features.

< INDICATE BROWSER, DEMO APP & OTHER ENV DETAILS USED FOR TESTING HERE >

< INDICATE TESTED SCENARIOS (USER INTERFACE INTERACTION, CODE FLOWS) HERE >

  • C4. I have performed a thorough self-review of my code after submitting the PR,
    and have updated the code & comments accordingly.

Architecture & Documentation

  • D1. I made it easy to reason locally about the code, by (1) using proper abstraction boundaries,
    (2) commenting these boundaries correctly, (3) adding inline comments for context when needed.
  • D2. All public-facing APIs & meaningful (non-local) internal APIs are properly documented in code
    comments.
  • D3. If appropriate, the general architecture of the code is documented in a code comment or
    in a Markdown document.
  • D4. An appropriate Changeset has been generated (and committed) for changes that touch npm published packages (currently pacakges/core and packages/react), see here for more info.

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Feb 11, 2025

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: caa054f
Status: ✅  Deploy successful!
Preview URL: https://3c8eb51d.happychain.pages.dev
Branch Preview URL: https://gabriel-transaction-reorder.happychain.pages.dev

View logs

Copy link
Contributor Author

GabrielMartinezRodriguez commented Feb 11, 2025

@GabrielMartinezRodriguez GabrielMartinezRodriguez mentioned this pull request Feb 11, 2025
10 tasks
@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review February 11, 2025 14:29
@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title feat(txm): added Interrupted status to transaction entity Added Interrupted status to transaction entity Feb 11, 2025
@linear
Copy link

linear bot commented Feb 11, 2025

HAPPY-365 TXM: Handle cases where the nonce moved past an intent but all our attemps are not included

Probably add new status, something like TRANSACTION_REORDER.

The tx can then re-enqueue it from an originator if need be.

The downside is we can have the tx "cut in line" — but because we already assigned nonces and probably submitted the other txs in the queue, this would require implementing nonce renumbering, which is bound to be extremely messy and complex.

This should never happen anyway — in theory it only happens if the submitter key is used concurrently somewhere else.

@GabrielMartinezRodriguez GabrielMartinezRodriguez added the reviewing-1 Ready for, or undergoing first-line review label Feb 11, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transaction-reorder branch 3 times, most recently from 0add1f7 to e2f7e3e Compare February 12, 2025 11:23
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transaction-reorder branch 2 times, most recently from c57ca4e to ff8bd52 Compare February 12, 2025 14:24
@norswap norswap added the question Something has to be cleared up after review label Feb 15, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transaction-reorder branch 2 times, most recently from 0f06207 to 073aefb Compare February 17, 2025 13:08
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/transaction-reorder branch 2 times, most recently from 926f11e to 734d609 Compare February 17, 2025 13:16
Base automatically changed from gabriel/deploy-randomness to master February 17, 2025 13:26
@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-2 Ready for, or undergoing final review and removed question Something has to be cleared up after review labels Feb 17, 2025
@norswap norswap added merge-after-changes Can be merged after requested changes are made merge-after-restack Can be merged after a restack and removed reviewing-2 Ready for, or undergoing final review merge-after-changes Can be merged after requested changes are made labels Feb 25, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez merged commit 76d2a32 into master Feb 26, 2025
2 of 3 checks passed
@GabrielMartinezRodriguez GabrielMartinezRodriguez deleted the gabriel/transaction-reorder branch February 26, 2025 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-after-restack Can be merged after a restack

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants