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.

Sign up for GitHub

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

Jump to bottom

NONEVM-745 LogPoller db models #921

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

NONEVM-745 LogPoller db models #921

wants to merge 8 commits into from

Conversation

dhaidashenko
Copy link
Contributor

@dhaidashenko dhaidashenko commented Nov 8, 2024 •
edited
Loading

@dhaidashenko dhaidashenko requested a review from a team as a code owner November 8, 2024 18:58
@reductionista
Copy link
Contributor

I was thinking for this JIRA we would add a definition for LogPollerBlock and the LogPoller interface as well. I guess those would go in a separate file than models.go though (logpoller.go or maybe types.go?). Type aliases for solana.PublicKey, solana.Signature, and solana.Hash should go here though.

@dhaidashenko dhaidashenko requested a review from a team as a code owner November 13, 2024 19:27
@reductionista
Copy link
Contributor

Wow! It looks like you did a lot more work here than was necessary at present. That's great to see, although I'm a bit confused as to why this is all in this PR as opposed to separate PR's.

It says NONEVM-745 in the title, but it looks like a combination of that, NONEVM-746, and some of NONEVM-744:

https://smartcontract-it.atlassian.net/browse/NONEVM-744
https://smartcontract-it.atlassian.net/browse/NONEVM-745
https://smartcontract-it.atlassian.net/browse/NONEVM-746

NONEVM-746 (Implementation of the ORM) has been blocked on the db prod + unit testing PR's I'm working on. But it looks like you've managed to work around that for now?

reductionista
reductionista reviewed Nov 14, 2024
View reviewed changes
pkg/solana/logpoller/orm_test.go
dbURL, ok := os.LookupEnv("CL_DATABASE_URL")
require.True(t, ok, "CL_DATABASE_URL must be set")
chainID := uuid.NewString()
dbx := pg.NewSqlxDB(t, dbURL)
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 really confused... I have a PR open to move NewSqlDB and some other things over from chainlink to chainlink-common, but as far as I know it hasn't been merged yet. 🤔

I wonder if someone else moved it over already.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's not merged. I've used your PR as a dependency to unblock myself.

@reductionista
Copy link
Contributor

Could you add links in the description here and in smartcontractkit/chainlink#15171 to each other and any other PR's they depend on?

@dhaidashenko
Copy link
Contributor Author

Wow! It looks like you did a lot more work here than was necessary at present. That's great to see, although I'm a bit confused as to why this is all in this PR as opposed to separate PR's.

It says NONEVM-745 in the title, but it looks like a combination of that, NONEVM-746, and some of NONEVM-744:

https://smartcontract-it.atlassian.net/browse/NONEVM-744 https://smartcontract-it.atlassian.net/browse/NONEVM-745 https://smartcontract-it.atlassian.net/browse/NONEVM-746

NONEVM-746 (Implementation of the ORM) has been blocked on the db prod + unit testing PR's I'm working on. But it looks like you've managed to work around that for now?

Yeah, I had to do some additional work that exceeds the scope of the NONEVM-745 to ensure that we can actually read/write into defined models from the database.
But this PR does not fully address other tickets that you've referenced. I'm not sure if it makes sense to split this work into more granular ones, given it's still a manageable size and that we are on a tight schedule.

@dhaidashenko dhaidashenko mentioned this pull request Nov 14, 2024
Draft
@reductionista reductionista mentioned this pull request Nov 14, 2024
Merged
@dhaidashenko dhaidashenko mentioned this pull request Nov 21, 2024
Open
@dhaidashenko
Merge branch 'develop' into feature/NONEVM-745-logpoller-db-models
5688156 
# Conflicts:
#	go.mod
#	go.sum
#	integration-tests/go.mod
#	integration-tests/go.sum
@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 75%)

See analysis details on SonarQube

@EasterTheBunny EasterTheBunny mentioned this pull request Dec 4, 2024
Open
reductionista
reductionista reviewed Dec 7, 2024
View reviewed changes
pkg/solana/logpoller/query.go
"fmt"
"strings"
"time"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

While reviewing @EasterTheBunny 's DSL Parsing PR, I noticed we're missing unit tests for the functions in this file. In evm we have a Test_QueryArgs set of tests that runs through these in a query_test.go file. Should we add something similar here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reductionista reductionista reductionista left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dhaidashenko @reductionista