Sam/remove tx metadata files #689
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| "currency-codes": "^1.5.1", | ||
| "disklet": "^0.5.2", | ||
| "edge-sync-client": "^0.2.8", | ||
| "edge-sync-client": "../edge-sync-client", |
There was a problem hiding this comment.
Bug: Local path dependency will break npm package
The edge-sync-client dependency is set to a local filesystem path "../edge-sync-client" instead of an npm version. This is development code that will break when the package is published to npm, as the relative path won't exist on consumer machines. The previous version ^0.2.8 was replaced with this local path.
There was a problem hiding this comment.
This will be resolved when edge-sync-client PR is merged and published
| /** Provide when this disklet is synchronized with edge-sync-client's syncRepo */ | ||
| deletedDisklet?: Disklet |
There was a problem hiding this comment.
This deletion logic needs to live in the edge-sync-client, as part of the wrapped disklet. The edge-sync-client needs abstract all the sync stuff (adds / changes / deletions / etc.) behind the simple Disklet API, managing the deletions folder internally.
| import { TransactionFile } from '../../../../src/core/currency/wallet/currency-wallet-cleaners' | ||
| import { isEmptyTxFile } from '../../../../src/core/currency/wallet/currency-wallet-files' | ||
|
|
||
| describe('currency wallet files', function () { |
There was a problem hiding this comment.
I feel like there are too many tests here. You've already covered this logic e2e style in the currency-wallet test, so I would delete these detailed tests. More tests is not more betterer - we don't need extra slop clogging the context window of both humans and machines.
The biggest danger with this new file-deletion logic happens if we add a new field to the transaction file, and forget to add it to one of the isEmpty* functions. Then users with only that field will have their data randomly disappear. However, your tests don't cover this case (and I'm not sure how you test non-existent future code), which means we are getting a lot of false confidence from these tests (wow! so detailed!) while not actually covering the really dangerous case. So yeah, please delete them.
| ).equals(true) | ||
| }) | ||
|
|
||
| it('returns false for metadata with name', function () { |
There was a problem hiding this comment.
Same here. I feel like these could be deleted.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| resolved "https://registry.yarnpkg.com/edge-sync-client/-/edge-sync-client-0.2.8.tgz#150c9d97676b5ddd158efd38565de1be29e525bc" | ||
| integrity sha512-5iaVYvws4fEk9gSkijo98nh46wyT2D0YSAYzwAyM4v/oHu+tVa18WrdGC+aP4HZVcrsg/LzrLu/NX9JyTsW9Cw== | ||
| edge-sync-client@../edge-sync-client: | ||
| version "0.2.9-1" |
There was a problem hiding this comment.
yarn.lock references local path instead of npm package
High Severity
The yarn.lock shows edge-sync-client@../edge-sync-client which is a local filesystem path. This was likely used during development but shouldn't be committed. The package.json specifies "^0.2.8" but the lockfile resolves to a local path, causing builds to fail for anyone who doesn't have that exact directory structure. The lockfile needs to be regenerated from the npm registry version.
| } | ||
|
|
||
| return true | ||
| } |
There was a problem hiding this comment.
Missing feeRateUsed check in isEmptyTxFile function
Medium Severity
The isEmptyTxFile function checks many fields (savedAction, swap, payees, deviceDescription, secret, feeRateRequested) but does not check feeRateUsed. If a transaction file contains only feeRateUsed data (and no user metadata), it will be incorrectly identified as "empty" and deleted. The feeRateUsed value is used as a fallback in combineTxWithFile when the engine doesn't provide it, so deleting such files causes data loss for synced transactions.
"We generally avoid Object.values in the core for historical reasons... So this would become for (const currencyCode of Object.keys(file.currencies) { const asset = file.currencies[currencyCode]; ... }. This keeps things clean on really bad Androids."
Note: Changed to use Map.keys() since currencies/tokens are ES6 Maps.
"The empty file detection and deletion logic assumes all transaction files in out are from the new transaction/ directory. However, out may contain legacy files loaded from Transactions/ directory (if no new-format file exists for that txidHash). When such a legacy file is detected as empty, the code tries to delete from transaction/${fileName} which is incorrect - the file actually exists at Transactions/${fileName}."
Track which txidHashes have new-format files and use correct directory for deletion.
"I feel like there are too many tests here. You've already covered this logic e2e style in the currency-wallet test, so I would delete these detailed tests. More tests is not more betterer - we don't need extra slop clogging the context window of both humans and machines." Remove currency-wallet-files.test.ts and metadata.test.ts per reviewer request.
samholmes
force-pushed
the
sam/remove-tx-metadata-files
branch
from
99b6b3c to
a927c0f
Compare
2 participants
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneDescription
noneNote
Implements automatic cleanup of empty transaction metadata and adopts the upgraded repo sync flow.
loadTxFiles(addsisEmptyMetadata/isEmptyTxFile), firesCURRENCY_WALLET_FILE_DELETED, and updates reducers to drop file entriesencryptDiskletnow marks deletions via adeleted/disklet;makeRepoPathsincludesdeletedDiskletsyncClient.syncRepo;syncStorageWalletpersistsstatus.jsonfrom returnedSyncResultSyncResultfrom edge-sync-client; dependency switched to localedge-sync-clientWritten by Cursor Bugbot for commit a927c0f. This will update automatically on new commits. Configure here.