Use kysely instead of mikro-orm by GabrielMartinezRodriguez · Pull Request #250 · HappyChainDevs/happychain

GabrielMartinezRodriguez · 2024-11-11T15:42:04Z

Linked Issues

Description

We are experiencing issues with bundling and Mikro-ORM. The problem is that Mikro-ORM cannot detect decorators in the bundled file due to how Bun transpiles the code from TypeScript to JavaScript.

After consideration, we have decided to use Kysely instead of Mikro-ORM. Kysely is not an ORM, it's a type-safe query builder.

An important change introduced by this PR is that we no longer have an entity manager, as we did with Mikro-ORM. For this reason, we now need to explicitly update every transaction we modify by calling the repository, instead of simply making changes and then flushing, as was the case with Mikro-ORM. I think this is actually an improvement, as it makes the code cleaner. Now, we have full control over what and when we commit a change to the database.

More context:

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.

I tested it using the randomness service connected to Anvil

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.

GabrielMartinezRodriguez · 2024-11-11T15:42:19Z

Add flush mechanism #289 : 2 dependent PRs (#297 , #298 )
Allow run multiple txm simultaneously within the same service #285
Added docs for txm #272
Mutex in tx monitor #264
Purge transactions #232
Txm hooks #229
Use kysely instead of mikro-orm #250 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

cloudflare-workers-and-pages · 2024-11-11T15:43:46Z

Deploying happychain with Cloudflare Pages

Latest commit:	`ce7a4af`
Status:	✅ Deploy successful!
Preview URL:	https://e101251f.happychain.pages.dev
Branch Preview URL:	https://gabriel-chore-change-orm.happychain.pages.dev

View logs

not-reed

kysely to take over the world 😈 😁 jk, but i do like it

norswap

Good stuff! My main comment is we should probably reintroduce something like flush so we avoid writing txs multiple times to the DB with no changes.

norswap · 2024-12-04T12:53:23Z


        const result = await ResultAsync.fromPromise(
-            this.transactionManager.transactionRepository.flush(),
+            this.transactionManager.transactionRepository.updateTransactions(transactions),


Maybe we should reintroduce a flush abstraction (probably implemented via a "dirty" flag in Transaction). Some of these transactions will have been written to db already due to their handling, so rewriting them is inefficient.

Maybe some also don't change at all after going through the monitor?

I have created a new issue to address this because it is better to implement it on top of all the changes we currently have. In the PR for prune TX, I added the updatedAt field, which we can use to determine whether we need to flush or not.

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

norswap · 2024-12-04T12:54:24Z

+        } else if (it.status === "Error") {
+            console.error(`failed to execute migration "${it.migrationName}"`)
+        }
+    })


Just out of curiosity, how does it track which migrations have already been applied, do kysely maintain its own db of applied migrations?

Yes, Kisely creates two tables in our database to track which migrations have been executed:

norswap · 2024-12-04T12:57:16Z

-        this.txmgr.transactionRepository.saveTransactions(transactionsBatch)
+        const saveResult = await this.txmgr.transactionRepository.saveTransactions(transactionsBatch)

-        // TODO: If flush fails, we should notify the user


We should preserve that TODO (do we have a related issue?), as similarly, we can still fail to write the tx to DB.

I added the comment again and I created an issue to implement the notification. Now it's possible given that we have the hook system

https://linear.app/happychain/issue/HAPPY-249/notify-user-if-txm-fails-saving-a-transaction

norswap · 2024-12-04T14:35:10Z

+        const result = await ResultAsync.fromPromise(
+            db.transaction().execute(async (dbTransaction) => {
+                const promises = transactions.map((t) =>
+                    dbTransaction.insertInto("transaction").values(t.toDbRow()).execute(),


I think we could do a single insert statement that inserts all transactions at once instead of a transaction that contains a bunch of singular insert statements.

You are right

I think it's still doing multiple SQL queries under the hood. I think it should look something like this: https://kysely.dev/docs/examples/insert/multiple-rows

Either fix it here, or open a new issue (post here) for this and merge this one.

I am going to merge because there is no update function to update multiple rows

https://kysely.dev/docs/examples/update/single-row

GabrielMartinezRodriguez self-assigned this Nov 11, 2024

GabrielMartinezRodriguez added the reviewing-1 Ready for, or undergoing first-line review label Nov 11, 2024

GabrielMartinezRodriguez marked this pull request as ready for review November 11, 2024 21:10

GabrielMartinezRodriguez requested a review from not-reed November 11, 2024 21:11

GabrielMartinezRodriguez commented Nov 11, 2024

View reviewed changes

Comment thread packages/randomness-service/build.config.ts

not-reed approved these changes Nov 11, 2024

View reviewed changes

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

Comment thread packages/randomness-service/build.config.ts

GabrielMartinezRodriguez changed the title ~~feat(txm): use kisely instead of mikro-orm~~ Use kisely instead of mikro-orm Nov 11, 2024

This was referenced Nov 11, 2024

Txm hooks #229

Merged

Purge transactions #232

Merged

GabrielMartinezRodriguez changed the title ~~Use kisely instead of mikro-orm~~ Use kysely instead of mikro-orm Nov 11, 2024

GabrielMartinezRodriguez removed the reviewing-1 Ready for, or undergoing first-line review label Nov 13, 2024

GabrielMartinezRodriguez requested a review from norswap November 14, 2024 08:18

GabrielMartinezRodriguez added the reviewing-2 Ready for, or undergoing final review label Nov 14, 2024

GabrielMartinezRodriguez force-pushed the gabriel/chore-change-orm branch from 36d0751 to 8fdcc44 Compare November 20, 2024 14:21

GabrielMartinezRodriguez mentioned this pull request Nov 20, 2024

Mutex in tx monitor #264

Merged

10 tasks

GabrielMartinezRodriguez mentioned this pull request Nov 28, 2024

Added docs for txm #272

Merged

10 tasks

GabrielMartinezRodriguez force-pushed the gabriel/chore-change-orm branch from 8fdcc44 to 7df84a7 Compare December 4, 2024 10:25

GabrielMartinezRodriguez mentioned this pull request Dec 4, 2024

Allow run multiple txm simultaneously within the same service #285

Merged

10 tasks

norswap reviewed Dec 4, 2024

View reviewed changes

norswap added updating Updating after review and removed reviewing-2 Ready for, or undergoing final review labels Dec 4, 2024

GabrielMartinezRodriguez added 6 commits December 5, 2024 11:18

feat(txm): use kisely instead of mikro-orm

a59f414

chore: format

9cb2037

chore(txm): fixes kysely

b5af894

chore(txm): autoreview pr

9fd3cc9

chore(txm): remove console.log

0b61816

chore(txm): remove console.log

a0d5664

GabrielMartinezRodriguez force-pushed the gabriel/chore-change-orm branch from 7df84a7 to 2ac9c34 Compare December 5, 2024 10:22

GabrielMartinezRodriguez added reviewing-2 Ready for, or undergoing final review and removed updating Updating after review labels Dec 5, 2024

GabrielMartinezRodriguez requested a review from norswap December 5, 2024 11:32

GabrielMartinezRodriguez mentioned this pull request Dec 5, 2024

Add flush mechanism #289

Merged

10 tasks

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 5, 2024

GabrielMartinezRodriguez merged commit c8018df into master Dec 6, 2024

GabrielMartinezRodriguez deleted the gabriel/chore-change-orm branch December 6, 2024 09:57

This was referenced Dec 13, 2024

Random oracle contracts #307

Merged

Submit drand numbers #318

Merged

This was referenced Dec 26, 2024

Script to easily launch the randomness service locally #324

Merged

Script to monitor randomness service #325

Merged

This was referenced Feb 3, 2025

Add config contract #404

Merged

Deploy randomness service #416

Merged

This was referenced Feb 11, 2025

Added Interrupted status to transaction entity #434

Merged

debug #435

Closed

Implemented block inactivity timeout #441

Merged

GabrielMartinezRodriguez mentioned this pull request Feb 25, 2025

Fix: random() reverts once every 3 blocks #458

Merged

11 tasks

Uh oh!

Conversation

GabrielMartinezRodriguez commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linked Issues

Description

Checklist

Basics

Correctness

Architecture & Documentation

Uh oh!

GabrielMartinezRodriguez commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloudflare-workers-and-pages Bot commented Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Deploying happychain with Cloudflare Pages

Uh oh!

Uh oh!

not-reed left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

norswap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GabrielMartinezRodriguez commented Nov 11, 2024 •

edited

Loading

GabrielMartinezRodriguez commented Nov 11, 2024 •

edited

Loading

cloudflare-workers-and-pages Bot commented Nov 11, 2024 •

edited

Loading