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

Introduce unknown address type. #248

Merged
merged 3 commits into from
Oct 22, 2024
Merged

Introduce unknown address type. #248

merged 3 commits into from
Oct 22, 2024

Conversation

winder
Copy link
Contributor

@winder winder commented Oct 17, 2024

This PR replaces the many account types (string, []byte, types.Account) with UnknownAddress and UnknownEncodedAddress. These types are now used across the CCIP code and can extended in the future to add chain agnostic encoding and validation functions.

Related chainlink changes:

@@ -31,7 +31,7 @@ jobs:
with:
repository: smartcontractkit/chainlink
# ref: develop
ref: fix-fee-quoter
Copy link
Contributor

Choose a reason for hiding this comment

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

How about making it work as a manual trigger as well? I mean overriding this value with every PR is very error prone. Maybe ability to trigger the job from GHA with custom branch from chainlink would be sufficient just to run these tests before merging? cc @b-gopalswami

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I pitched this idea before and received some pushback. I'm not against it, it should be straightforward as a flow:

  • comment on the pull request "cmd branch " to on demand run the test using the appropriate chainlink branch
  • some sort of trigger on comment with github actions. Parse the comment and ensure it comes from PR author, etc. then trigger the workflow with the branch name

@makramkd
Copy link
Collaborator

Please make sure to revert back to develop in the GH action prior to merging.

Copy link

Metric will/unknown-address main
Coverage 72.5% 72.4%

@winder winder merged commit e8564a2 into main Oct 22, 2024
4 checks passed
@winder winder deleted the will/unknown-address branch October 23, 2024 11:45
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