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

feat(api/clients/v2): relay registry caller #1175

Conversation

ethenotethan
Copy link
Contributor

@ethenotethan ethenotethan commented Jan 28, 2025

Why are these changes needed?

Relay information must be ingested via EigenDA proxy. Either the user can provide this manually or just have the proxy query the on-chain EigenDARelayRegistry during initialization to bootstrap relayClient config state. Added a RelayRegistryCaller type which does the contract queries for constructing the registry state mapping.

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@ethenotethan ethenotethan changed the title feat(api): relay registry caller feat(api/clients/v2): relay registry caller Jan 28, 2025
@ethenotethan ethenotethan requested review from samlaf and litt3 January 28, 2025 18:59
@ian-shim
Copy link
Contributor

Do we need a standalone client to do this? We probably shouldn't have a standalone client for every contract call.
We already have this method in chain reader. Can we use that instead?

type RelayRegistryCaller interface {
// Reads the current sockets or key->url mapping
// from the on-chain registry
GetSockets() (map[corev2.RelayKey]string, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be called "GetSockets". I know that's the terminology used in the relay_client, but these are URLs, not sockets. Possible I'm wrong, but I've never heard a URL be called a "socket"

Copy link
Contributor

Choose a reason for hiding this comment

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

They use Socket to mean URL:port combination. Not sure whether relays register a full socket address or just a URL where default 443 port or something is used though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would call that an "endpoint". Socket really just seems like the wrong term

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Socket != SocketAddress. Maybe you could argue this map contains socket addresses, but certainly not sockets

SocketAddress does indeed contain an address and a port, but it seems to me like the term is used internally on a machine, to reference the literal sockets that have been created locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

socket generally has a technical meaning in networking, and an IP_ADDRESS:PORT string isn't a socket (although it does tell you how to open a socket). I think we should be careful about how we use the term socket.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also didn't like the term originally, and did a bunch of googling lol. Seems like its an accepted term? https://en.wikipedia.org/wiki/Network_socket#Socket_addresses

I personally find endpoint to not be precise enough, but agree that socket by itself is super confusing and nor is it anymore precise.

Copy link
Contributor

@litt3 litt3 Jan 28, 2025

Choose a reason for hiding this comment

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

It's an accepted term, but I don't think in this context. From the linked article:

This combination is often known as a socket address. It is the network-facing access handle to the network socket.

It's used in a context specifically related to interactions with network sockets.


func (rrc *relayRegistryCaller) GetSockets() (map[corev2.RelayKey]string, error) {
// read the # of relays by processing next key position
key, err := rrc.caller.NextRelayKey(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider renaming key to keyCount, to make it more clear what it is

@ethenotethan
Copy link
Contributor Author

@ian-shim wasn't aware of this chain reader type. Would prefer to use that instead since it provides read interaction with other core contracts as well and builds a dependency mapping just from the service_manager address which offloads user burden when running a proxy instance. Ty for this 🙏🏻

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.

5 participants