BCF-2654 chain reader get latest value evm poc WIP #11272
Conversation
Also add some initialization and validation relay skeletons
This avoids us having to modify the signature of NewMedianFactory, which would require further modifications to all non-evm repos and chainlink-relay
Co-authored-by: Jordan Krage <jmank88@gmail.com>
Logic was inverted here, causing ChainReader to be used if it's unimplemented rather than if it's implemeneted. Also, clarify error message to avoid ambiguity
This was getting setting medianProvider.chainReader to *chainReader(nil) instead of nil
…s/relay/evm/chain_reader.go
- Parse abis in constructor - Unexport validation - Unexport constructor
|
I see that you haven't updated any CHANGELOG files. Would it make sense to do so? |
|
SonarQube Quality Gate
|
ilija42
changed the title
reductionista
force-pushed
the
BCF-2612-ChainReader-EVM-POC
branch
9 times, most recently
from
2324382 to
c4f0291
Compare
reductionista
force-pushed
the
BCF-2612-ChainReader-EVM-POC
branch
from
4de881b to
109e0d9
Compare
ilija42
force-pushed
the
BCF-2612-ChainReader-EVM-POC
branch
2 times, most recently
from
7673e0d to
4e70656
Compare
| @@ -111,6 +132,13 @@ func NewMedianServices(ctx context.Context, | |||
| CreatedAt: time.Now(), | |||
| }, lggr) | |||
|
|
|||
| if medianProvider.ChainReader() != nil { | |||
| medianProvider = medianProviderWrapper{ | |||
There was a problem hiding this comment.
@ilija42 Looking at the details of this now, I think this modification is happening too high up the call-stack. Have you considered adding a shim around median.NumericalMedianFactory that overrides the median contract with a ChainReader based implementation? This would behave almost like what @reductionista described as the ideal suggestion, which would be to modify median directly.
I think implementing it that way has a couple of advantages:
- the shim provides a centralized, single abstraction point where we can modify median in a way that will affect all users of it.
- easier migration -- IIRC we are going to take ownership over median in the near future, so this will make those changes easier to make.
There was a problem hiding this comment.
@cedric-cordenier sry, completely missed this. This makes sense to me, I will change this in the final POC version
There was a problem hiding this comment.
Oh, I also missed this somehow till today, despite being tagged!
I don't see a problem with this, but curious to understand the motivation better.
the shim provides a centralized, single abstraction point where we can modify median in a way that will affect all users of it.
What other users of NumericalMedianFactory would there be aside from the Median plugin? Is the issue just that for migration you need a separate implementation of Median plugin to co-exist with the current one? Or is it more than that?
reductionista
force-pushed
the
BCF-2612-ChainReader-EVM-POC
branch
7 times, most recently
from
ad7a6a1 to
1cd875f
Compare
| contractAddr := common.HexToAddress(bc.Address) | ||
|
|
||
| // TODO overriding params with params from config | ||
| // TODO overriding returnVal with returnVal from config |
There was a problem hiding this comment.
Should we add a JIRA # here?
WIP