ccip: Add unknown address types #836
Conversation
winder
requested review from
rstout,
asoliman92,
makramkd,
dimkouv,
mateusz-sekara and
0xnogo
and removed request for
a team
winder
force-pushed
the
will/unknown-addr-2
branch
from
140eb71
to
7a0b494
Compare
Remove mustNewBytes Remove mustNewBytes
winder
force-pushed
the
will/unknown-addr-2
branch
from
7a0b494
to
f495481
Compare
| } | ||
|
|
||
| var res Bytes32 | ||
| copy(res[:], b) |
There was a problem hiding this comment.
Would we want to put the bytes at the end of the array?
ie. should the result of the conversion be "0x1234" -> "0x000...01234" rather than "0x1234" -> "0x12340000..."?
There was a problem hiding this comment.
The former seems to be more of what I would expect, at least if the hex bytes represented a number
There was a problem hiding this comment.
This code isn't changed from v1. There aren't many places using Bytes32 after this change, we would have to review those to see if it makes sense to modify the behavior.
I guess my expectation would be s == NewBytes32FromString(s).String() which definitely isn't the case with the current implementation which also silently truncates the input.
| } | ||
|
|
||
| func (s SeqNumRange) String() string { | ||
| return fmt.Sprintf("[%d -> %d]", s[0], s[1]) |
There was a problem hiding this comment.
Seems like the SeqNumRange is inclusive of the endpoints, let's include that in a comment somewhere.
Do we have a preference for this specific arrow notation instead of math notation [start, end]?
|
|
||
| // String returns the hex representation of the unknown address. | ||
| func (a UnknownAddress) String() string { | ||
| return Bytes(a).String() |
There was a problem hiding this comment.
Should we prefix this by something to make it clear that its unknown?
| return Bytes(a).String() | |
| return fmt.Sprintf("unkown_%s", Bytes(a).String()) |
There was a problem hiding this comment.
Should unknown addresses string representations ever be passed in somewhere that will make use of that representation (e.g like the contract reader)?
There was a problem hiding this comment.
Yeah, that would be great.
There was a problem hiding this comment.
I would rather not attach semantics to the string representation. It's mainly kept here for convenience and backwards compatibility.
There was a problem hiding this comment.
It's part of the CCIP message, so I guess we need to keep it without a prefix.
| } | ||
|
|
||
| func (a UnknownAddress) MarshalJSON() ([]byte, error) { | ||
| return Bytes(a).MarshalJSON() |
| } | ||
|
|
||
| var res Bytes32 | ||
| copy(res[:], b) |
There was a problem hiding this comment.
The former seems to be more of what I would expect, at least if the hex bytes represented a number
There was a problem hiding this comment.
Does this API surface need to be exported? Who will use it?
There was a problem hiding this comment.
Why do these types use json struct tags? Are we mixing layers of the API?
Introduce new address types in a backwards compatible way.