clean up for zingolib v3.0.0 rc#2194
Conversation
| merge_scan_ranges(sync_state, ScanPriority::ChainTip); | ||
|
|
||
| let verification_height = sync_state | ||
| let reorg_detection_start_height = sync_state |
zancas
left a comment
zancas
left a comment
There was a problem hiding this comment.
I left non-blocking comments, with 1 exception, why is truncate_scan_ranges a free fn that takes a &mut instead of a method of SyncState?
Everything else looks good to me. I like the renames, and erroring below Sapling.
|
|
||
| /// Splits the range containing [`truncate_height` + 1] and removes all ranges containing block heights above | ||
| /// `truncate_height`. | ||
| pub(super) fn truncate_scan_ranges(truncate_height: BlockHeight, sync_state: &mut SyncState) { |
There was a problem hiding this comment.
Why not define this as a method of a SyncState?
| #[error( | ||
| "birthday {0} below sapling activation height {1}. pre-sapling wallets are not supported!" | ||
| )] | ||
| BirthdayBelowSapling(u32, u32), |
| /// * the number of blocks AHEAD of proxy_reported_chain_height allowed | ||
| /// (2) Sapling Epoch Height: | ||
| /// * the maximum number of blocks the wallet can truncate during re-org detection | ||
| /// (2) Sapling Activation Height: |
| Err(SyncError::WalletError( | ||
| crate::mocks::MockWalletError::AnErrorVariant(ref s) | ||
| )) if s == test_error | ||
| } else if birthday < sapling_activation_height { |
| )); | ||
| } | ||
|
|
||
| mod last_known_chain_height { |
There was a problem hiding this comment.
Why are all these tests removed? Are they included elsewhere?
| Ok(()) | ||
| } | ||
|
|
||
| /// Compares the wallet birthday to sapling activation height and returns the highest block height. |
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { |
| )) if s == test_error | ||
| )); | ||
| } | ||
| #[ignore = "in progress"] |
I'm not sure which changes have been requested for merge. Regarding the truncate being a method on sync state, there was a bunch of trade-offs I considered with this and decided a functional architecture worked best for the library. This allows us to keep SyncState succinct in the 'wallet' module (public APi for use in consumer wallet structs) and keep all the state logic cleanly separated. I also think it would be confusing to have a bunch of SyncState impl spread across the library... I like them to be directly under the struct or in clean submodules so consumers can clearly see all methods/functionality of a type. There is also the fact that many state fns don't just operate on the SyncState so I think there is more consistency this way. There are possibly other reasons but it's a while since I decided to go this way. If you have a counter argument for a benefit of object-oriented I have missed we can discuss |
|
I think the reasoning you offer here is very useful. I haven't carefully considered tradeoffs in this space, but I think it's very worthwhile to capture our thinking somewhere. |
3 participants
last_known_chain_heightshigher than the currentchain_height