feat: project & rounds related repositories w/kysely #7
Conversation
packages/repository/package.json
Outdated
| "test:cov": "vitest run --config vitest.config.ts --coverage" | ||
| }, | ||
| "dependencies": { | ||
| "@grants-stack-indexer/shared": "workspace:0.0.1", |
There was a problem hiding this comment.
Do we want to use fixed versions for local packages or should we use *?
There was a problem hiding this comment.
aaa good q, idk
cc @0xkenj1 @jahabeebs thoughts?
There was a problem hiding this comment.
lets use * for development phase, to avoid assuming that we are managing versions on the monorepo
| /* @inheritdoc */ | ||
| async getRoundByRole( | ||
| chainId: ChainId, | ||
| roleName: "admin" | "manager", | ||
| roleValue: string, | ||
| ): Promise<Round | undefined> { |
There was a problem hiding this comment.
- I'm not able to find this method in the
IRoundRepositoryinterface. Might be missing in the interface? - Probably want to use the newly defined
RoundRoleNamesfor typingroleName?
There was a problem hiding this comment.
thanks good catch
| }; | ||
|
|
||
| export type NewRoundRole = RoundRole; | ||
| export type PartialRoundRole = Partial<RoundRole>; |
There was a problem hiding this comment.
Is this going to be used in the future? I think it's not being used anywhere at the moment.
There was a problem hiding this comment.
it's used in the updateRound function, for Kysely this types are complaint with their customs Selectable, Updatable, etc, types but in case you wanted to use a different ORM, probably we'll need a mapping from model to ORM entity
| export type PendingRoundRole = { | ||
| id?: number; | ||
| chainId: ChainId; | ||
| role: string; | ||
| address: Address; | ||
| createdAtBlock: bigint; | ||
| }; |
There was a problem hiding this comment.
Should this be something like the following? Or it'd introduce too much coupling?
| export type PendingRoundRole = { | |
| id?: number; | |
| chainId: ChainId; | |
| role: string; | |
| address: Address; | |
| createdAtBlock: bigint; | |
| }; | |
| export type PendingRoundRole = { id?: number } & Omit<RoundRole, "roundId">; |
If role should be typed as RoundRoleNames in both types, this would have saved you from having a role: string in one place and role: RoundRoleNames in the other.
There was a problem hiding this comment.
regarding schema typing, we're just copy-pasting their definitions since we don't want to introduce any modifications (in the first place), but your suggestion makes sense
There was a problem hiding this comment.
this suggestion makes lot of sense , but since they are different tables, i would keep it as it is to about coupling them
it will be in another PR, we will improve their current migration mechanism
|
There was a problem hiding this comment.
Amazing job, LGTM!! 💎
Just a small thing let's keep in mind that many other repositories will be introduced in the future, so the current directory structure may become a bit messy. I would suggest making a slight change, such as:
repositories/kysely/{ chaindata | pricing | metadata | whatever }/...
| export type PendingRoundRole = { | ||
| id?: number; | ||
| chainId: ChainId; | ||
| role: string; | ||
| address: Address; | ||
| createdAtBlock: bigint; | ||
| }; |
There was a problem hiding this comment.
this suggestion makes lot of sense , but since they are different tables, i would keep it as it is to about coupling them
| // In Allo V2 profile roles are emitted before a profile exists. | ||
| // The role emitted is the profile id. | ||
| // Once a profile is created we search for roles with that profile id | ||
| // and add real project roles. After that we can remove the pending project roles. |
🤖 Linear
Closes GIT-90 GIT-91
Description
repositoriespackageChecklist before requesting a review