feat: back up hardware wallet names - #681
Draft
jvsena42 wants to merge 7 commits into
Draft
Conversation
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This PR:
Ports synonymdev/bitkit-android#1173, the follow-up to bitkit-android#1163 that this repo ported in #675.
Description
#675 made hardware wallet tags survive a restore: they ride the metadata backup as pre-activity metadata keyed by the Core-derived wallet id, and Core re-attaches them once the device watcher recreates the activities. Two gaps were left behind, and both exist here exactly as they did on Android.
The name itself was never backed up. It lives only on the local device entry, so restoring on a new phone and pairing the Trezor again brought the tags back but not the name the user chose. Nothing even marked a backup as required when a name changed, so a rename was invisible to the backup system entirely.
The second gap is the more damaging one. Removing a hardware wallet destroyed its tags in the backup, silently. Core deletes a wallet's activities, its activity tags and its pre-activity metadata in one cascade, which is both of the sources the metadata envelope draws hardware tags from, and the deletion then signals that tags changed, so a metadata envelope without them was uploaded within seconds. The dialog said only that funds are safe and coins will not be deleted; it never mentioned the tags. A full wallet wipe was never affected, because wiping suppresses uploads and leaves the stored backup intact.
The name travels on the same key the tags already use. Core's wallet id derives from a device's account extended public keys, is stable across installs and platforms, and does not depend on the order of those keys, so a name keyed by it survives a restore exactly as well as the tags do.
The name of a paired wallet still lives on its device entry, which is untouched. What is new is a small pending map for the two moments when no device entry exists: a name restored from a backup before the device has been paired again, and a name kept when a wallet was removed. Pairing adopts a pending name into the entry's own label, and a wallet the device list already names is masked out of the pending set rather than pruned from it, so consuming a name needs no second write that could be lost on its own.
Removal takes the name and tag snapshot before the cascade and writes the tags back after it, carrying the name in the same store write that forgets the device entries. The two kinds of data are kept independently: a wallet that was never renamed still keeps its tags, which is the more common shape by far. Turning the toggle off is the current behaviour made explicit: the tags go, and any name kept from an earlier removal of the same wallet is dropped too.
The write-back is the tail of every delete of that wallet rather than a step in the removal alone. Core drops a wallet's pre-activity metadata along with its activities whether or not any matched, so a cleanup delete arriving later repairs itself instead of taking the rows a removal deliberately kept, and a later removal that keeps nothing is what clears the repair.
Two failure points are treated differently on purpose, because the cascade sits between them. Failing to read the tags happens before anything is deleted, so the wallet is untouched and the removal is refused with a message naming the way through — retry, or remove without keeping the data. Failing to write them back happens after the activities are already gone and the watchers already stopped, where there is nothing to roll back to and reporting a failed removal would be false, so that one is logged and the removal completes.
Two pre-existing bugs surfaced while wiring this up and are fixed here. Removing a wallet never dropped it from the manager's device list, so the push that follows deleted its activities a second time, and the device grouping kept yielding the removed group long enough for a watcher event still in flight to re-persist the activities that had just been deleted. The wallet now leaves the list synchronously, inside the removal. The existing test asserting that a removal deletes a wallet's activities exactly once passed only because it ran without a device session, so no push ever happened; it now performs that push.
One behaviour falls out of this for free: re-pairing a wallet that was removed with the toggle on restores its tags with no restore flow involved at all, because the pre-activity metadata rows survived locally and Core re-attaches them as the watcher recreates the activities.
The envelope is shared with Android, and this adds a field to it rather than reusing one, so both sides need it. Swift ignores unknown keys when decoding but drops them when it re-encodes, and this app re-uploads the metadata envelope often, so until this ships a user running both apps on one seed would lose their backed-up names. The wallet-scoped tag read that removal uses orders rendered tags ahead of stored rows, the opposite of the Android equivalent and for the same reason already documented for the envelope build here: a stored row can outlive the activity it was meant for and hold tags the user has since edited, and keeping a wallet's tags means keeping what the user currently sees.
The remove confirmation is a card rather than a native alert, which cannot hold a switch. There was no existing dialog component and no design for this screen, so it is built from the app's own tokens in the shape the Android dialog uses.
Envelope format is otherwise unchanged and bitkit-core is not bumped.
Companion Android PR: synonymdev/bitkit-android#1173
Linked Issues/Tasks
Screenshot / Video
QA Notes
Requires a paired Trezor (the bitkit-docker emulator is enough) and a wallet with existing hardware activity. Manual tests below have not been run yet.
Manual Tests
regression:Disconnect and reconnect the device without renaming → Settings → Data Backups: Tags is not re-uploaded.regression:Remove dialog → cancel: the wallet stays paired with its name and tags.Automated Checks
BitkitTests/TrezorKnownDeviceStorageTests.swift, covering a pending name and the device list written together, a paired wallet masking its own pending entry, a paired name winning over a pending one, a restore letting a local name win and never clearing on an envelope without the field, forgetting a wallet dropping the name kept for it, a wipe clearing both keys, and the backup signal firing on a rename but not on a reconnect.BitkitTests/HwWalletManagerTests.swift, covering the tag write-back following Core's cascade, a later cleanup delete re-applying the kept rows, a removal deleting a wallet's activities exactly once, the toggle-off branch reading and writing nothing, an unreadable tag snapshot refusing the removal with the wallet left entirely untouched, a removal surviving a failed write-back, a keep-off removal disarming an earlier repair, and a watcher event after removal no longer re-persisting the wallet.BitkitTests/HwWalletManagerTests.swiftnow performs the device push that follows a removal. Without it the test passed vacuously, which is what hid the double delete.BitkitTests/HwWalletManagerPassphraseTests.swiftnow asserts that the stray wallet a typo opened keeps its backup data, since storing it already consumed any name restored for it.BitkitTests/PubkyProfileManagerTests.swift, alongside the existing payload tests, covering the new field surviving a round trip and an envelope written without it decoding to nil rather than an explicit null.AddressTypeIntegrationTestsskipped. The 2 failures areUtxoSelectionTestsfailing onFailed to deposit to address: Blocktank error, which needs the bitkit-docker regtest stack;AddressTypeIntegrationTestsfails for the same reason. Both are unrelated to this change.node scripts/validate-translations.js: 0 errors, 3 new strings.swiftformat .: clean.