median: prefer adapting ChainReader to MedianContract

[jmank88]

This PR moves type medianContract from core, which is the adapter for implementing the MedianContract interface with ChainReader. By using it here in the plugin, at the outermost client layer, instead of from the core host, in the middle of proxying the provider, we ensure that the new ChainReader is used end-to-end underneath, so that the old MedianContract grpc support can safely be phased out.

There are a few differences with medianContract from core:

• The fields are exported so that package json can access them
• Pending: false - this matches existing behavior for LatestTransmissionDetails here: https://github.com/smartcontractkit/chainlink/blob/bd7e233d3a489e8dacae0a8c8539802b3ae10832/core/services/relay/evm/median.go#L73
• The lookback argument is passed along as an arg to GetLatestValue

[jmank88]

@vyzaldysanchez the linter is broken again. Didn't we resolve this already or was that in another repo?

<checkstyle version="5.0">
  <file name="../../../../../opt/hostedtoolcache/go/1.21.3/x64/src/slices/sort.go">
    <error column="7" line="67" message="undefined: min" severity="error" source="typecheck"></error>
    <error column="7" line="97" message="undefined: max" severity="error" source="typecheck"></error>
  </file>

[jmank88]

@vyzaldysanchez the linter is broken again. Didn't we resolve this already or was that in another repo?

<checkstyle version="5.0">
  <file name="../../../../../opt/hostedtoolcache/go/1.21.3/x64/src/slices/sort.go">
    <error column="7" line="67" message="undefined: min" severity="error" source="typecheck"></error>
    <error column="7" line="97" message="undefined: max" severity="error" source="typecheck"></error>
  </file>

Nevermind - it just need a bump from 1.53 to 1.55. Passing now.

[cedric-cordenier]

@jmank88 We have two places where we create median plugins now (here and in the medianpoc package), so I think we need a place where we can add common code like this (this adaptor should apply to both plugins really) that isn't client-specific. My idea was to create a constructor function for NumericalMedianFactory and add these changes to that.

What do you think?

[jmank88]

@jmank88 We have two places where we create median plugins now (here and in the medianpoc package), so I think we need a place where we can add common code like this (this adaptor should apply to both plugins really) that isn't client-specific. My idea was to create a constructor function for NumericalMedianFactory and add these changes to that.

What do you think?

Should medianpoc be moved to this repo?

But regardless, a NumericalMedianFactory constructor would couple together some unrelated things, and that type lives in libocr, so it seems like an unnecessary complication 🤷

[cedric-cordenier]

@jmank88 and I synced offline: we are going to move medianpoc to this package and expose the two different server interfaces via different methods on the plugin type. I'm happy to go with the adaptation listed here and will circle back with a PR to combine median and medianpoc.

[reductionista]

This change will also need to be made in NewMedianFactory in chainlink-commons. I guess it doesn't matter for the EVM POC since it's not using LOOP yet, but anything which uses LOOP will go through that code path instead. I think that was the original reason why I chose to do it in the place I did--seemed better to wrap the struct in a central place rather than 2 places (which has now become 3 with @cedric-cordenier 's changes).

https://github.com/smartcontractkit/chainlink-common/blob/main/pkg/loop/internal/median.go#L87C28-L92

[reductionista]

Actually, come to think of it, if we're going to do it this way then the definition of medianContract along with its methods needs to go in chainlink-commons rather than here. Otherwise there is no way for chainlink-commons to import it.

[jmank88]

Why would chainlink-common need to import this? We only want to use it at the outermost client layer. If we used it on the server side, then we would still be using the GRPC services over the wire, and could not phase them out.

[reductionista]

Yeah, nevermind I think I was confused about the interconnection of all of this. I was thinking the NewMedianFactory method in chainlink-common was calling NumericalMedianFactory directly but I guess the call it's making must end up getting back to this function (in chainlink-feeds at some point, where the actual call gets made... right?

[jmank88]

Kind of yeah. I think it's more about the GRPC layer being a separate concern from chain reader being available or not.

[jmank88]

This change will also need to be made in NewMedianFactory in chainlink-commons. I guess it doesn't matter for the EVM POC since it's not using LOOP yet, but anything which uses LOOP will go through that code path instead. I think that was the original reason why I chose to do it in the place I did--seemed better to wrap the struct in a central place rather than 2 places (which has now become 3 with @cedric-cordenier 's changes).

smartcontractkit/chainlink-common@main/pkg/loop/internal/median.go#L87C28-L92

I don't think that needs to change, because we only pass a provider ID.

[jmank88]

Updated to remove TODO and renamed to avoid median.median*.

[reductionista]

lookback is an obsolete parameter unused anywhere at Chainlink. We can't support it with GetLatestValue() without changing the meaning of the method and breaking the chain agnostic nature of it in an awkward way. Only contract method params (or equivalently, log topics) should be passed in params field to GetLatestValue()