Operator opt-out

[emlautarom1]

Implements #170

[emlautarom1]

I need to explore how to test this code first so marked as a draft for now.

[taco-paco]

Good job!
One thing that I noticed is that plugin function doesn't return error so our app always exists gracefully with code 0 even if error occurred.

Notice here we use deprecated signature. I would propose to change this

[emlautarom1]

After spending quite some time trying to test the code I'm struggling to come up with a solution. My intent is to write a test that registers an operator, checks that it's registered, unregisteres it and checks that it's no longer registered.

Our entry point is plugin/cmd/main.go:

https://github.com/NethermindEth/near-sffl/blob/47e84e0fe342e19d2b7a3cfcbb4189d9fd13e3a0/plugin/cmd/main.go#L190-L203

There is no point in testing the CLI since it's responsible for reading the config values and passing them to AvsManager (the component that actually performs the logic), and passing back the resulting values. Thus, we're interested in testing AvsManager

operator/avs_manager.go

https://github.com/NethermindEth/near-sffl/blob/47e84e0fe342e19d2b7a3cfcbb4189d9fd13e3a0/operator/avs_manager.go#L289-L302

Here is the actual logic that we want to test, but in order to do so we need to construct an AvsManager. This struct is opaque, only exposing a new method that uses real dependencies (ex. chainio.BuildAvsWriter):

https://github.com/NethermindEth/near-sffl/blob/47e84e0fe342e19d2b7a3cfcbb4189d9fd13e3a0/operator/avs_manager.go#L62

We could add an additional constructor that allows us to pass the dependencies or expose the AvsManager struct fields but then we would need to mock things like chainio.AvsWriter which is the one doing the real work

https://github.com/NethermindEth/near-sffl/blob/47e84e0fe342e19d2b7a3cfcbb4189d9fd13e3a0/operator/avs_manager.go#L294

If we mock it, then the only thing we can assert is that we're calling the DeregisterOperator method, which is useless information.

---

At this point, I don't see any reason to continue pushing for a unit test: to test this behavior an integration test would be more reasonable since all the code is eventually coupled to an on-chain interaction with the AVS contracts.

I'd appreciate any suggestions on how this topic: @taco-paco @Hyodar

[Hyodar]

I agree, this is better fitted for an integration test. Actually, we don't even use the plugin deposit, so we could remove it, and in the end this just initializes and wraps an AvsManager. That said, I don't think it's too relevant having a unit test for this.

We could still do it if it's desirable somehow, like:

1. We define an OperatorPlugin struct and turn our actions into receivers:

type OperatorPlugin struct {
    blsKeyPair *bls.KeyPair
    ecdsaPrivateKey *ecdsa.PrivateKey
    avsManager AvsManagerer
}

func NewOperatorPlugin(/* same fields as struct */) *OperatorPlugin {
    // just create struct
}

func (*OperatorPlugin) OptOut() error {
    // ...
}

2. We generate a gomock from AvsManagerer
3. We use the mock in a test and expect calls to be made like that

func createMockOperatorPlugin(mockCtrl *gomock.Controller, /* keypairs... */) (*OperatorPlugin, *mocks.MockAvsManagerer) {
    mockAvsManager := mocks.NewMockAvsManagerer(mockCtrl)
    return NewOperatorPlugin(/* keypairs... */, mockAvsManager), mockAvsManager
}

func SomeTest(t *testing.T) {
    mockCtrl := gomock.NewController(t)
    defer mockCtrl.Finish()

    mockOperatorPlugin, mockAvsManager := createMockOperatorPlugin(mockCtrl, /* ... */)
    
    mockAvsManager.EXPECT().DeregisterOperator(blsKeypair).Return(nil)
    err := mockOperatorPlugin.OptOut()
    assert.Nil(t, err)
}

Then, having unit tested AvsManager, we are assuming relaying the call is enough. You could also implement a more complete mock for AvsManager there that tracks the registrations and uses it for checking if an operator is registered.
Again, at this point I don't think it makes sense, and I'd agree in this case we care about the side effects that just mocking may not be enough, just a quick sketch showing one way to do it.

[Hyodar]

LGTM!

[emlautarom1]

I ended up separating the OperatorPlugin into an interface with the three operations ("opt-in", "opt-out", "deposit") and a concrete implementation that uses the CLI to construct all dependencies (ex. AvsManager). This would allow us to construct an OperatorPlugin with explicit dependencies but testing is still hard: some method like OptOut are just getting data from the environment/filesystem and passing them to the AvsManager.

Again, at this point I don't think it makes sense, and I'd agree in this case we care about the side effects that just mocking may not be enough, just a quick sketch showing one way to do it.

I agree and I don't think its worth pursuing any further with this task. I'll mark it as complete and we can revisit it if we find issues.

[Hyodar]

Agreed. Only a few comments, LGTM.