-
Notifications
You must be signed in to change notification settings - Fork 56
Sam/remove tx metadata files #689
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,7 +37,7 @@ import { | |
| import { CurrencyWalletInput } from './currency-wallet-pixie' | ||
| import { TxFileNames } from './currency-wallet-reducer' | ||
| import { currencyCodesToTokenIds } from './enabled-tokens' | ||
| import { mergeMetadata } from './metadata' | ||
| import { isEmptyMetadata, mergeMetadata } from './metadata' | ||
|
|
||
| const CURRENCY_FILE = 'Currency.json' | ||
| const LEGACY_MAP_FILE = 'fixedLegacyFileNames.json' | ||
|
|
@@ -46,6 +46,34 @@ const SEEN_TX_CHECKPOINT_FILE = 'seenTxCheckpoint.json' | |
| const TOKENS_FILE = 'Tokens.json' | ||
| const WALLET_NAME_FILE = 'WalletName.json' | ||
|
|
||
| /** | ||
| * Checks if a transaction file is "empty" (contains no user metadata). | ||
| * An empty file is one that matches the default template with no user-added data. | ||
| */ | ||
| export function isEmptyTxFile(file: TransactionFile): boolean { | ||
| // Check top-level user data fields: | ||
| if (file.savedAction != null) return false | ||
| if (file.swap != null) return false | ||
| if (file.payees != null && file.payees.length > 0) return false | ||
| if (file.deviceDescription != null) return false | ||
| if (file.secret != null) return false | ||
| if (file.feeRateRequested != null) return false | ||
|
|
||
| // Check currencies map for non-empty metadata: | ||
| for (const asset of file.currencies.values()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We generally avoid |
||
| if (!isEmptyMetadata(asset.metadata)) return false | ||
| if (asset.assetAction != null) return false | ||
| } | ||
|
|
||
| // Check tokens map for non-empty metadata: | ||
| for (const asset of file.tokens.values()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. |
||
| if (!isEmptyMetadata(asset.metadata)) return false | ||
| if (asset.assetAction != null) return false | ||
| } | ||
|
|
||
| return true | ||
| } | ||
|
|
||
| const legacyAddressFile = makeJsonFile(asLegacyAddressFile) | ||
| const legacyMapFile = makeJsonFile(asLegacyMapFile) | ||
| const legacyTokensFile = makeJsonFile(asLegacyTokensFile) | ||
|
|
@@ -297,7 +325,10 @@ export async function loadTxFiles( | |
| const fileNames = input.props.walletState.fileNames | ||
| const walletFiat = input.props.walletState.fiat | ||
|
|
||
| const out: { [filename: string]: TransactionFile } = {} | ||
| const out: { [txidHash: string]: TransactionFile } = {} | ||
| const emptyFileInfos: Array<{ txidHash: string; path: string }> = [] | ||
|
|
||
| // Load legacy transaction files: | ||
| await Promise.all( | ||
| txIdHashes.map(async txidHash => { | ||
| if (fileNames[txidHash] == null) return | ||
|
|
@@ -307,6 +338,8 @@ export async function loadTxFiles( | |
| out[txidHash] = fixLegacyFile(clean, walletCurrency, walletFiat) | ||
| }) | ||
| ) | ||
|
|
||
| // Load new transaction files: | ||
| await Promise.all( | ||
| txIdHashes.map(async txidHash => { | ||
| if (fileNames[txidHash] == null) return | ||
|
|
@@ -317,6 +350,38 @@ export async function loadTxFiles( | |
| }) | ||
| ) | ||
|
|
||
| // Detect empty files and queue them for deletion: | ||
| for (const txidHash of Object.keys(out)) { | ||
| const file = out[txidHash] | ||
| if (isEmptyTxFile(file)) { | ||
| const path = `transaction/${fileNames[txidHash].fileName}` | ||
| emptyFileInfos.push({ txidHash, path }) | ||
| // Remove from output so it's not loaded into state: | ||
| // eslint-disable-next-line @typescript-eslint/no-dynamic-delete | ||
| delete out[txidHash] | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Empty legacy files deleted at wrong pathThe empty file detection and deletion logic assumes all transaction files in
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this looks like a legitimate path mix-up that Cursor caught. |
||
|
|
||
| // Delete empty files in a non-blocking way (fire-and-forget): | ||
| if (emptyFileInfos.length > 0) { | ||
| const txidHashes: string[] = [] | ||
| for (const info of emptyFileInfos) { | ||
| txidHashes.push(info.txidHash) | ||
| // Delete files without awaiting: | ||
| disklet.delete(info.path).catch(error => { | ||
| input.props.log.warn( | ||
| `Failed to delete empty tx file ${info.path}: ${String(error)}` | ||
| ) | ||
| }) | ||
| } | ||
| // Dispatch action to remove from fileNames state so that loadTxFiles | ||
| // won't attempt to load these empty files again: | ||
| dispatch({ | ||
| type: 'CURRENCY_WALLET_FILE_DELETED', | ||
| payload: { txidHashes, walletId } | ||
| }) | ||
| } | ||
|
|
||
| dispatch({ | ||
| type: 'CURRENCY_WALLET_FILES_LOADED', | ||
| payload: { files: out, walletId } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,14 +6,30 @@ import { EdgeIo } from '../../types/types' | |
| import { decrypt, decryptText, encrypt } from '../../util/crypto/crypto' | ||
| import { utf8 } from '../../util/encoding' | ||
|
|
||
| /** | ||
| * Creates an encrypted disklet that wraps another disklet. | ||
| * Optionally accepts a deletedDisklet for sync-aware deletions. | ||
| * When deletedDisklet is provided, delete operations will mark files | ||
| * for deletion by writing an empty file to the deleted/ directory, | ||
| * which will be processed during the next sync. | ||
| */ | ||
| export function encryptDisklet( | ||
| io: EdgeIo, | ||
| dataKey: Uint8Array, | ||
| disklet: Disklet | ||
| disklet: Disklet, | ||
| /** Provide when this disklet is synchronized with edge-sync-client's syncRepo */ | ||
| deletedDisklet?: Disklet | ||
|
Comment on lines
+20
to
+21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| ): Disklet { | ||
| const out = { | ||
| delete(path: string): Promise<unknown> { | ||
| return disklet.delete(path) | ||
| async delete(path: string): Promise<unknown> { | ||
| // If we have a deletedDisklet, mark the file for deletion | ||
| // by writing an empty file to the deleted/ directory. | ||
| // The sync process will handle the actual deletion. | ||
| if (deletedDisklet != null) { | ||
| await deletedDisklet.setText(path, '') | ||
| } | ||
| // Also delete locally for immediate effect: | ||
| return await disklet.delete(path) | ||
| }, | ||
|
|
||
| async getData(path: string): Promise<Uint8Array> { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Local path dependency will break npm package
The
edge-sync-clientdependency 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.8was replaced with this local path.