Skip to content
This repository was archived by the owner on Dec 16, 2024. It is now read-only.

Create addresses which are found in the network but are not in wallet-cli#184

Open
Daksh14 wants to merge 3 commits into
mainfrom
restore_addresses
Open

Create addresses which are found in the network but are not in wallet-cli#184
Daksh14 wants to merge 3 commits into
mainfrom
restore_addresses

Conversation

@Daksh14

@Daksh14 Daksh14 commented Aug 22, 2023

Copy link
Copy Markdown
Contributor

If the a note is owned by a vk and that vk isn't in wallet, we create it in sequence and then save latest wallet file

Closes #80

@HDauven HDauven left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Please do update the changelog though.

Comment thread src/clients/sync.rs Outdated
Comment thread src/clients/sync.rs Outdated
if vk.owns(&note) {
if let Some(existing_addresses) = existing_addresses {
if existing_addresses.get(i).is_none() {
addresses_to_create.add_assign(1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This way, you are loosing the index of the address you need to create.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Additionally:

  • if you find multiple notes belonging to the same address, you're counting them multiple times
  • if there are no existing_address you're not counting at all
  • even if the above are fixed, what if I have 5 existing address and I find a note that belong to the address with index=50? Remember that we check notes against all MAX_Addresses

I suggest to change the behaviors to return the "max_address_index" found instead of the missing addresses count, and then create all the missing address to reach "max_address_index"

Anyway, some tests for this new feature would be welcome

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

del

3 participants