Skip to content
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

Remove duplicate fsi assert from veci #366

Merged
merged 9 commits into from
Feb 6, 2025

Conversation

sergiupuhalschi-rdx
Copy link
Contributor

@sergiupuhalschi-rdx sergiupuhalschi-rdx commented Jan 31, 2025

Remove the assert of address and factor instance derivation path discrepancies from VirtualEntityCreatingInstance ctor non-debug code. If we identify a discrepancy, we log it with error!.

/// [identpr]: https://github.com/radixdlt/sargon/pull/254/files#r1860748013
/// [badpr]: https://github.com/radixdlt/babylon-wallet-android/pull/1042
/// [goodpr]: https://github.com/radixdlt/babylon-wallet-android/pull/1256
fn check_duplicate_factor_source_instances(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fn has an incorrect name. It sgould be check_for_derivation_path_discrepancies or similar

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthemore you can maybe put a debug_assertion gated behind #[cfg(test)] in the body

Copy link
Contributor

@Sajjon Sajjon Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

    fn check_for_derivation_path_discrepancies(
        factor_instance: &HierarchicalDeterministicFactorInstance,
        address: &AddressOfAccountOrPersona,
    ) {
       let discrepancy_found = factor_instance.derivation_path().get_entity_kind()
            == address.get_entity_kind();
       let error_msg = "Discrepancy! Address and DerivationPath of FactorInstances have different entity kinds.";
        if discrepancy_found
        {
            error!(error_msg);
        }
            #[cfg(test]
            debug_assert!(!discrepancy_found, error_msg)
    }

with formatting... :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although I was close to getting it right, I think your comment saved me from another round of changes :)) Thanks!

Copy link

codecov bot commented Jan 31, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.64%. Comparing base (56196a5) to head (88d3d82).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #366      +/-   ##
==========================================
+ Coverage   92.61%   92.64%   +0.03%     
==========================================
  Files        1180     1180              
  Lines       26658    26658              
  Branches       77       77              
==========================================
+ Hits        24690    24698       +8     
+ Misses       1957     1949       -8     
  Partials       11       11              
Flag Coverage Δ
kotlin 97.63% <ø> (ø)
rust 92.31% <100.00%> (+0.03%) ⬆️
swift 92.86% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

) {
let discrepancy_found =
factor_instance.derivation_path().get_entity_kind()
== address.get_entity_kind();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be != and not == right?.

Missing unit tests as well :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this unit test enough?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize it was ME who accidentally gave you == :D :D sorry about that

and yes, sorry I was accidentally checking only last commit... that test is fine!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, I should have double-checked. Thanks for spotting this!

This reverts commit 4618940.
@sergiupuhalschi-rdx sergiupuhalschi-rdx merged commit e85b6da into main Feb 6, 2025
13 checks passed
@sergiupuhalschi-rdx sergiupuhalschi-rdx deleted the sp/remove-veci-duplicate-fsi-assert branch February 6, 2025 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants