Check for colliding component FVKs #1489
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…accounts. Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
fcbf3f3 to
90f8789
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1489 +/- ##
==========================================
- Coverage 61.16% 61.13% -0.03%
==========================================
Files 141 141
Lines 16661 16663 +2
==========================================
- Hits 10190 10187 -3
- Misses 6471 6476 +5 ☔ View full report in Codecov by Sentry. |
050a58e to
59d9224
Compare
daira
commented
Aug 10, 2024
| // Make a best effort to determine the AccountId of the pre-existing row | ||
| // An account conflict occurred. This should already have been caught by | ||
| // the check using `get_account_for_ufvk` above, but in case it wasn't, | ||
| // make a best effort to determine the AccountId of the pre-existing row | ||
| // and provide that to our caller. |
There was a problem hiding this comment.
This code just below this will no longer be reached when a collision occurs, which explains the small reduction in test coverage.
(if available) collides with an existing imported or derived FVK. This does not check for collisions on IVK for `Incoming` accounts. Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
59d9224 to
bd38645
Compare
daira
commented
Aug 10, 2024
| let seed_based = st.wallet_mut().create_account(&seed, &birthday).unwrap(); | ||
| let seed_based_account = st.wallet().get_account(seed_based.0).unwrap().unwrap(); | ||
| let ufvk = seed_based_account.ufvk().unwrap(); | ||
|
|
||
| assert_matches!( | ||
| st.wallet_mut().import_account_ufvk(ufvk, &birthday, AccountPurpose::Spending), | ||
| st.wallet_mut().import_account_hd(&seed, zip32_index_0, &birthday), |
There was a problem hiding this comment.
The import_account_ufvk check is done in check_collisions below, so this is adding an extra check for collisions between create_account and import_account_hd.
nuttycom
approved these changes
Aug 11, 2024
|
Post-hoc utACK bd38645 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When adding an account, check whether any component of its UFVK (if available) collides with an existing imported or derived FVK.
This does not check for collisions on IVK for
Incomingaccounts (#1490), which is okay because those are not yet exposed in the public API.Builds on #1479, fixes #1474