Refactor chain store types #144
Conversation
* Added Read / Write traits for TipIndex similiar to key-value store * Included rocksdb in ChainStore
| pub trait Write { | ||
| fn put(&mut self, meta: &TipSetMetadata) -> Result<(), Error>; | ||
| } | ||
|
|
||
| pub trait Read { | ||
| fn get(&self, key: u64) -> Result<TipSetMetadata, Error>; | ||
| fn get_tipset(&self, idx: &dyn Index) -> Result<Tipset, Error>; | ||
| fn get_tipset_state_root(&self, idx: &dyn Index) -> Result<Cid, Error>; | ||
| fn get_tipset_receipts_root(&self, idx: &dyn Index) -> Result<Cid, Error>; | ||
| } |
There was a problem hiding this comment.
what is the benefit to having these as traits? They will never need to be used ambiguously. If so, why do they need to be two seperate traits?
There was a problem hiding this comment.
I suppose there no real benefit other than having our read and write operations set up similar to the way the datastore is set up for improved consistency.
There was a problem hiding this comment.
I mean attaching these functions to a trait in any context, when would the implementation be swapped out?
There was a problem hiding this comment.
Fair point. I can revert.
| @@ -1,6 +1,15 @@ | |||
| // Copyright 2020 ChainSafe Systems | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| #![allow(dead_code)] | |||
There was a problem hiding this comment.
Without it, the compiler complains that Chain is never constructed.
There was a problem hiding this comment.
ahh gotcha, so I'm fine with this as is but why are you including this private struct before it is even used or without a constructor? Definitely alright if you remember to come back to it but could lead to bloated unused code if we do this a lot.
There was a problem hiding this comment.
The main reason its without a constructor is because of the lack of clarity of what should be included within this struct via the spec. Once we have established what should be included than I can include it. Within go-filecoin it is comprised of the following fields:
ChainStore
MessageStore
StateReadWriter
HeaviestTipSet
Consensus
However, we only have the ChainStore included thus far.
| pub use rocks::RocksDb; | ||
| pub use rocks::*; |
There was a problem hiding this comment.
Not sure why rust doesn't detect this is redundant, but only need to export the wildcard (only public is RocksDb)
| @@ -1,6 +1,15 @@ | |||
| // Copyright 2020 ChainSafe Systems | |||
| // SPDX-License-Identifier: Apache-2.0 | |||
|
|
|||
| #![allow(dead_code)] | |||
There was a problem hiding this comment.
ahh gotcha, so I'm fine with this as is but why are you including this private struct before it is even used or without a constructor? Definitely alright if you remember to come back to it but could lead to bloated unused code if we do this a lot.
Changes introduced in this PR:
TipIndexsimilar to the key-value storeChainStoreChainstruct tolib.rsNot including the
checkpointtype definitions as I think it's better to hold off on implementing until we have a clearer idea of how they are used with our verifier node.Closes #116 , closes #81