-
Notifications
You must be signed in to change notification settings - Fork 75
ETCM-938 Refactor Blockchain and extract methods getEvmCodeByHash and mptStateSavedKeys #1017
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,14 +4,16 @@ import akka.actor.{Actor, ActorLogging, ActorRef, PoisonPill, Props, Scheduler} | |
import io.iohk.ethereum.blockchain.sync.fast.FastSync | ||
import io.iohk.ethereum.blockchain.sync.regular.RegularSync | ||
import io.iohk.ethereum.consensus.validators.Validators | ||
import io.iohk.ethereum.db.storage.{AppStateStorage, FastSyncStateStorage} | ||
import io.iohk.ethereum.db.storage.{AppStateStorage, EvmCodeStorage, FastSyncStateStorage, NodeStorage} | ||
import io.iohk.ethereum.domain.Blockchain | ||
import io.iohk.ethereum.ledger.Ledger | ||
import io.iohk.ethereum.utils.Config.SyncConfig | ||
|
||
class SyncController( | ||
appStateStorage: AppStateStorage, | ||
blockchain: Blockchain, | ||
evmCodeStorage: EvmCodeStorage, | ||
nodeStorage: NodeStorage, | ||
fastSyncStateStorage: FastSyncStateStorage, | ||
ledger: Ledger, | ||
validators: Validators, | ||
|
@@ -77,6 +79,8 @@ class SyncController( | |
fastSyncStateStorage, | ||
appStateStorage, | ||
blockchain, | ||
evmCodeStorage, | ||
nodeStorage, | ||
validators, | ||
peerEventBus, | ||
etcPeerManager, | ||
|
@@ -120,6 +124,8 @@ object SyncController { | |
def props( | ||
appStateStorage: AppStateStorage, | ||
blockchain: Blockchain, | ||
evmCodeStorage: EvmCodeStorage, | ||
nodeStorage: NodeStorage, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder at what point we'll create a case class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We already have something similar It is a big bag with every storage in it so we will probably want to separate them to have storages that work together in the same container, and identify storages that are a "view" of other storages. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
syncStateStorage: FastSyncStateStorage, | ||
ledger: Ledger, | ||
validators: Validators, | ||
|
@@ -134,6 +140,8 @@ object SyncController { | |
new SyncController( | ||
appStateStorage, | ||
blockchain, | ||
evmCodeStorage, | ||
nodeStorage, | ||
syncStateStorage, | ||
ledger, | ||
validators, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you expect it will happen more during the refactoring that actors will have access to the storages directly? (To clarify: I don't really see a problem with it.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think might happen for state storage also as the Blockchain class does not really bring anything in term of read access (I think it's ok for read access).
We will probably want write access to be through a single service though, or redefined what we call a storage to be a class that manage several rocksdb namespace internally, because we have some writes that span several storages (for instance when adding a block we want to store the block itself but also update the transaction mapping and potentially some other metadata).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍