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

Unify keys #1414

Merged
merged 28 commits into from
Oct 7, 2024
Merged

Unify keys #1414

merged 28 commits into from
Oct 7, 2024

Conversation

Oscar-Pepper
Copy link
Contributor

@Oscar-Pepper Oscar-Pepper commented Sep 20, 2024

Problem

Previously zingolib derived an externally scoped BIP32/44 key, and used that as its transparent key. This architecture was not flexible enough to handle the new ephemeral scope, specified in ZIP320.

Solution

This upgrades the WalletCapability to use a keystore backed by a UnifiedSpendingKey/UnifiedFullViewingKey which also includes a fully ZIP320 compliant transparent key type.

  • replaces Capability with UnifiedKeyStore in WalletCapability
  • moves Capability to legacy and removes unecessary impls no longer needed
  • cleans up WalletKind command due to removal of kind_str()
  • updates new_address() to also derive scope with a legacy_key bool for loading old wallet versions with external scope already derived
  • remove unused key related test utils fns
  • added KeyError for improving error handling
  • added fns for converting legacy keys to UnifiedSpendingKey or UnifiedFullViewingKey
  • added an input to Write() in ReadableWriteable trait so chain type can be passed for writing UnifiedFullViewingKeys
  • updated read/write to handle new keys and also load correctly from previous wallet versions
  • added logic to update the keystore with a new UnifiedSpendingKey derived from seed, if the seed exists. The tkey in this UnifiedSpendingKey is fully BIP32/44 compatible and derived to the correct layer as described above.
  • moved key info and transaction context creation to the end of read_internal() after keystore is updated

zingocli/src/main.rs Outdated Show resolved Hide resolved
zingolib/src/commands.rs Show resolved Hide resolved
zingolib/src/lightclient/sync.rs Outdated Show resolved Hide resolved
zingolib/src/testutils.rs Outdated Show resolved Hide resolved
zingolib/src/wallet/keys/unified.rs Show resolved Hide resolved
Copy link
Contributor

@AloeareV AloeareV left a comment

Choose a reason for hiding this comment

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

This all appears to be on the write track! (pardon the pun)

AloeareV
AloeareV previously approved these changes Sep 30, 2024
Copy link
Contributor

@AloeareV AloeareV left a comment

Choose a reason for hiding this comment

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

From a careful readthrough of the new read/write methods, I believe them to be correct.

@Oscar-Pepper Oscar-Pepper marked this pull request as ready for review October 3, 2024 22:08
@Oscar-Pepper Oscar-Pepper marked this pull request as draft October 3, 2024 22:09
@Oscar-Pepper Oscar-Pepper marked this pull request as ready for review October 4, 2024 10:43
Copy link
Contributor

@AloeareV AloeareV left a comment

Choose a reason for hiding this comment

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

I'm worried I'll lose github state if I leave this review open, so here's an incomplete review! I'll be back for that last file

zingolib/src/wallet/keys/legacy.rs Show resolved Hide resolved
@zancas zancas self-assigned this Oct 5, 2024
Copy link
Collaborator

@pacu pacu left a comment

Choose a reason for hiding this comment

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

I don't have strong objections to this PR. Over all is a good improvement. There are existing code smell on critical areas that could be improved given the opportunity and them not being big changes.

nice work

libtonode-tests/tests/concrete.rs Outdated Show resolved Hide resolved
zingolib/src/wallet/disk.rs Show resolved Hide resolved
zingolib/src/wallet/disk.rs Show resolved Hide resolved
zingolib/src/wallet/keys/legacy.rs Outdated Show resolved Hide resolved
zingolib/src/wallet/keys/unified.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@pacu pacu left a comment

Choose a reason for hiding this comment

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

did not test but looks good to me.

@AloeareV AloeareV mentioned this pull request Oct 7, 2024
@zancas
Copy link
Member

zancas commented Oct 7, 2024

I have sanity tested this version locally.

@zancas zancas merged commit fd86965 into zingolabs:dev Oct 7, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants