Skip to content

Add flush mechanism#289

Merged
GabrielMartinezRodriguez merged 4 commits into
masterfrom
gabriel/flush-mechanism
Jan 7, 2025
Merged

Add flush mechanism#289
GabrielMartinezRodriguez merged 4 commits into
masterfrom
gabriel/flush-mechanism

Conversation

@GabrielMartinezRodriguez

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented Dec 5, 2024

Copy link
Copy Markdown
Contributor

Linked issues

closes https://linear.app/happychain/issue/HAPPY-250/only-flush-transactions-that-have-been-modified

Description

This PR aims to add a mechanism to avoid re-updating transactions that have no changes

Key changes:

  • Added pendingFlush and notPersisted field to track database persistence
  • Consolidated updateTransaction, updateTransactions, and saveTransactions into a single saveTransactions method
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.

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 is documented.

@cloudflare-workers-and-pages

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

Copy link
Copy Markdown

Deploying happychain with  Cloudflare Pages  Cloudflare Pages

Latest commit: 6d6cd5c
Status: ✅  Deploy successful!
Preview URL: https://b1523595.happychain.pages.dev
Branch Preview URL: https://gabriel-flush-mechanism.happychain.pages.dev

View logs

@GabrielMartinezRodriguez GabrielMartinezRodriguez mentioned this pull request Dec 5, 2024
10 tasks
@GabrielMartinezRodriguez GabrielMartinezRodriguez marked this pull request as ready for review December 5, 2024 15:31

GabrielMartinezRodriguez commented Dec 5, 2024

Copy link
Copy Markdown
Contributor Author

@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title feat(txm): add flushedAt timestamp to transactions Add flush mechanism Dec 5, 2024
@GabrielMartinezRodriguez GabrielMartinezRodriguez added the reviewing-1 Ready for, or undergoing first-line review label Dec 5, 2024 — with Graphite App
@linear

linear Bot commented Dec 5, 2024

Copy link
Copy Markdown

Comment thread packages/transaction-manager/lib/TransactionRepository.ts
pendingFlush,
notPersisted,
metadata,
}: TransactionConstructorConfig & {

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 think we should reorder the fields above, such that the fields from TransactionConstructorConfig come first, followed by the rest.

I'm not quite clear on why the distinction exists at all btw, what's the thinking there?

Comment thread packages/transaction-manager/lib/Transaction.ts Outdated
Comment thread packages/transaction-manager/lib/Transaction.ts Outdated
Comment thread packages/transaction-manager/lib/Transaction.ts Outdated
Comment thread packages/transaction-manager/lib/Transaction.ts Outdated
Comment thread packages/transaction-manager/lib/Transaction.ts Outdated
@norswap norswap removed the reviewing-1 Ready for, or undergoing first-line review label Dec 20, 2024
@GabrielMartinezRodriguez GabrielMartinezRodriguez mentioned this pull request Dec 20, 2024
10 tasks
@norswap norswap added merge-blocked Ready to merge, waiting for downstack and removed reviewing-2 Ready for, or undergoing final review labels Dec 21, 2024
Base automatically changed from gabriel/txm-address to master January 7, 2025 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-blocked Ready to merge, waiting for downstack

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants