Move chain agnostic sql types in core/store/models to chainlink-common/sqlutil/models

[reductionista]

This moves the chain agnostic db types into chainlink-common, and evm-specific types from models into models/evm to match models/solana & models/cosmos. (Soon, all of these should be moved into their respective relays.)

(This will allow for a cleaner extraction of evm chainreader, since it depends on the type models.Interval, which it can now import from chainlink-common.)

Requires Dependencies

• https://github.com/smartcontractkit/chainlink-common/pull/665

[github-actions[bot]]

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:

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

:medal_military: No JIRA issue number found in: PR title, commit message, or branch name. Please include the issue ID in one of these.

[jmank88]

models.Interval

Are other types used? Or just this one?

This moves the chain agnostic db types into chainlink-common

I think this is too broad of a rule. Yes, common must remain chain agnostic, but it doesn't follow that everything chain agnostic should move to common. Core does a lot of chain-agnostic things that do not need to leak out of that module at all. Common is reserved for things that are common to everything.

[cl-sonarqube-production[bot]]

Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

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

See analysis details on SonarQube

[reductionista]

models.Interval

Are other types used? Or just this one?

This moves the chain agnostic db types into chainlink-common

I think this is too broad of a rule. Yes, common must remain chain agnostic, but it doesn't follow that everything chain agnostic should move to common. Core does a lot of chain-agnostic things that do not need to leak out of that module at all. Common is reserved for things that are common to everything.

Interval was the only one I needed for extracting ChainReader, but looking over the rest all of them except Cron looked like things that we'll probably need at some point to import from the relays. They should be available to any code that needs to read and write from a database where these types might be part of the schema. Otherwise, anyone who needs to add a column of one of these types (eg, JSON types or URL's) will be faced with the choice of duplicating their own custom types or having to raise a new PR to move them over into common. I can already guess that nobody is going to bother moving them if we don't put them there now, because it will be too big of a pain to do one at a time. (And then we'll end up with a re-implementation of the same type in slightly different ways in every repo rather than just letting them all share the same one.)

I think you have a point when it comes to the Cron type, it will probably only ever be needed by chainlink repo--we can leave that there, I just moved it with the rest since I figured it doesn't hurt and it's kind of weird moving everything but one type.

Here is the full list of types that are moved to common in this PR, for reference:

core/store/models/common.go:

// JSON stores the json types string, number, bool, and null.
// Arrays and Objects are returned as their raw json types.
type JSON struct {
       gjson.Result
}

// WebURL contains the URL of the endpoint.
type WebURL url.URL

// Cron holds the string that will represent the spec of the cron-job.
type Cron string

// Interval represents a time.Duration stored as a Postgres interval type
type Interval time.Duration

core/store/models/errors.go:

// JSONAPIErrors holds errors conforming to the JSONAPI spec.
type JSONAPIErrors struct {
       Errors []JSONAPIError `json:"errors"`
}

// JSONAPIError is an individual JSONAPI Error.
type JSONAPIError struct {
       Detail string `json:"detail"`
}

core/store/models/secrets.go:

// Secret is a string that formats and encodes redacted, as "xxxxx".
//
// Use Value to get the actual secret.
type Secret string

// SecretURL is a URL that formats and encodes redacted, as "xxxxx".
type SecretURL commonconfig.URL

[reductionista]

Oh, and it also moves these helper functions from cltest to commontest (chainlink-common/pkg/utils/tests), which is needed in order for the tests on the json types to run in chainilnk-common:

// JSONFromString create JSON from given body and arguments
func JSONFromString(t testing.TB, body string, args ...interface{}) models.JSON {
      return JSONFromBytes(t, []byte(fmt.Sprintf(body, args...)))
}

// JSONFromBytes creates JSON from a given byte array
func JSONFromBytes(t testing.TB, body []byte) models.JSON {
         j, err := models.ParseJSON(body)
       require.NoError(t, err)
       return j
}

func MustJSONMarshal(t *testing.T, val interface{}) string {
       t.Helper()
       bs, err := json.Marshal(val)
       require.NoError(t, err)
       return string(bs)
}

For the same reason, I expect these will be useful for many different repos to be able to import

[jmank88]

things that we'll probably need at some point to import from the relays.

This is not the right place for things common to relays. There will be a common integrations module for that.

[reductionista]

This is not the right place for things common to relays. There will be a common integrations module for that.

Oh okay, I didn't realize that. If I recall, it started out as exactly that when when it was named chainlink-relay... but has gradually incorporated an assortment of other shared packages as well. Last I recall seeing, it still has a lot of things common to the relays which they import--is the plan to move all of that into the integrations repo at some point?

[jmank88]

This is not the right place for things common to relays. There will be a common integrations module for that.

Oh okay, I didn't realize that. If I recall, it started out as exactly that when when it was named chainlink-relay... but has gradually incorporated an assortment of other shared packages as well. Last I recall seeing, it still has a lot of things common to the relays which they import--is the plan to move all of that into the integrations repo at some point?

The plan was to move chainlink/common to chainlink-integrations/common. I don't know if the location changes now though :shrug:

[reductionista]

The plan was to move chainlink/common to chainlink-integrations/common. I don't know if the location changes now though 🤷

I do remember seeing some things there, come to think of it. If all the relay modules are in that repo though, importing this common one, won't we have that issue with using replace you mentioned last week?

[jmank88]

The plan was to move chainlink/common to chainlink-integrations/common. I don't know if the location changes now though 🤷

I do remember seeing some things there, come to think of it. If all the relay modules are in that repo though, importing this common one, won't we have that issue with using replace you mentioned last week?

Using a relative replace is always a choice. It is never necessary.