-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Feature ORM for CCIP in-db prices #12813
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.
lgtm to the small nit about the test
"job_id": jobId, | ||
"token_addr": price.TokenAddr, | ||
"token_price": price.TokenPrice, | ||
"created_at": now, |
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.
I think we typically let the database set this field instead of passing it explicitly.
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.
my understanding is there isn't a single best practice when it comes to passing in timestamp v.s DB default, in this case:
- DB default makes tests tricky, the get logic is dependent on sorting by timestamp, timestamp during tests is not increasing reliably
- delete logic is based on application's timestamp, it'd be more consistent for created_at to be based on application timestamp too
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.
I think of best practice as using db clock for both inserts and deletes rather than golang clock.
now()
is an alias for transaction_timestamp()
which returns the start time of the current transaction. That does have issues with tests, because we run all tests within a single transaction. However, I think any of the following should probably work without issues: statement_timestamp()
, clock_timestamp()
, or just TIMESTAMP 'now'
https://www.postgresql.org/docs/current/functions-datetime.html#FUNCTIONS-DATETIME-CURRENT
Quality Gate failedFailed conditions See analysis details on SonarQube Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Add ORM and corresponding tables for CCIP gas prices and token pries