Db macro for coordinator/tributary #450
Conversation
| CommitDb: (genesis: [u8; 32], block: &[u8; 32]) -> Vec<u8>, | ||
| BlockAfterDb: (genesis: [u8; 32], hash: [u8; 32]) -> [u8; 32], | ||
| UnsignedIncludedDb: (genesis: [u8; 32], hash: [u8; 32]) -> Vec<u8>, | ||
| ProvidedIncludedDb: (genesis: [u8; 32], hash: [u8; 32]) -> [u8; 0], |
| BlockAfterDb: (genesis: [u8; 32], hash: [u8; 32]) -> [u8; 32], | ||
| UnsignedIncludedDb: (genesis: [u8; 32], hash: [u8; 32]) -> Vec<u8>, | ||
| ProvidedIncludedDb: (genesis: [u8; 32], hash: [u8; 32]) -> [u8; 0], | ||
| NextNonceDb: (genesis: [u8; 32], hash: [u8; 32]) -> Vec<u8> |
There was a problem hiding this comment.
NextNonce is by signer: [u8; 32], not hash: [u8; 32]. I also believe this Vec<u8> should be a u32
| create_db!( | ||
| TributaryBlockchainDb { | ||
| TipsDb: (genesis: [u8; 32]) -> [u8; 32], | ||
| BlockNumberDb: (genesis: [u8; 32]) -> Vec<u8>, |
There was a problem hiding this comment.
Shouldn't this return a u32?
| BlockHashDb: (genesis: [u8; 32], block_number: u32) -> [u8; 32], | ||
| CommitDb: (genesis: [u8; 32], block: &[u8; 32]) -> Vec<u8>, | ||
| BlockAfterDb: (genesis: [u8; 32], hash: [u8; 32]) -> [u8; 32], | ||
| UnsignedIncludedDb: (genesis: [u8; 32], hash: [u8; 32]) -> Vec<u8>, |
| CommitDb::get( | ||
| self.db.as_ref().unwrap(), | ||
| self.genesis, | ||
| &BlockHashDb::get(self.db.as_ref().unwrap(), self.genesis, block).unwrap(), |
There was a problem hiding this comment.
This was self.block_hash(block)?. Now it's BlockHashDb::get(...).unwrap(). That means this function will now panic if called with a block number which hasn't been used, not return None.
| CurrentDb: (genesis: &[u8]) -> Vec<u8>, | ||
| LocalQuantityDb: (genesis: &[u8], order: &[u8]) -> u32, | ||
| OnChainQuantityDb: (genesis: &[u8], order: &[u8]) -> u32, | ||
| BlockQuantityDb: (genesis: &[u8], block: &[u8], order: &[u8]) -> u32, | ||
| OnChainTxDb: (genesis: &[u8], order: &[u8], id: u32) -> [u8; 32] |
There was a problem hiding this comment.
The first 4 dropped "Provided", the last one replaced it with "Tx". The first 4 are now ambiguous (quantity of what), and the last one loses specificity (as it's only for on-chain provided TXs, not for on-chain transactions in general). Mind if we keep "Provided"?
| create_db!( | ||
| TributaryProvidedDb { | ||
| TransactionDb: (genesis: &[u8], hash: &[u8]) -> Vec<u8>, | ||
| CurrentDb: (genesis: &[u8]) -> Vec<u8>, |
There was a problem hiding this comment.
This can be Vec<[u8; 32]>, not Vec.
| let this_provided_id = on_chain_quantity; | ||
| txn.put(Self::on_chain_provided_key(&self.genesis, order, this_provided_id), tx); | ||
|
|
||
| // let block_order_key = Self::block_provided_quantity_key(&self.genesis, &block, order); |
There was a problem hiding this comment.
Is there a reason this was left floating here?
| Blockchain::<MemDb, SignedTransaction>::block_after(&db, genesis, &block.parent()).unwrap(), | ||
| block.hash() | ||
| ); | ||
| assert_eq!(BlockAfterDb::get(&db, genesis, block.parent()).unwrap(), block.hash()); |
There was a problem hiding this comment.
Hm. The public API of this grew from a read-only accessor to full read-write access, which can likely break safety (except block_after was prior pub(crate), so it doesn't actually since this API is also still only pub(crate)).
There's nothing to do in response to this comment, just being mindful and raising it as something to keep in mind with future edits which may actually affect the public API.
|
Thanks for working on this :) |
implemented create_db for blockchain.rs and provided.rs