Skip to content

feat(txm): added txm callbacks and promises when tx status change#719

Merged
GabrielMartinezRodriguez merged 4 commits intomasterfrom
gabriel/txm-transaction-callbacks
May 21, 2025
Merged

feat(txm): added txm callbacks and promises when tx status change#719
GabrielMartinezRodriguez merged 4 commits intomasterfrom
gabriel/txm-transaction-callbacks

Conversation

@GabrielMartinezRodriguez
Copy link
Contributor

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented May 5, 2025

Linked Issues

Description

Implemented a simple mechanism to wait for transaction finalization in the transaction manager

  • 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 are documented in the docs.
    Public APIS and 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) with make changeset for
    breaking and meaningful changes in packages (not required for cleanups & refactors).

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented May 5, 2025

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: e5b67a8
Status: ✅  Deploy successful!
Preview URL: https://9b389e5f.happychain.pages.dev
Branch Preview URL: https://gabriel-txm-transaction-call.happychain.pages.dev

View logs

Copy link
Contributor Author

GabrielMartinezRodriguez commented May 5, 2025

@GabrielMartinezRodriguez GabrielMartinezRodriguez added the reviewing-1 Ready for, or undergoing first-line review label May 5, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review May 5, 2025 14:12
@linear
Copy link

linear bot commented May 5, 2025

HAPPY-496 Implement a mechanism to easily allow TXM users to be notified when a transaction changes status

Currently, it's a bit tedious for a TXM user to know when a transaction has finished. This is because the hooks are general for all transactions, not for a specific one. It might be a good idea to implement a method in the Transaction object that allows users to subscribe to specific events of that transaction.

One possible approach is to provide a callback when creating the transaction, and once the transaction is included in the TXM, collect that callback and notify the user when the transaction is updated.

The usage could be:

const tx = new Transaction(...)

tx.on("success", () => {
...
})

txm.collectTransactions([tx]);

this.waitingPromises = []
}

this.callbacks[status].forEach((callback) => callback(this))
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens if the callback throws?

this.callbacks[status].push(callback)
}

waitForFinalization(timeout?: number): Promise<Result<Transaction, Error>> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not immediately obvious this is milliseconds, maybe rename to timeoutMs or document/comment.


test("Use transaction.waitForFinalization() to wait for a transaction to be finalized", async () => {
let promiseResolved = false
const transaction = await createCounterTransaction()
Copy link
Contributor

Choose a reason for hiding this comment

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

not urgent but createCounterTransaction should not be async

* Stores promises that wait for the transaction to be finalized.
* These promises are resolved when the transaction status changes to a finalized state.
*/
private waitingPromises: ((transaction: Result<Transaction, Error>) => void)[]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: rename to waitingFinalizedPromises

Copy link
Collaborator

@norswap norswap May 16, 2025

Choose a reason for hiding this comment

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

Could even be finalizedPromiseResolvers

let promiseResolved = false
const transaction = await createCounterTransaction()

transaction.waitForFinalization().then((result) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we add a timeout test to check that that functionality works as intended?

@aodhgan aodhgan added reviewing-2 Ready for, or undergoing final review and removed reviewing-1 Ready for, or undergoing first-line review labels May 8, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/txm-transaction-callbacks branch from 3f8de75 to 3bf00b1 Compare May 12, 2025 10:41
Comment on lines +208 to +214
this.callbacks = Object.values(TransactionStatus).reduce(
(acc, status) => {
acc[status] = []
return acc
},
{} as Record<TransactionStatus, ((transaction: Transaction) => void)[]>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
this.callbacks = Object.values(TransactionStatus).reduce(
(acc, status) => {
acc[status] = []
return acc
},
{} as Record<TransactionStatus, ((transaction: Transaction) => void)[]>,
this.callbacks = {} as Record<TransactionStatus, ((transaction: Transaction) => void)[]>
Object.values(TransactionStatus).forEach((status) => { this.callbacks[status] = [] })

Copy link
Collaborator

Choose a reason for hiding this comment

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

I ran stats on the repo the other day and this is the biggest file in it :D
Not important, but we can probably consider splitting it up sometime in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 May 16, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/txm-transaction-callbacks branch from 3bf00b1 to 40a224e Compare May 21, 2025 08:56
@GabrielMartinezRodriguez GabrielMartinezRodriguez merged commit 947f008 into master May 21, 2025
4 checks passed
@GabrielMartinezRodriguez GabrielMartinezRodriguez deleted the gabriel/txm-transaction-callbacks branch May 21, 2025 09:27
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.

4 participants