Skip to content

Add retry policy manager for failed transactions#299

Merged
GabrielMartinezRodriguez merged 6 commits into
masterfrom
gabriel/retry-policy
Jan 21, 2025
Merged

Add retry policy manager for failed transactions#299
GabrielMartinezRodriguez merged 6 commits into
masterfrom
gabriel/retry-policy

Conversation

@GabrielMartinezRodriguez

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented Dec 9, 2024

Copy link
Copy Markdown
Contributor

Linked Issues

Description

Introduces a new RetryPolicyManager to handle transaction retry logic in a more modular way. The manager determines if failed transactions should be retried based on their receipt and revert reason. Currently implements retry logic for "Out of Gas" errors, either by checking gas consumption or explicit revert messages.

Key changes:

  • New RetryPolicyManager with extensible interface for custom retry policies
  • Integration with TransactionManager for configurable retry behavior
  • Refactored transaction monitoring to use the new retry policy system
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).

Correctness

  • C1. Builds and passes tests.

  • C2. The code is properly parameterized & compatible with different environments.

  • C3. I have manually tested my changes & connected features.

  • C4. I have performed a thorough self-review of my code after submitting the PR.

Architecture & Documentation

  • D1. I made it easy to reason locally about the code.
  • D2. All public-facing APIs & meaningful internal APIs are properly documented.
  • D3. If appropriate, the general architecture of the code is documented.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Dec 9, 2024

Copy link
Copy Markdown

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: 95598fd
Status: ✅  Deploy successful!
Preview URL: https://7fc8403d.happychain.pages.dev
Branch Preview URL: https://gabriel-retry-policy.happychain.pages.dev

View logs

GabrielMartinezRodriguez commented Dec 9, 2024

Copy link
Copy Markdown
Contributor Author

@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review December 9, 2024 12:08
@GabrielMartinezRodriguez GabrielMartinezRodriguez added the reviewing-1 Ready for, or undergoing first-line review label Dec 9, 2024
@linear

linear Bot commented Dec 9, 2024

Copy link
Copy Markdown
HAPPY-110 txmanager: Allow retry policy customization

We should allow users to implement their own retry logic with code or perhaps through a configuration when a transaction reverts

import type { Attempt, Transaction } from "./Transaction"
import type { TransactionManager } from "./TransactionManager"

export interface RevertedTransactionReceipt<T extends "success" | "reverted"> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this seems like it should match the return result from the viem getTransactionReceipt type. we probably don't want to/need to redefine it here do we?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree, this only add the extra typing for the status, but if we really want that (honestly, it doesn't buy us much), we can have it by extending the getTransactionReceipt return type like

export type RevertedTransactionReceipt<Status extends "success" | "reverted> =
     TransactionReceipt<bigint, number, Status, "eip1559">

Comment thread packages/transaction-manager/lib/TxMonitor.ts Outdated

@norswap norswap left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Mostly just docs + some type changes, but otherwise this is good!

Comment thread packages/transaction-manager/lib/RetryPolicyManager.ts Outdated
Comment thread packages/transaction-manager/lib/RetryPolicyManager.ts Outdated
Comment thread packages/transaction-manager/lib/RetryPolicyManager.ts Outdated
Comment thread packages/transaction-manager/lib/RetryPolicyManager.ts
Comment thread packages/transaction-manager/lib/TxMonitor.ts Outdated
import type { Attempt, Transaction } from "./Transaction"
import type { TransactionManager } from "./TransactionManager"

export interface RevertedTransactionReceipt<T extends "success" | "reverted"> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree, this only add the extra typing for the status, but if we really want that (honestly, it doesn't buy us much), we can have it by extending the getTransactionReceipt return type like

export type RevertedTransactionReceipt<Status extends "success" | "reverted> =
     TransactionReceipt<bigint, number, Status, "eip1559">

@norswap norswap added updating Updating after review and removed reviewing-1 Ready for, or undergoing first-line review labels Jan 5, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-2 Ready for, or undergoing final review and removed updating Updating after review labels Jan 6, 2025
@GabrielMartinezRodriguez GabrielMartinezRodriguez force-pushed the gabriel/retry-policy branch 2 times, most recently from 2d1b669 to 039ecfa Compare January 13, 2025 12:53
@GabrielMartinezRodriguez

Copy link
Copy Markdown
Contributor Author

@norswap I think there was an error with the diff files in this PR because I didn't restack it, and it was showing the diff from another PR. But anyway, I think the comments you made, even if they aren't for this PR, are good and can be applied to this one.

@GabrielMartinezRodriguez GabrielMartinezRodriguez added reviewing-2 Ready for, or undergoing final review and removed updating Updating after review labels Jan 13, 2025
): Promise<boolean>
}

export class DefaultRetryPolicyManager implements RetryPolicyManager {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Add docstring that says this retries if running out of gas.

NotFinalizedStatuses.includes(transaction.status),
)
if (result.isOk()) {
this.notFinalizedTransactions.push(...notPersistedTransactions)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Correct. What we could do here is invert the two lines, which saves on filtering the notPersistedTransactions since they'll always be included. Bit of a micro-optimization.

this.notFinalizedTransactions = this.notFinalizedTransactions.filter((transaction) =>
     NotFinalizedStatuses.includes(transaction.status),
)
this.notFinalizedTransactions.push(...notPersistedTransactions)

(current code has the reversed ordering)

@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 Jan 14, 2025
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.

3 participants