Repository reorganisation #13
Conversation
alekseysidorov
changed the title
There was a problem hiding this comment.
Review as of bb5e8a7.
Mostly good, but there are a couple of nitpicks. Besides line comments:
- Maybe, move
details/transactions.rsto thedetails/btcfolder? It seems slightly confusing thatblockchain/transactions.rsanddetails/transactions.rsare not related in any way - I don't quite agree with the naming of
src/blockchain/consensus_storage.rs. In a big scheme of things,AnchoringConfigis a binding for JSON-serialized data that is stored somewhere in the blockchain state. That is, it is a "consensus storage" in the same way that tables are "consensus storage". Understandably, this is a very minor nitpick - Is
details/sandbox.rsreally needed in the main repository? As far as I understand, it is only used for sandbox testing
src/details/btc/redeem_script.rs
Outdated
| @@ -87,16 +87,16 @@ impl RedeemScript { | |||
| mod tests { | |||
| extern crate blockchain_explorer; | |||
|
|
|||
| use bitcoin::util::base58::FromBase58; | |||
| // use bitcoin::util::base58::FromBase58; | |||
There was a problem hiding this comment.
Will the commented code be refactored back into the codebase in another PR? If not, remove it.
| impl AnchoringConfig { | ||
| /// Creates default anchoring configuration from given public keys and funding transaction | ||
| /// which were created earlier by other way. | ||
| pub fn new(validators: Vec<btc::PublicKey>, tx: FundingTx) -> AnchoringConfig { |
There was a problem hiding this comment.
Why does an initial funding tx need to be specified manually? Can't nodes find it (or multiple UTXOs) by the initial anchoring address automatically? (Possibly filtering the UTXO set, say, by the amount of bitcoins.)
Maybe, this is a matter for a new issue/PR; I won't press on solving it here.
There was a problem hiding this comment.
Now the service does not use the whitelist, it simply takes transaction from the config. If you want to use the whitelist, create discussion on active collab.
There was a problem hiding this comment.
Just to be clear, does this mean that once the initial funds are depleted, the anchoring process will stop? Seems like an issue that needs to be solved separately
There was a problem hiding this comment.
If you change the funding_tx with the configuration_service and then if it would be suitable for anchoring it will be used as input funding transaction.
If you calculate so that you need to add funding_transacton once at month along with the changing of the btc keys, then this should not cause difficulties for the admins.
| AnchoringConfig { | ||
| validators: validators, | ||
| funding_tx: tx, | ||
| fee: 1000, |
There was a problem hiding this comment.
Are hardcoded values adequate here? Do I understand correctly that this constructor is intended for testing purposes only?
There was a problem hiding this comment.
These are just default values that claim to be adequate.
There was a problem hiding this comment.
Maybe, implement the Default trait for AnchoringConfig then?
There was a problem hiding this comment.
It is a good idea, but I can't fill funding_tx with blank value.
| pub fn signatures(&self, | ||
| txid: &btc::TxId) | ||
| -> ListTable<MapTable<View, [u8], Vec<u8>>, u64, MsgAnchoringSignature> { | ||
| let prefix = [&[ANCHORING_SERVICE_ID as u8, 2], txid.as_ref()].concat(); |
There was a problem hiding this comment.
Maybe, define table ids as constants, similarly to service/message ids?
There was a problem hiding this comment.
I think we can return to this issue after exonum storage refactoring. We need to have general solution for prefixes.
There was a problem hiding this comment.
Table prefix: service_id + ordinal_id_within_service + arbitrary_suffix (for entity hash, other id, etc.) ?
It'll be up to good will of following convention within service (harder to mess up with tables from core or other service). But nothing prevents messing up any other table in case of bad will.
Maybe we should write a helper function to concatenate (service_id + ordinal_id_within_service + arbitrary_suffix), which may be reused inside of state hash aggregation function to identify tables by service_id + ordinal_id_within_service.
There was a problem hiding this comment.
I think it is time to think about a common solution of this problem, something like StorageKey trait for keys and common helper functions that makes prefix creation more easy.
|
Closes #12