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

Feature ORM for CCIP in-db prices #12813

Merged
merged 16 commits into from
May 7, 2024
Merged

Feature ORM for CCIP in-db prices #12813

merged 16 commits into from
May 7, 2024

Conversation

matYang
Copy link
Contributor

@matYang matYang commented Apr 15, 2024

Add ORM and corresponding tables for CCIP gas prices and token pries

@matYang matYang marked this pull request as ready for review May 3, 2024 05:26
@matYang matYang requested a review from a team as a code owner May 3, 2024 05:26
@matYang matYang requested a review from connorwstein May 3, 2024 05:26
@matYang matYang changed the title Feature CCIP in-db prices Feature ORM for CCIP in-db prices May 3, 2024
krehermann
krehermann previously approved these changes May 3, 2024
Copy link
Contributor

@krehermann krehermann left a 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

core/services/ccip/orm.go Outdated Show resolved Hide resolved
core/services/ccip/orm_test.go Outdated Show resolved Hide resolved
core/services/ccip/orm_test.go Outdated Show resolved Hide resolved
@matYang matYang requested a review from jmank88 May 6, 2024 05:16
core/services/ccip/orm.go Outdated Show resolved Hide resolved
core/services/ccip/orm.go Outdated Show resolved Hide resolved
"job_id": jobId,
"token_addr": price.TokenAddr,
"token_price": price.TokenPrice,
"created_at": now,
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 typically let the database set this field instead of passing it explicitly.

Copy link
Contributor Author

@matYang matYang May 7, 2024

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:

  1. DB default makes tests tricky, the get logic is dependent on sorting by timestamp, timestamp during tests is not increasing reliably
  2. delete logic is based on application's timestamp, it'd be more consistent for created_at to be based on application timestamp too

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 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

jmank88
jmank88 previously approved these changes May 6, 2024
@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
2 New Blocker Issues (required ≤ 0)

See analysis details on SonarQube

Catch issues before they fail your Quality Gate with our IDE extension SonarLint SonarLint

@matYang matYang added this pull request to the merge queue May 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 7, 2024
@matYang matYang added this pull request to the merge queue May 7, 2024
Merged via the queue into develop with commit ac89336 May 7, 2024
106 of 107 checks passed
@matYang matYang deleted the feature/ccip-in-db-prices branch May 7, 2024 05:39
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.

4 participants