adapt to teleporter v1.0.3 #373
Conversation
feuGeneA
force-pushed
the
teleporter-v1.0.2
branch
from
0ae0e41 to
9984b7b
Compare
because the go.mod version string is assumed to be a github ref, eg to be checked out during CI's e2e workflow, but currently we're using a reference that was created by using `go get` with a branch name, which resolves to a pseudo version in the go.mod file, which doesn't refer to any git ref in the subnet-evm repo.
feuGeneA
force-pushed
the
teleporter-v1.0.2
branch
from
047e0bc to
5a8ddd0
Compare
feuGeneA
force-pushed
the
teleporter-v1.0.2
branch
from
5a8ddd0 to
1d0c408
Compare
feuGeneA
requested review from
iansuvak,
geoff-vball,
minghinmatthewlam,
bernard-avalabs,
michaelkaplan13 and
cam-schultz
relayer/application_relayer.go
Outdated
| sentTo := r.network.Network.Send(outMsg, vdrSet, r.sourceBlockchain.GetSubnetID(), subnets.NoOpAllower) | ||
| sentTo := r.network.Network.Send( | ||
| outMsg, | ||
| // TODO: consider whether we need to specify the other |
There was a problem hiding this comment.
My understanding is that the other fields are used to specify the number of additional nodes to sample. Since we are making requests to specific nodes explicitly, I don't think additional sampling is necessary.
There was a problem hiding this comment.
thanks for the input, removed the TODO in e0794af
peers/info_client.go
Outdated
| addrPort, err := i.client.GetNodeIP(ctx, i.options...) | ||
| return addrPort.Addr().String(), err |
There was a problem hiding this comment.
Let's have InfoAPI continue to be a passthrough to InfoClient, and have GetNodeIP have the same return values as InfoClient's method.
There was a problem hiding this comment.
addressed in 641e532
|
I made an issue for this here #376 |
addresses review comment #373 (comment)
addresses review comment #373 (comment)
| @@ -47,7 +48,7 @@ func (i *InfoAPI) GetNodeID(ctx context.Context) (ids.NodeID, *signer.ProofOfPos | |||
| return i.client.GetNodeID(ctx, i.options...) | |||
| } | |||
|
|
|||
| func (i *InfoAPI) GetNodeIP(ctx context.Context) (string, error) { | |||
| func (i *InfoAPI) GetNodeIP(ctx context.Context) (netip.AddrPort, error) { | |||
There was a problem hiding this comment.
Are we able to change the name of this function, or is it part of an interface?
There was a problem hiding this comment.
It's not part of an interface, but our InfoAPI type is written as an extension to InfoClient that embeds the rpc options in the object, rather than leaving it to the caller. As such, matching the InfoClient interface makes the code more readable.
What's the motivation for changing the function name?
There was a problem hiding this comment.
The function is named GetNodeIP but we're returning both the IP and the port
There was a problem hiding this comment.
My preference would be to keep parity with the underlying client interface, even if the name could be improved. That greatly helps code discovery when referencing existing usages as examples.
addresses review comment ava-labs/icm-services#373 (comment)
addresses review comment ava-labs/icm-services#373 (comment)
Why this should be merged
How this works
How this was tested
How is this documented