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

op-signer proxy #1

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

op-signer proxy #1

wants to merge 17 commits into from

Conversation

parkgunou
Copy link
Member

Description

A clear and concise description of the features you're adding in this pull request.

Tests

Please describe any tests you've added. If you've added no tests, or left important behavior untested, please explain why not.

Additional context

Add any other context about the problem you're solving.

Metadata

  • Fixes #[Link to Issue]

op-signer/service/types.go Outdated Show resolved Hide resolved
op-signer/app.go Outdated
}

var result bool
if err := c.CallContext(ctx, &result, "opsignerproxy_serveSigner"); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we want to add some parameters to this rpc, so that we can verify the connection is being made from two compatible signer / proxy.

For example, if proxyA is running to serve for signingKey A, and this signer app is requesting connection to this proxyA's endpoint. But if the signer app is configured for signingKey B, and the proxy endpoint is misconfigured for proxyA, then we should deny the connection.

So, when the proxy receives serveSigner, it should verify that the incoming ws connection from signer is expected in the proxy configuration.

Copy link
Member

Choose a reason for hiding this comment

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

This might be too much overengineering, but if we also consider general usage of this signer for other signing keys (like batcher / proposer, other than backup sequencer), then a single signer proxy may be running with multiple signing keys configured.

Then, each of the signing request to the proxy should be routed to the correct ws connection.

Screenshot 2025-01-20 at 5 05 20 PM

very sloppy drawing...

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was to have multiple proxies as needed.

This is really a point for quite a heavy discussion and I am very glad you pointed it out.

So looking at it from the most general perspective, i think we could consider the following cases:

  • one proxy serves only one client
    • one proxy for one sequencer node
    • N proxy for N sequencer nodes, 1 each
    • N + 2 proxy for N sequencer nodes, 1 batcher, and 1 proposer, respectively
  • one proxy serves one type of client
    • one proxy for one sequencer node
    • one proxy for N sequencer nodes
    • 3 proxy for N sequencer nodes (1), 1 batcher (1), and 1 proposer (1)
  • one proxy serves all clients
    • one proxy for all the clients
  • one proxy for 1 op-signer key
    • proxy serves RPC's for a single key from the op-signer
  • one proxy for one op-signer
    • proxy serves multiple keys available on the op-signer

Currently the implementation is under the assumption that one proxy serves one type of client, attached to op-signer as a node for that client (i.e. for one dedicated key).

I think right now it is about coming up with a model that we can ship fast, with least operational costs, that is secure. We should definitely discuss the possible use cases to decide which is the best way forward!

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me! Agreed that we should make it easy enough to ship it fast.
I do think that if we intend on expand this functionality to other uses cases you described above, we should consider adding some struct to the rpc method so we can add other metadata to the rpc as neded in the future without breaking the rpc version.

Copy link
Member Author

Choose a reason for hiding this comment

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

The conclusion from our offline discussion was that we will consider one type of client per proxy, and one signer per proxy.

I do think that if we intend on expand this functionality to other uses cases you described above, we should consider adding some struct to the rpc method so we can add other metadata to the rpc as neded in the future without breaking the rpc version.

This is still to implement. Just realized I left this bit out.

op-signer/app.go Outdated Show resolved Hide resolved
op-signer/app.go Outdated Show resolved Hide resolved
@parkgunou parkgunou force-pushed the gunou/tmp-opsignerproxy-2 branch from 0beb191 to d9bf70d Compare January 21, 2025 12:55
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