Skip to content

Conversation

@GabrielMartinezRodriguez
Copy link
Contributor

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented Nov 20, 2024

Linked Issues

Description

This PR implements a mutex in the TxMonitor class to prevent overlapping executions of multiple transactions. Additionally, if more than one process is waiting to acquire the mutex, only the last process will acquire it, and the previous ones will be discarded

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.

This PR has been tested using the randomness service on localhost, interacting with the Anvil blockchain and simulating that a monitoring execution took longer than expected to verify that the mutex was working properly

  • 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.

This was referenced Nov 20, 2024
Copy link
Contributor Author

GabrielMartinezRodriguez commented Nov 20, 2024

@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review November 20, 2024 14:26
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Nov 20, 2024

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: 89f8e7e
Status: ✅  Deploy successful!
Preview URL: https://22bbb714.happychain.pages.dev
Branch Preview URL: https://gabriel-tx-monitor-lock.happychain.pages.dev

View logs

@linear
Copy link

linear bot commented Nov 20, 2024

HAPPY-136 txmanager: Avoid overlapping TxMonitor executions

cf. #182 (comment)

Maybe needed in some other blocks that listen on new blocks and do RPC calls?

@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title feat(txm): mutex in tx monitor Mutex in tx monitor Nov 20, 2024
@GabrielMartinezRodriguez GabrielMartinezRodriguez added the reviewing-1 Ready for, or undergoing first-line review label Nov 20, 2024
@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-2 Ready for, or undergoing final review and removed reviewing-1 Ready for, or undergoing first-line review labels Nov 22, 2024
@norswap norswap added the updating Updating after review label Dec 20, 2024
@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-2 Ready for, or undergoing final review and removed updating Updating after review labels Dec 20, 2024
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/tx-monitor-lock branch 2 times, most recently from 0812786 to b2791e0 Compare December 20, 2024 11:22
@GabrielMartinezRodriguez GabrielMartinezRodriguez mentioned this pull request Dec 20, 2024
10 tasks
"@happychain/common": "workspace:^",
"@happychain/configs": "workspace:^",
"@happychain/contracts": "workspace:^",
"async-mutex": "^0.5.0",
Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't need this one anymore :)

@norswap norswap added merge-after-changes Can be merged after requested changes are made and removed reviewing-2 Ready for, or undergoing final review labels Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-after-changes Can be merged after requested changes are made

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants