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

Modules from core #88

Merged
merged 6 commits into from
Mar 9, 2023
Merged

Modules from core #88

merged 6 commits into from
Mar 9, 2023

Conversation

sdrug
Copy link
Contributor

@sdrug sdrug commented Mar 7, 2023

No description provided.

@sdrug sdrug temporarily deployed to integration March 7, 2023 22:46 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 7, 2023 22:46 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 7, 2023 22:51 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 7, 2023 22:51 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 8, 2023 15:14 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 8, 2023 15:14 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 8, 2023 15:45 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 8, 2023 15:45 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 8, 2023 16:25 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 8, 2023 16:25 — with GitHub Actions Inactive
@sdrug sdrug marked this pull request as ready for review March 8, 2023 16:26
@sdrug sdrug requested a review from krebernisak as a code owner March 8, 2023 16:26
@aalu1418 aalu1418 self-requested a review March 8, 2023 17:15
Copy link
Contributor

@aalu1418 aalu1418 left a comment

Choose a reason for hiding this comment

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

Can you copy over the test files for the utils that you copied over too?

Also to fix the failing solana CI check I think you can remove this line: https://github.com/smartcontractkit/chainlink-relay/blob/0298f0e913fe98bcbf2d5a4c95f33b08a968dd59/.github/workflows/build_external.yml#L62

@@ -31,3 +31,12 @@ type ErrEmpty struct {
func (e ErrEmpty) Error() string {
return fmt.Sprintf("%s: empty: %s", e.Name, e.Msg)
}

type KeyNotFoundError struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How will this be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's being used in the txm tests, for example here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm OK - we might have to revisit this with plugins, since error types are not preserved over the wire. Fortunately it seems like we could just return nil instead of a custom error for this particular case.

@sdrug sdrug temporarily deployed to integration March 9, 2023 00:36 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 9, 2023 00:36 — with GitHub Actions Inactive
@sdrug sdrug force-pushed the modules-from-core branch from c835d4c to d13c50d Compare March 9, 2023 01:09
@sdrug sdrug temporarily deployed to integration March 9, 2023 01:09 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 9, 2023 01:09 — with GitHub Actions Inactive
@sdrug
Copy link
Contributor Author

sdrug commented Mar 9, 2023

@aalu1418
I think the CI is failing because it cannot build chainlink-solana (develop branch?) with the golang version lower than 1.20. This should help.

Copy link
Contributor

@aalu1418 aalu1418 left a comment

Choose a reason for hiding this comment

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

lgtm! I will take a look at fixing the CI runs in a separate PR

@sdrug sdrug temporarily deployed to integration March 9, 2023 14:44 — with GitHub Actions Inactive
@sdrug sdrug temporarily deployed to integration March 9, 2023 14:44 — with GitHub Actions Inactive
@sdrug
Copy link
Contributor Author

sdrug commented Mar 9, 2023

Merging this part of work for now. The further improvements will follow in Aaron's PR

@sdrug sdrug merged commit 99c7689 into main Mar 9, 2023
@sdrug sdrug deleted the modules-from-core branch March 9, 2023 15:17
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.

3 participants