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

Moved Generalised Headtracker component from Core #139

Closed
wants to merge 8 commits into from

Conversation

yongkangc
Copy link
Contributor

@yongkangc yongkangc commented Jul 14, 2023

This PR moves over the Generalised HeadTracker components from core common/types repo to the chainlink-relay repo. This allows chain specific repositories to make use of the generalised Headtracker interfaces.

Changes

  • Moved over files from common/types. Moved over necessary utils and tests as well.
  • Added mock files

Note:

  • The tests will be on chain specific repository. For instance test for evm headtracker will be on chainlink-core, and test for Solana will be on chainlink-solana.
  • This PR is an updated version of the previous PR attempt to migrate ht components. Added Generalised Headtracker Components #123
  • The code is being tested here by Solana using components from chainlink-relay and all tests passes.

Future work:

  • Research to automate PR creation when there are changes on core/common and run chain specific tests of chainlink-relay headtracker

Ticket

@yongkangc yongkangc temporarily deployed to integration July 14, 2023 17:31 — with GitHub Actions Inactive
@yongkangc yongkangc temporarily deployed to integration July 14, 2023 17:34 — with GitHub Actions Inactive
@yongkangc yongkangc temporarily deployed to integration July 14, 2023 17:37 — with GitHub Actions Inactive
@yongkangc yongkangc temporarily deployed to integration July 14, 2023 17:41 — with GitHub Actions Inactive
@yongkangc yongkangc temporarily deployed to integration July 14, 2023 17:41 — with GitHub Actions Inactive
@yongkangc yongkangc temporarily deployed to integration July 14, 2023 17:41 — with GitHub Actions Inactive
@yongkangc yongkangc requested a review from aalu1418 July 14, 2023 17:42
@yongkangc yongkangc temporarily deployed to integration July 27, 2023 16:37 — with GitHub Actions Inactive
@yongkangc yongkangc temporarily deployed to integration July 27, 2023 16:37 — with GitHub Actions Inactive
@yongkangc yongkangc temporarily deployed to integration July 27, 2023 16:37 — with GitHub Actions Inactive
@yongkangc yongkangc marked this pull request as ready for review July 27, 2023 16:51
@jmank88
Copy link
Collaborator

jmank88 commented Jul 27, 2023

@ExtremelySunnyYK
Do you feel that this code is tested sufficiently when isolated in this repo? Or are we still depending on integration tests in the core repo for complete coverage?

@jmank88
Copy link
Collaborator

jmank88 commented Jul 27, 2023

The tests will be on chain specific folder. For instance EVM will have their own test for evm specific implementation, and Non-Evm will have their own test on their chain specific folder.

What do you mean by 'folder'? We will at least need module separation to prevent the imports from polluting core, and the current policy is to avoid all chain-specific imports in this repo.

@jmank88 jmank88 requested a review from chudilka1 July 27, 2023 17:19
Copy link
Collaborator

@jmank88 jmank88 left a comment

Choose a reason for hiding this comment

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

Tentatively blocking to asses whether we need to wait to be sure that CI is in a good place (tests/linter/sonar/etc.) before merging. Feel free to dismiss this if CI looks healthy and comprehensive, or if the code is literally copied without changes.
cc @chudilka1

@yongkangc
Copy link
Contributor Author

The tests will be on chain specific folder. For instance EVM will have their own test for evm specific implementation, and Non-Evm will have their own test on their chain specific folder.

What do you mean by 'folder'? We will at least need module separation to prevent the imports from polluting core, and the current policy is to avoid all chain-specific imports in this repo.

@jmank88 sorry for the lack of clarity. By folder I meant chain specific repository.

For instance test for evm headtracker will be on chainlink-core, and test for Solana will be on chainlink-solana

Will update the PR description for clarity.

@yongkangc
Copy link
Contributor Author

@ExtremelySunnyYK
Do you feel that this code is tested sufficiently when isolated in this repo? Or are we still depending on integration tests in the core repo for complete coverage?

Thanks for bringing this up @jmank88 . We will still be having to depend on integration tests in the core / chainlink-<chain> repository for integration test for coverage.

Curious if there might be a better approach?

@aalu1418 has also suggested how we can make use of GitHub automated PR in the meantime before completely moving from core/common to chainlink-relay.
This helps to keep chainlink-relay updated of changes in core so that they remain the same.

@yongkangc
Copy link
Contributor Author

Tentatively blocking to asses whether we need to wait to be sure that CI is in a good place (tests/linter/sonar/etc.) before merging. Feel free to dismiss this if CI looks healthy and comprehensive, or if the code is literally copied without changes.
cc @chudilka1

The code is copied without changes. However I will be letting this PR sit for a while for discussion with @prashantkumar1982 and @aalu1418 on the best path for dependency and testing moving forward as well. Since this is the first generalisation part to be moved over, it might highlight some issues.

@jmank88
Copy link
Collaborator

jmank88 commented Jul 27, 2023

@ExtremelySunnyYK
Do you feel that this code is tested sufficiently when isolated in this repo? Or are we still depending on integration tests in the core repo for complete coverage?

Thanks for bringing this up @jmank88 . We will still be having to depend on integration tests in the core / chainlink-<chain> repository for integration test for coverage.

Curious if there might be a better approach?

Ideally all generic code would have some dummy concrete test instances to fully cover everything. If we are going to have a bunch of related types using similar sets of generic type parameters in this repo, then it may make sense to have an internal test package that exports shared dummy types to use everywhere. This might naturally start to look like a simplified reference implementation too, which could be leveraged for docs.

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.

None yet

2 participants