Break out chainio and crypto #805
Conversation
mooselumph
requested review from
cody-littley,
hopeyen,
jianoaix and
ian-shim
| } | ||
|
|
||
| // ChainState is an interface for getting information about the current chain state. | ||
| type IndexedChainState interface { |
There was a problem hiding this comment.
Wondering why IndexedOperatorState and IndexedChainState would be under indexer.go instead of state.go. Maybe we could rename this file to indexed.go?
chainio/eth/state.go
Outdated
| Tx core.Transactor | ||
| } | ||
|
|
||
| func NewChainState(tx core.Transactor, client common.EthClient) *ChainState { |
There was a problem hiding this comment.
Have we thought about just moving these methods to reader? These are just thin wrapper over operator state onchain?
There was a problem hiding this comment.
I'd like to do this, but could we do it in a follow-up PR?
chainio/eth/state.go
Outdated
|
|
||
| type ChainState struct { | ||
| Client common.EthClient | ||
| Tx core.Transactor |
There was a problem hiding this comment.
Should we use reader instead?
chainio/eth/reader.go
Outdated
| EthClient common.EthClient | ||
| Logger logging.Logger | ||
| Bindings *ContractBindings |
There was a problem hiding this comment.
changed to private
|
|
||
| func NewReader( | ||
| logger logging.Logger, | ||
| client common.EthClient, |
There was a problem hiding this comment.
As we're separating reader & writer, one thing that would be nice to handle is how eth client is set up. Configuring private key in eth client is bit awkward right now. Definitely out of scope of this PR, but we could probably enforce that the client doesn't have pk configured in reader (and vice versa for writer)
| @@ -0,0 +1,203 @@ | |||
| package bn254 | |||
There was a problem hiding this comment.
is this just copy paste of existing logic or is there logic changes here?
There was a problem hiding this comment.
copy paste from core. We can clean up in future PR.
| const ( | ||
| // We use uint8 to count the number of quorums, so we can have at most 255 quorums, | ||
| // which means the max ID can not be larger than 254 (from 0 to 254, there are 255 | ||
| // different IDs). | ||
| MaxQuorumID = 254 | ||
| ) | ||
|
|
||
| type QuorumID = uint8 | ||
|
|
||
| type OperatorID = [32]byte |
There was a problem hiding this comment.
I feel like these types (and helper methods) should belong somewhere outside than chainio. Why do we want to redefine them outside existing core package?
There was a problem hiding this comment.
Where should they be defined? Currently, core/v2 depends on chainio for operator state types, and chainio is meant to depend only on chain bindings.
We could potentially reverse the ordering, but thi is inconvenient because it means core can use the chainio objects. Any other ideas?
There was a problem hiding this comment.
I see. Conceptually, I thought it made sense for chainio to depend on core, but I guess the argument can be made the other way too. I'm fine either way.
It's bit confusing having the same types (OperatorID, QuorumID) defined again here though, in addition to the existing core. Is having this duplicate type perferred over reusing core.OperatorID?
There was a problem hiding this comment.
These are simply type aliases, not type definitions. I'm using this just as a tool for maintaining easy consistency and upgradeability within this package. From that perspective, I wonder if it is less confusing?
I'd prefer not to have any dependencies on core as this will be deprecated.
Why are these changes needed?
Breaks out chainio and crypto from the core library?
Checks