-
Notifications
You must be signed in to change notification settings - Fork 250
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move remaining pool tests from zcash_client_sqlite
to zcash_client_backend
#1543
Conversation
Third commit is best reviewed with:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1543 +/- ##
==========================================
+ Coverage 60.51% 61.32% +0.80%
==========================================
Files 147 147
Lines 17313 18667 +1354
==========================================
+ Hits 10477 11447 +970
- Misses 6836 7220 +384 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK a463f7d with minor suggestion for improvement that can be addressed post-merge if necessary. Third commit reviewed by local diff with --color-moved=zebra.
fn get_confirmed_sends( | ||
&self, | ||
txid: &TxId, | ||
) -> Result<Vec<(u64, Option<String>, Option<String>, Option<u32>)>, <Self as WalletRead>::Error>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the inner result type here be a struct? I have no idea what this tuple means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that would be nicer. I pulled this API as-is from #1533. We also need to add documentation etc. to the new APIs in this PR, which includes All Of The Tests (but I'd spent long enough on this PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #1544.
let to_address: Option<String> = row.get(1)?; | ||
let ephemeral_address: Option<String> = row.get(2)?; | ||
let address_index: Option<u32> = row.get(3)?; | ||
Ok((u64::from(v), to_address, ephemeral_address, address_index)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this would be much more usable if it were a struct.
@@ -1532,12 +1526,6 @@ pub(crate) fn checkpoint_gaps<T: ShieldedPoolTester>() { | |||
// Scan the block | |||
st.scan_cached_blocks(account.birthday().height() + 10, 1); | |||
|
|||
// Fake that everything has been scanned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little bit concerned that this test still passes without this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I verified that without the scan_cached_blocks
it fails, so it needs that specific block to be scanned, but apparently not the rest of the chain.
Co-authored-by: Jack Grigg <[email protected]>
…_backend` Co-authored-by: Willem Olding <[email protected]>
Now that these are in a separate trait, we can just require them for all downstreams, in order to make them usable in all tests.
a463f7d
to
e67e7ab
Compare
Force-pushed to address the SQL review comment. I'll make the struct changes in a separate PR next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-utACK e67e7ab
Please add an issue to track the documentation & struct changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-hoc ACK
No description provided.