Zingo sync reorg logic #1348
Zingo sync reorg logic #1348
Conversation
|
I clicked update branch. |
…o_sync_reorg_logic
|
I noticed that CI was failing, and that this branch was out of date, so I clicked "Update Branch". |
Oh I forgot to run clippy! Ok fixing now |
| @@ -31,7 +31,7 @@ async fn sync_mainnet_test() { | |||
| let mut lightclient = LightClient::create_from_wallet_base_async( | |||
| WalletBase::from_string(HOSPITAL_MUSEUM_SEED.to_string()), | |||
| &config, | |||
| 2_611_700, | |||
| 2_715_150, | |||
There was a problem hiding this comment.
If this is, for example a NU height, or protocol epoch, I think it would be best to import the most general relevant constant and use it explicitly.
If it's an HMS-specific birthday, then maybe we should associate it with that seed in some ergonomic way, so that e.g. other tests can get the seed-associated bday?
If it's not, for example if it's a birthday that's idiosyncratic to this test, then let's throw some kind of comment on it, specifying that.
There was a problem hiding this comment.
This test will be removed eventually it is ignored and I only use it sometimes when a larger chain is needed. This birthday is just a block height near to the server tip at time of writing.
| @@ -77,8 +77,8 @@ async fn sync_test() { | |||
| .await | |||
| .unwrap(); | |||
|
|
|||
There was a problem hiding this comment.
I thought we wanted to cut dbgs out? I guess I am ok with commented out dbgs in test code. Is that our practice?
There was a problem hiding this comment.
For sync development its good to have these dbgs ready to go when needed. I don't think we should normally allow debug but that's why I created a separate sync.rs test suite
| @@ -35,6 +35,15 @@ impl SyncState { | |||
| spend_locations: Vec::new(), | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
From local checks this fn could currently be pub(crate).. should we promote it to fully pub "on demand"?
Do we already have a consumer using it?
There was a problem hiding this comment.
I thought this could be useful API but Im happy with making it pub(crate), i will make this change on the following PR. I think it's a good policy to keep things private unless there is a known use case for sure
before zingo sync is released all error handling and a large % test coverage will be added. as unit tests have to change when a fn changes I want to write a PoC first to be more time efficient |
atop #1343