Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wip: db scaffolding #4

Merged
merged 9 commits into from
Jan 30, 2024
Merged

wip: db scaffolding #4

merged 9 commits into from
Jan 30, 2024

Conversation

wtfsayo
Copy link
Contributor

@wtfsayo wtfsayo commented Jan 22, 2024

todo: create migrations

todo: create migrations
@wtfsayo wtfsayo requested a review from plor January 22, 2024 14:46
Copy link
Contributor

@plor plor left a comment

Choose a reason for hiding this comment

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

Happy to chat on this further.

Comment on lines 39 to 66
model AppRule {
id String @id @unique @default(cuid())
name String
value String
editable Boolean
validationValue String
app App @relation(fields: [appId], references: [id])
appId String
appRuleType AppRuleType @relation(fields: [appRuleTypeId], references: [id])
appRuleTypeId String
}

model AppRuleType {
id String @id @unique @default(cuid())
validationType ValidationType
oneOrMany OneOrMany
appRules AppRule[]
}

enum ValidationType {
list
regex
}

enum OneOrMany {
one
many
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to talk about this more. My goal was to separate this logically from the domain logic so there is a more clear handoff between the frontend and the gateway. This is where we went off track in our meeting, so I understand why our handle on this different.

Comment on lines 10 to 15
model User {
id String @id @default(cuid())
email String @unique
name String?
id String @id @unique @default(cuid())
org Org @relation(fields: [orgId], references: [id])
orgId String
apps App[] @relation("UserApps")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we got off User too soon, since this is the first thing we'll need, we should model how we handle authentication in the database. Do we add eth_address here? Or do we link to an authentication interface which handles the implementation?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we avoid checking in OS specific files? Add to gitignore perhaps.

@wtfsayo
Copy link
Contributor Author

wtfsayo commented Jan 28, 2024

@plor ready for review from today's discussion; added [x] in comments at bottom

@plor
Copy link
Contributor

plor commented Jan 28, 2024

I don't know if the changes made it in. Can you double check? Also, would it make sense to separate this from the web-portal and put it in a service layer?

@wtfsayo
Copy link
Contributor Author

wtfsayo commented Jan 28, 2024

I did an oopsie; check now; also yeah will migrate Prisma to different path tomorrow

Copy link
Contributor

@plor plor left a comment

Choose a reason for hiding this comment

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

Looks good, still hoping you can remove the DS_STORE

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this from commit?

Comment on lines 23 to 24
paymentLedger PaymentLedger?
relayLedger RelayLedger?
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this is confusing, I'm curious how this looks in SQL form.

tenantId String @unique
referenceId String
amount Int
chainId String
Copy link
Contributor

Choose a reason for hiding this comment

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

This will be important to tie to chain weight, I wonder if it needs to be generalized if there are other things we want to weight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to my understanding, based on what we discussed that would be a business logic (i.e. how to come to a chain weight); I would want to keep that separate

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I'll have to think about this, if we don't have the weight here we might obscure the logic.

referenceId String
amount Int
chainId String
keyId String // fed from redis(redis-pg sync service // tbd
Copy link
Contributor

Choose a reason for hiding this comment

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

Just for reference, this might be necessary to have as a reference to the id on the key table so we have consistancy (a string here that doesn't match a row in the key table would result in brokenness).

@plor
Copy link
Contributor

plor commented Jan 28, 2024

Still wondering if we want a separate service layer from the portal backend, I don't know what the best way to logically separate this stuff is.

Copy link
Contributor

@plor plor left a comment

Choose a reason for hiding this comment

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

Just a couple nitpicks. Also, I think the naming on "services/postgres" isn't quite right. My point was more to push the tenant stuff to a service separate from the portal. So this would be something like a core-account service which would include the postgres and the api to serve postgres. I don't know if it makes sense to have postgres live in a project of its own.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to complain about this until it is gone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hopefully in next commit; as I didn't see it was in a sub directory

updatedAt DateTime
}

model Key {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking "Key" might be too generic here, ApiKey or TenantAuthKey might be more descriptive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TenantAuthKey sounds best to me


-- CreateTable
CREATE TABLE "Org" (
"id" TEXT NOT NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be nitpicky, but is it best to use text for these type of short string fields? I know I used to do VARCHAR(32) or something in the past. Does "text" not have any performance hit? I know this is generated, but I assume you can give prisma a hint.

@plor plor linked an issue Jan 29, 2024 that may be closed by this pull request
@wtfsayo wtfsayo merged commit 7a1ce0f into develop Jan 30, 2024
@wtfsayo wtfsayo deleted the database-scaffolding branch January 31, 2024 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

database initialization
2 participants