-
Notifications
You must be signed in to change notification settings - Fork 0
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
base: main
Are you sure you want to change the base?
Conversation
op-signer/app.go
Outdated
} | ||
|
||
var result bool | ||
if err := c.CallContext(ctx, &result, "opsignerproxy_serveSigner"); err != nil { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
very sloppy drawing...
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
0beb191
to
d9bf70d
Compare
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