-
Notifications
You must be signed in to change notification settings - Fork 0
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
chore: add serial id #92
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would also need to update parts in spark deal submitter where we look for unsubmitted deals and mark deals as submitted before merging this.
Is there a certain reason we did not use |
No there is not, I guess that should be also added. |
@@ -10,6 +10,7 @@ const PayloadRetrievabilityState = { | |||
const PayloadRetrievabilityStateType = Type.Enum(PayloadRetrievabilityState) | |||
|
|||
const ActiveDealDbEntry = Type.Object({ | |||
id: Type.Optional(Type.Number()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be missing to make this type required (which should be safer)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I right in my understanding that we need this to be optional so that when we create a new entry from the block event (in backend/lib/utils.js) we can keep it empty?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to be strict about types, then there should be two types:
- "NewActiveDealDbEntry" that does not have any ID value - we want the database to generate the value for us
- "ActiveDealDbEntry" where the ID value is required (always present)
I don't know if we need or want to go that far 🤷🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In TypeScript, it's possible to create a new type from another type by omitting certain properties - see https://www.typescriptlang.org/docs/handbook/utility-types.html#omittype-keys.
type NewActiveDealDbEntry = Omit<ActiveDealDbEntry, "id">
That obviously does not provide runtime validation, so if we want to go down the route of two types, then we need to find a solution based on Typebox features.
deal.term_min, | ||
deal.term_max, | ||
deal.sector_id | ||
deal.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we're updating the UPDATE query, however I'm not seeing a matching SELECT query update. Are we using a SELECT *, or is that part missing?
I believe this is what I discovered in #92 (comment) as well. Can you please add a test @NikolasHaimerl that shows that this fails, and then follow up with a fix? |
The table
active_deals
does not have a unique property right now. This makes it tedious to fetch deals as one needs to check each field to a specific value to retrieve it. We can make this shorter by introducing a serial and set it as the primary key of the tableactive_deals
. Consequently, we only need to ask for the primary key if we want to fetch or update a certain deal.This PR adds a serial to the active deals table which is then set as the primary key with the column name
id
.Closes #86