core/services/keystore: switch to sqlutil.DataStore #12810

jmank88 · 2024-04-13T14:47:32Z

https://smartcontract-it.atlassian.net/browse/BCF-2978

Requires:

pkg/sqlutil: fix WrapDataSource <> TimeoutHook interaction chainlink-common#477

github-actions · 2024-04-13T14:47:48Z

I see you updated files related to core. Please run pnpm changeset in the root directory to add a changeset as well as in the text include at least one of the following tags:

#nops : For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#added : For any new functionality added.
#changed : For any change to the existing functionality. 
#removed : For any functionality/config that is removed.
#updated : For any functionality that is updated.
#deprecation_notice : For any upcoming deprecation functionality.
#breaking_change : For any functionality that requires manual action for the node to boot.
#db_update : For any feature that introduces updates to database schema.
#wip : For any change that is not ready yet and external communication about it should be held off till it is feature complete.
#bugfix - For bug fixes.
#internal - For changesets that need to be excluded from the final changelog.

jmank88 · 2024-04-17T11:29:09Z

The integration tests are failing on ensure X key.

patrickhuie19 · 2024-04-19T16:42:56Z

core/services/keystore/cosmos_test.go

 	ks := keyStore.Cosmos()
 	reset := func() {
+		ctx := context.Background() // Executed during cleanup


Would it benefit to be testutils.Context(t)? I'm not 100% on my understanding of how go test cleanup operates, but i've had some friction with debugging tests that hang/fail in cleanup vs in the test execution.

No it can't be a normal ctx since we execute it during t.Cleanup, when the context may already be done.

patrickhuie19 · 2024-04-19T16:47:13Z

core/services/keystore/eth.go

-	if err := q.Get(state, sql, address, chainID.String()); err != nil {
-		return errors.Wrap(err, "failed to insert evm_key_state")
+	if err := ds.GetContext(ctx, state, sql, address, chainID.String()); err != nil {
+		return errors.Wrap(err, "failed to insert key_state")
 	}
 	// consider: do we really need a cache of the keystates?


I still think we don't 😛

patrickhuie19 · 2024-04-19T16:52:29Z

core/services/keystore/eth_test.go

@@ -27,18 +29,20 @@ import (
 func Test_EthKeyStore(t *testing.T) {
 	t.Parallel()

-	db := pgtest.NewSqlxDB(t)
-	cfg := configtest.NewTestGeneralConfig(t)
+	db := sqlutil.WrapDataSource(pgtest.NewSqlxDB(t), logger.Test(t), sqlutil.TimeoutHook(func() time.Duration {


Why wrap pgtest.NewSqlxDb(t)?

Just mimicking production. This is where I debugged the issue.

it's worth a code comment because it's not obvious.

or should the test db impl be changed to return the wrapper?

Yeah, in the final PR #12456 I updated cltest at least to use a timeout wrapper. We could consider updating the test funcs to return a wrapped DataSource instead of raw sqlx most places 🤷

patrickhuie19 · 2024-04-19T17:08:05Z

core/web/jobs_controller_test.go

@@ -477,7 +480,6 @@ targets:
 func TestJobsController_Create_WebhookSpec(t *testing.T) {
 	app := cltest.NewApplicationEVMDisabled(t)
 	require.NoError(t, app.Start(testutils.Context(t)))
-	t.Cleanup(func() { assert.NoError(t, app.Stop()) })


nice - i've seen similar cleanup needed in helpers in mercury and llo. Maybe worth creating a ticket to audit (good for a new joiner?)

cl-sonarqube-production · 2024-04-19T18:29:04Z

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
92.2% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube

jmank88 temporarily deployed to sdlc April 13, 2024 14:47 — with GitHub Actions Inactive

jmank88 mentioned this pull request Apr 13, 2024

use sqlutil instead of pg.QOpts/Q/Queryer #12456

Merged

1 task

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 98d889f to 2919c97 Compare April 13, 2024 15:28

jmank88 temporarily deployed to sdlc April 13, 2024 15:28 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 2919c97 to 3ac208c Compare April 13, 2024 17:56

jmank88 temporarily deployed to sdlc April 13, 2024 17:56 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 3ac208c to 6e7cdd8 Compare April 13, 2024 17:58

jmank88 temporarily deployed to sdlc April 13, 2024 17:58 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 6e7cdd8 to 23004be Compare April 14, 2024 04:33

jmank88 temporarily deployed to sdlc April 14, 2024 04:33 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 23004be to 8c7290d Compare April 14, 2024 12:40

jmank88 temporarily deployed to sdlc April 14, 2024 12:40 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 8c7290d to 6a01834 Compare April 15, 2024 14:42

jmank88 temporarily deployed to sdlc April 15, 2024 14:42 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 6a01834 to e1c5253 Compare April 16, 2024 20:40

jmank88 temporarily deployed to sdlc April 16, 2024 20:40 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from e1c5253 to 2ebc5b1 Compare April 16, 2024 20:47

jmank88 temporarily deployed to sdlc April 16, 2024 20:48 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 2ebc5b1 to d695fde Compare April 16, 2024 23:48

jmank88 temporarily deployed to sdlc April 16, 2024 23:48 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from d695fde to 804f45d Compare April 17, 2024 00:31

jmank88 temporarily deployed to sdlc April 17, 2024 00:31 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from 804f45d to e69cd4e Compare April 17, 2024 19:53

jmank88 temporarily deployed to sdlc April 17, 2024 19:53 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from e69cd4e to 0ad0eb5 Compare April 17, 2024 20:09

jmank88 temporarily deployed to sdlc April 17, 2024 20:09 — with GitHub Actions Inactive

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch 2 times, most recently from 551d1d7 to f2b38f3 Compare April 17, 2024 20:58

patrickhuie19 reviewed Apr 19, 2024

View reviewed changes

jmank88 requested a review from patrickhuie19 April 19, 2024 18:03

core/services/keystore: switch to sqlutil.DataStore

33be896

jmank88 force-pushed the BCF-2978-rm-qopts-keystore branch from a44dc6f to 33be896 Compare April 19, 2024 18:13

jmank88 temporarily deployed to sdlc April 19, 2024 18:13 — with GitHub Actions Inactive

samsondav approved these changes Apr 19, 2024

View reviewed changes

jmank88 added this pull request to the merge queue Apr 19, 2024

Merged via the queue into develop with commit 1fce16e Apr 19, 2024
105 checks passed

jmank88 deleted the BCF-2978-rm-qopts-keystore branch April 19, 2024 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core/services/keystore: switch to sqlutil.DataStore #12810

core/services/keystore: switch to sqlutil.DataStore #12810

jmank88 commented Apr 13, 2024 •

edited

github-actions bot commented Apr 13, 2024

jmank88 commented Apr 17, 2024

patrickhuie19 Apr 19, 2024

jmank88 Apr 19, 2024

patrickhuie19 Apr 19, 2024

patrickhuie19 Apr 19, 2024

jmank88 Apr 19, 2024

krehermann Apr 19, 2024

jmank88 Apr 19, 2024

patrickhuie19 Apr 19, 2024

cl-sonarqube-production bot commented Apr 19, 2024

core/services/keystore: switch to sqlutil.DataStore #12810

core/services/keystore: switch to sqlutil.DataStore #12810

Conversation

jmank88 commented Apr 13, 2024 • edited

github-actions bot commented Apr 13, 2024

jmank88 commented Apr 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cl-sonarqube-production bot commented Apr 19, 2024

Quality Gate passed

jmank88 commented Apr 13, 2024 •

edited