Move DatabaseExtras back to trace #10868
Conversation
Add a new BlockChainWithExtras newtype to ethcore Impl DatabaseExtras for BlockChainWithExtras
Impl From for BlockChainWithExtras for convenient instantiation Change TraceDB::new to take a T: DatabaseExtras (instead of an Arc)
dvdplm
added
A0-pleasereview 🤓
| @@ -6,7 +6,6 @@ authors = ["Parity Technologies <admin@parity.io>"] | |||
| edition = "2018" | |||
|
|
|||
| [dependencies] | |||
| common-types = { path = "../types"} | |||
| ethcore-blockchain = { path = "../blockchain" } | |||
There was a problem hiding this comment.
Hmm, so we still can't get rid of the blockchain depenency? Then I don't think it makes much sense.
There was a problem hiding this comment.
There was a problem hiding this comment.
dyn BlockChainWithExtras? :)
There was a problem hiding this comment.
dyn BlockChainWithExtrasAndExtraMayo ;)
Sure, we can work on disentangling things further, but I think we can get more bang for our effort elsewhere at this time. I'm fine adding an issue to make trace independent of blockchain but for the time being I think we should pick the lesser of two evils, so the question is: which is worst, this PR or the current master? Personally I think they both smell a little so I don't really care which it is. @tomusdrw seem to feel this PR is worse. What is your opinion @debris, merge or close?
There was a problem hiding this comment.
Since we depend on blockchain the wrapper type is unecessary since we can write the implementation right next to the DatabaseExtras definition. I'd just add this in traces:
impl<T: BlockProvider> trace::DatabaseExtras for T {
fn block_hash(&self, block_number: BlockNumber) -> Option<H256> {
BlockProvider::block_hash(&self, block_number)
}
fn transaction_hash(&self, block_number: BlockNumber, tx_position: usize) -> Option<H256> {
self.block_hash(block_number)
.and_then(|block_hash| {
let tx_address = TransactionAddress {
block_hash,
index: tx_position
};
self.transaction(&tx_address)
})
.map(|tx| tx.hash())
}
}and be done with it.
| @@ -57,6 +57,16 @@ impl Key<FlatBlockTraces> for H256 { | |||
| } | |||
| } | |||
|
|
|||
| /// `DatabaseExtras` provides an interface to query extra data which is not stored in TraceDB, | |||
| /// but necessary to work correctly. | |||
| pub trait DatabaseExtras { | |||
There was a problem hiding this comment.
If we still need the blockchain dependenccy we can probably just implement DatabaseExtras for any BlockProvider here easily. There is no need for a wrapper struct afaict.
|
Updated as per @tomusdrw's grumble here: #10868 (comment) |
Move
DatabaseExtrasback totraceand remove the impl forBlockChain. Instead add aBlockChainWithExtrastype toethcoreand implDatabaseExtrasfor it.ref. #10840 (comment)