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

ccip: Add unknown address types #832

Closed
wants to merge 3 commits into from
Closed

ccip: Add unknown address types #832

wants to merge 3 commits into from

Conversation

winder
Copy link
Collaborator

@winder winder commented Oct 7, 2024

Use new address types in chainlink, chainlink-common and chainlink-ccip.

Related:

@winder winder requested a review from a team as a code owner October 7, 2024 12:16
@winder winder requested review from rstout, asoliman92, makramkd, dimkouv, mateusz-sekara and 0xnogo and removed request for a team October 7, 2024 12:16
@@ -7,6 +7,26 @@ import (
"strings"
)

// UnknownAddress represents a raw address with an unknown encoding.
type UnknownAddress []byte
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking out loud, but do we need some kind of equals/compare method then? How do we want to use it in the plugin code?

func (u *UnknownAddress) Compare(other UnknownAddress, selector ChainSelector) int

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What if the unknown addresses are from two different families, would it ever make sense to compare them? I'm imagining this PR as the first step towards a final solution which has Address and EncodedAddress types which know how to convert between each other.

Maybe at that point they could be compared.

Some experiments from a couple weeks ago: smartcontractkit/chainlink-ccip#158

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, good question, I think it would be great to start with the example usage. If Message stores all addresses as UnknownAddress, it might be very cumbersome for the customer of that interface to do the casting back and forth everytime they need to compare addresses to each other.

That being said, maybe Compare/Equals should return an error when trying to compare incompatible address types? On the other hand, Message has SourceChainSelector / DestChainSelector so we should be able to automatically convert these addresses to the proper types, no?

Copy link
Collaborator Author

@winder winder Oct 7, 2024

Choose a reason for hiding this comment

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

I think it's already the case that working with Bytes is a little cumbersome. For comparison you would call the String() function, same as this new type.

This first change improves type safety and readability slightly by adding a specific type that tells you if it's encoded or not.

I'm imagining the CCIPReader would convert these to a the real address type (with validation) at which point you no longer have to worry about the chain selector or errors during encode/decode.

@winder winder changed the title ccip: Add unknown address types ccip: Use new unknown address types Oct 8, 2024
@winder winder changed the title ccip: Use new unknown address types ccip: Add unknown address types Oct 8, 2024
@winder
Copy link
Collaborator Author

winder commented Oct 8, 2024

Closed in favor of #836

@winder winder closed this Oct 8, 2024
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.

2 participants