Conversation
wjbeau
left a comment
wjbeau
left a comment
There was a problem hiding this comment.
I think the switch to tanstack/db makes sense because we get an implicitly reactive store which is great for our purposes.
Generally I think there might be too many "layers" in here though, probably a left over from the db <> object alignment issues. We have collections and repositories and rows and objects and it feels more complicated than it needs to be. I think it's worth a careful review and getting rid of unnecessary layers of indirection and abstraction. Also worth thinking carefully about separation of concerns and where we want to put which burden of responsibility so the ownership is clear and obvious.
| function rowToBalance(row: AccountBalanceCollectionRow): AccountBalanceRow { | ||
| return { | ||
| accountAddress: row.accountAddress, | ||
| algoBalance: row.algoBalance, | ||
| totalAssetsOptedIn: row.totalAssetsOptedIn, | ||
| totalCreatedAssets: row.totalCreatedAssets, | ||
| totalAppsOptedIn: row.totalAppsOptedIn, | ||
| minBalance: row.minBalance, | ||
| status: row.status, | ||
| authAddress: row.authAddress, | ||
| } | ||
| } |
There was a problem hiding this comment.
Feels like one of the benefits we get with tanstack is we avoid the impedence mismatch between db and ojbects. I think we can get rid of the AccountBalanceCollectionRow object and just use AccountBalanceRow or maybe even just AccountBalance.
| unitName: node.unitName ?? undefined, | ||
| url: node.url ?? undefined, | ||
| metadata: node.metadata ?? undefined, | ||
| peraMetadata: parseMetaJson(pera?.peraMetadataJson ?? null), |
There was a problem hiding this comment.
Is it a requirement that we have flat objects using tanstack? i.e. why is peraMetadata modelled as a string rather than just letting the object serialize/deserialize inside tanstack/db? It would avoid the whole object<>json dance on persistence and retrieval
| export type AccountBalanceRow = { | ||
| network: string | ||
| accountAddress: string | ||
| /** ALGO balance in display units (ALGOs, not microAlgos). */ | ||
| algoBalance: Decimal | ||
| totalAssetsOptedIn: number | ||
| totalCreatedAssets: number | ||
| totalAppsOptedIn: number | ||
| /** Minimum balance in display units (ALGOs, not microAlgos). */ | ||
| minBalance: Decimal | ||
| status: string | ||
| authAddress: string | null | ||
| updatedAt: number | ||
| } |
There was a problem hiding this comment.
This feels a little weird that we're breaking domain isolation boundaries like this. With this model we'll have to have the type definitions for various domain specific object shapes in the database package and the corresponding business logic in the domain package. Not sure what the solution would be but it feels suboptimal.
| * backed by `MemoryAdapter` — use this in `beforeEach` to guarantee | ||
| * test isolation without paying for JSON roundtrips. | ||
| */ | ||
| export function bootstrapTestCollections(): CollectionRegistry { |
There was a problem hiding this comment.
Maybe this should be in a separate file since it's test code?
| @@ -27,10 +27,16 @@ | |||
| "drizzle-orm": "catalog:", | |||
There was a problem hiding this comment.
Should we be removing drizzle and sqlite?
| return rows[0] ? rowToEntry(rows[0]) : null | ||
| const { nfdCache } = resolveRegistry(registry) | ||
| const row = nfdCache.get(nfdCacheKey({ network, address })) | ||
| return row ? rowToEntry(row) : null |
There was a problem hiding this comment.
to my previous point, why do we need this rowToEntry pattern? Can't we just persist the entry shape directly now?
| * Unlike the previous SQL-backed version, this runs entirely in memory: | ||
| * O(addresses.length) map lookups, no allocation beyond the result | ||
| * array. That's faster than the previous query for the common case | ||
| * where the caller hands us a short list (a single account's peers). |
There was a problem hiding this comment.
We should probably remove comments that refer to a previous implementation - that isn't so valuable going forwards.
2 participants
Pull Request Template
Description
Related Issues
Checklist
Additional Notes