make ecdsa optional for certain operations #522

shrimalmadhur · 2024-04-25T21:24:42Z

Why are these changes needed?

This makes ECDSA optional for list-quorums
It re-orders some codebase but ecdsa keys are too deep tied to make it pretty. I am also thinking a bit about future where folks wants this util just to generate payload without the need of ecdsa key and then they can use smart contract or cold key to sign some other way.

Checks

I've made sure the lint is passing in this PR.
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.
Testing Strategy
- Unit tests
- Integration tests
- This PR is not tested :(

shrimalmadhur · 2024-04-25T22:43:12Z

node/plugin/cmd/main.go

@@ -117,6 +124,16 @@ func pluginOps(ctx *cli.Context) {
 		}
 	}

+	if config.Operation == plugin.OperationListQuorums {


putting this operation before operator is initialized since that require us to have private key. I am making minimal changes to make sure we make ecdsa optional here too.

node/plugin/cmd/main.go

shrimalmadhur · 2024-04-25T23:10:32Z

node/plugin/cmd/main.go

@@ -117,6 +124,21 @@ func pluginOps(ctx *cli.Context) {
 		}
 	}

+	if config.Operation == plugin.OperationListQuorums {
+		operatorAddress, err := tx.OperatorIDToAddress(context.Background(), operatorID)


maybe we should just use this to get address for all cases and not rely on private key?

node/plugin/cmd/main.go

ian-shim · 2024-04-25T23:44:09Z

node/plugin/cmd/main.go

@@ -117,6 +124,21 @@ func pluginOps(ctx *cli.Context) {
 		}
 	}

+	if config.Operation == plugin.OperationListQuorums {


why don't we put this block before declaring sk and privateKey? Then we don't need the block in L77, right?

I thought about that but the reason is - this still require transactor which requires ethConfig which requires privateKey - well privateKey is just passed as config. One way is I create ethConfig without privateKey and if we get to a point where privateKey is needed for other ops and we read ecdsa key then I can add that config. would that be better?

oh but that won't work since transactor will already be created by then. I don't think we want to create transactor again.

node/plugin/cmd/main.go

node/plugin/config.go

jianoaix

Does this separation actually make sense? like this is going to be the same binary for both mutation and read-only operations, unless there is use case setting up for only list-quorums

node/plugin/cmd/main.go

shrimalmadhur · 2024-04-26T00:06:01Z

Does this separation actually make sense? like this is going to be the same binary for both mutation and read-only operations, unless there is use case setting up for only list-quorums

yea why not? I am assuming there are multiple types of operators - one who is probably okay using ecdsa key in this, other might want to just create payloads/ list quorums - which doesn't require ecdsa keys. I don't think so we want diff binary for these use cases as that will probably be more work on their end and having multiple binaries will only create more confusion

jianoaix · 2024-04-26T16:10:24Z

Does this separation actually make sense? like this is going to be the same binary for both mutation and read-only operations, unless there is use case setting up for only list-quorums

yea why not? I am assuming there are multiple types of operators - one who is probably okay using ecdsa key in this, other might want to just create payloads/ list quorums - which doesn't require ecdsa keys. I don't think so we want diff binary for these use cases as that will probably be more work on their end and having multiple binaries will only create more confusion

The list-quorums is intended to be used closely with opt-in/out: the opt-in / out are essentially "adding quorum" and "removing quorum", and list-quorums is like a GPS telling them where they are right now, so they can be informed to add/remove quorums.

jianoaix · 2024-04-26T16:15:57Z

The other way to look at it: if there are more read-only operations to add, the code here will be bloated with a lot such if-else catches to support them.

So my view on this is:

Read-only operations closely tied to mutation operations as supporting tools: In this case they are intended to be used together, so no separation needed
Read-only operations as a separate toolkit which may not be just used to support mutation operations: in this case, it'll make more sense to separate a binary/plugin (with its own flags, which will be less than mutation), so code will be cleaner and more maintainable

shrimalmadhur · 2024-04-26T17:52:34Z

The other way to look at it: if there are more read-only operations to add, the code here will be bloated with a lot such if-else catches to support them.

So my view on this is:

Read-only operations closely tied to mutation operations as supporting tools: In this case they are intended to be used together, so no separation needed

Read-only operations as a separate toolkit which may not be just used to support mutation operations: in this case, it'll make more sense to separate a binary/plugin (with its own flags, which will be less than mutation), so code will be cleaner and more maintainable

I still don't like the idea of two binaries per AVS - more binaries will make more effort on us to maintain plus on operators to use, plus other AVS might follow that and it might not be a good idea.
But I understand that we do have bloated code and we need to make clear separation. This could de solved by single binary but having different sub commands which are related. This is what standard CLI does.

Also as a probably a longer term - we should probably re-think nodeplugin - as in it can be just a binary which operators can download instead of docker image.

jianoaix · 2024-04-29T20:10:21Z

The other way to look at it: if there are more read-only operations to add, the code here will be bloated with a lot such if-else catches to support them.
So my view on this is:

Read-only operations closely tied to mutation operations as supporting tools: In this case they are intended to be used together, so no separation needed

Read-only operations as a separate toolkit which may not be just used to support mutation operations: in this case, it'll make more sense to separate a binary/plugin (with its own flags, which will be less than mutation), so code will be cleaner and more maintainable

I still don't like the idea of two binaries per AVS - more binaries will make more effort on us to maintain plus on operators to use, plus other AVS might follow that and it might not be a good idea. But I understand that we do have bloated code and we need to make clear separation. This could de solved by single binary but having different sub commands which are related. This is what standard CLI does.

Also as a probably a longer term - we should probably re-think nodeplugin - as in it can be just a binary which operators can download instead of docker image.

What is the subcommand approach? "list-quorums" is separate already from "opt-in"?

shrimalmadhur · 2024-04-30T15:19:05Z

The other way to look at it: if there are more read-only operations to add, the code here will be bloated with a lot such if-else catches to support them.
So my view on this is:

Read-only operations closely tied to mutation operations as supporting tools: In this case they are intended to be used together, so no separation needed

Read-only operations as a separate toolkit which may not be just used to support mutation operations: in this case, it'll make more sense to separate a binary/plugin (with its own flags, which will be less than mutation), so code will be cleaner and more maintainable

I still don't like the idea of two binaries per AVS - more binaries will make more effort on us to maintain plus on operators to use, plus other AVS might follow that and it might not be a good idea. But I understand that we do have bloated code and we need to make clear separation. This could de solved by single binary but having different sub commands which are related. This is what standard CLI does.
Also as a probably a longer term - we should probably re-think nodeplugin - as in it can be just a binary which operators can download instead of docker image.

What is the subcommand approach? "list-quorums" is separate already from "opt-in"?

I mean right now it's not needed. but eventually if we add more commands, we might be able to group commands. Right now they are fine.

jianoaix · 2024-04-30T15:56:41Z

The other way to look at it: if there are more read-only operations to add, the code here will be bloated with a lot such if-else catches to support them.
So my view on this is:

Read-only operations closely tied to mutation operations as supporting tools: In this case they are intended to be used together, so no separation needed

Read-only operations as a separate toolkit which may not be just used to support mutation operations: in this case, it'll make more sense to separate a binary/plugin (with its own flags, which will be less than mutation), so code will be cleaner and more maintainable

I still don't like the idea of two binaries per AVS - more binaries will make more effort on us to maintain plus on operators to use, plus other AVS might follow that and it might not be a good idea. But I understand that we do have bloated code and we need to make clear separation. This could de solved by single binary but having different sub commands which are related. This is what standard CLI does.
Also as a probably a longer term - we should probably re-think nodeplugin - as in it can be just a binary which operators can download instead of docker image.

What is the subcommand approach? "list-quorums" is separate already from "opt-in"?

I mean right now it's not needed. but eventually if we add more commands, we might be able to group commands. Right now they are fine.

Is using "list-quorums" on a separate machine than the one using opt-in/out needed right now?

shrimalmadhur · 2024-04-30T16:01:41Z

Is using "list-quorums" on a separate machine than the one using opt-in/out needed right now?

It could be - what if operator opts-in using smart contract operator (or other ways where ecdsa key is not in same machine?) and want to list quorums? They should be able to run this without ecdsa key and password

jianoaix · 2024-04-30T17:14:51Z

Is using "list-quorums" on a separate machine than the one using opt-in/out needed right now?

It could be - what if operator opts-in using smart contract operator (or other ways where ecdsa key is not in same machine?) and want to list quorums? They should be able to run this without ecdsa key and password

I don't even see there is a solution for this. If you are also based on speculation, I'd not jump into it before I see this is actually needed.

shrimalmadhur · 2024-04-30T17:39:14Z

Is using "list-quorums" on a separate machine than the one using opt-in/out needed right now?

It could be - what if operator opts-in using smart contract operator (or other ways where ecdsa key is not in same machine?) and want to list quorums? They should be able to run this without ecdsa key and password

I don't even see there is a solution for this. If you are also based on speculation, I'd not jump into it before I see this is actually needed.

wdym by I don't even see there is a solution for this?
We have all the feedback that we need to avoid giving access to ecdsa keys to node software as much as possible for any operations we do. This also applies to the nodeplugin cli where we should only require ecsda key for certain operations. Why would we want to force operators to provide ecdsa key path and password for list-quorums if it's not needed? We need to set best practices to AVSs to avoid needing ecdsa keys as much as possible for things which doesn't require it. I think that's a reason enough to do that - whatever we set right now becomes the way AVSs follow it. Why do you think we should not do that?

jianoaix · 2024-04-30T18:29:27Z

Is using "list-quorums" on a separate machine than the one using opt-in/out needed right now?

It could be - what if operator opts-in using smart contract operator (or other ways where ecdsa key is not in same machine?) and want to list quorums? They should be able to run this without ecdsa key and password

I don't even see there is a solution for this. If you are also based on speculation, I'd not jump into it before I see this is actually needed.

wdym by I don't even see there is a solution for this? We have all the feedback that we need to avoid giving access to ecdsa keys to node software as much as possible for any operations we do. This also applies to the nodeplugin cli where we should only require ecsda key for certain operations. Why would we want to force operators to provide ecdsa key path and password for list-quorums if it's not needed? We need to set best practices to AVSs to avoid needing ecdsa keys as much as possible for things which doesn't require it. I think that's a reason enough to do that - whatever we set right now becomes the way AVSs follow it. Why do you think we should not do that?

You are way over extraplating the point with principles as arguments. That doesn't help the discussion.
You may want to explain:

using "list-quorums" as a standalone tool has real use cases (not "they could use it separately...")
if the point is smart contract account, then the proper tooling solution for that, how does it work with this nodeplugin etc

make ecdsa optional for certain operations

c942910

shrimalmadhur requested review from jianoaix and ian-shim April 25, 2024 22:41

shrimalmadhur commented Apr 25, 2024

View reviewed changes

siddimore reviewed Apr 25, 2024

View reviewed changes

node/plugin/cmd/main.go Outdated Show resolved Hide resolved

get operator address from smart contract

602fe25

shrimalmadhur commented Apr 25, 2024

View reviewed changes

siddimore reviewed Apr 25, 2024

View reviewed changes

node/plugin/cmd/main.go Show resolved Hide resolved

ian-shim reviewed Apr 25, 2024

View reviewed changes

jianoaix reviewed Apr 26, 2024

View reviewed changes

node/plugin/cmd/main.go Outdated Show resolved Hide resolved

address some comments

cca159b

shrimalmadhur requested review from jianoaix and ian-shim April 26, 2024 00:15

shrimalmadhur mentioned this pull request Apr 26, 2024

remove the need of ecdsa key by default Layr-Labs/eigenda-operator-setup#120

Open

2 tasks

jianoaix closed this Apr 30, 2024

jianoaix reopened this Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make ecdsa optional for certain operations #522

make ecdsa optional for certain operations #522

shrimalmadhur commented Apr 25, 2024

shrimalmadhur Apr 25, 2024

shrimalmadhur Apr 25, 2024

ian-shim Apr 25, 2024

shrimalmadhur Apr 26, 2024 •

edited

Loading

shrimalmadhur Apr 26, 2024

jianoaix left a comment

shrimalmadhur commented Apr 26, 2024

jianoaix commented Apr 26, 2024

jianoaix commented Apr 26, 2024

shrimalmadhur commented Apr 26, 2024

jianoaix commented Apr 29, 2024

shrimalmadhur commented Apr 30, 2024

jianoaix commented Apr 30, 2024 •

edited

Loading

shrimalmadhur commented Apr 30, 2024

jianoaix commented Apr 30, 2024

shrimalmadhur commented Apr 30, 2024 •

edited

Loading

jianoaix commented Apr 30, 2024

make ecdsa optional for certain operations #522

Are you sure you want to change the base?

make ecdsa optional for certain operations #522

Conversation

shrimalmadhur commented Apr 25, 2024

Why are these changes needed?

Checks

shrimalmadhur Apr 25, 2024

Choose a reason for hiding this comment

shrimalmadhur Apr 25, 2024

Choose a reason for hiding this comment

ian-shim Apr 25, 2024

Choose a reason for hiding this comment

shrimalmadhur Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

shrimalmadhur Apr 26, 2024

Choose a reason for hiding this comment

jianoaix left a comment

Choose a reason for hiding this comment

shrimalmadhur commented Apr 26, 2024

jianoaix commented Apr 26, 2024

jianoaix commented Apr 26, 2024

shrimalmadhur commented Apr 26, 2024

jianoaix commented Apr 29, 2024

shrimalmadhur commented Apr 30, 2024

jianoaix commented Apr 30, 2024 • edited Loading

shrimalmadhur commented Apr 30, 2024

jianoaix commented Apr 30, 2024

shrimalmadhur commented Apr 30, 2024 • edited Loading

jianoaix commented Apr 30, 2024

shrimalmadhur Apr 26, 2024 •

edited

Loading

jianoaix commented Apr 30, 2024 •

edited

Loading

shrimalmadhur commented Apr 30, 2024 •

edited

Loading