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

fix: Windows replace use of non-ascii filename chars #1017

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 16 additions & 5 deletions packages/shared/lib/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -227,25 +227,36 @@ export const stripSpaces = (str) => {
* @returns The copied object
*/
export function deepCopy(obj) {
if(typeof obj !== 'object' || obj === null) {
if (typeof obj !== 'object' || obj === null) {
return obj;
}

if(obj instanceof Date) {
if (obj instanceof Date) {
return new Date(obj.getTime());
}

if(obj instanceof Array) {
if (obj instanceof Array) {
return obj.reduce((arr, item, i) => {
arr[i] = deepCopy(item);
return arr;
}, []);
}

if(obj instanceof Object) {
if (obj instanceof Object) {
return Object.keys(obj).reduce((newObj, key) => {
newObj[key] = deepCopy(obj[key]);
return newObj;
}, {})
}
}
}

/**
* Encode Non ASCII characters to escaped characters.
* @param value The value to encode.
* @returns The encoded value.
*/
export function encodeNonASCII(value: string): string | undefined {
return typeof value === "string"
? value.replace(/[\u007F-\uFFFF]/g, chr => `${(`0000${chr.charCodeAt(0).toString(16)}`).slice(-4)}`)
: undefined;
}
7 changes: 3 additions & 4 deletions packages/shared/lib/profile.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { AvailableExchangeRates } from 'shared/lib/currency'
import { persistent } from 'shared/lib/helpers'
import { generateRandomId } from 'shared/lib/utils'
import { asyncRemoveStorage, destroyActor, getStoragePath } from 'shared/lib/wallet'
import { destroyActor, getStoragePathParts } from 'shared/lib/wallet'
import { derived, get, Readable, writable } from 'svelte/store'
import type { ChartSelectors } from './chart'
import { Electron } from './electron'
Expand Down Expand Up @@ -252,9 +252,8 @@ export const cleanupInProgressProfiles = async () => {
*/
export const removeProfileFolder = async (profileName) => {
try {
const userDataPath = await Electron.getUserDataPath()
const profileStoragePath = getStoragePath(userDataPath, profileName)
await Electron.removeProfileFolder(profileStoragePath)
const storagePathParts = await getStoragePathParts(profileName)
await Electron.removeProfileFolder(storagePathParts.path)
} catch (err) {
console.error(err)
}
Expand Down
27 changes: 23 additions & 4 deletions packages/shared/lib/wallet.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { mnemonic } from 'shared/lib/app'
import { convertToFiat, currencies, CurrencyTypes, exchangeRates, formatCurrency } from 'shared/lib/currency'
import { stripTrailingSlash } from 'shared/lib/helpers'
import { encodeNonASCII, stripTrailingSlash } from 'shared/lib/helpers'
import { localize } from 'shared/lib/i18n'
import type { PriceData } from 'shared/lib/marketData'
import { HistoryDataProps } from 'shared/lib/marketData'
Expand All @@ -25,6 +25,7 @@ import type { Message, Payload, Transaction } from 'shared/lib/typings/message'
import type { MigrationBundle, MigrationData, SendMigrationBundleResponse } from 'shared/lib/typings/migration'
import { formatUnitBestMatch } from 'shared/lib/units'
import { get, writable, Writable } from 'svelte/store'
import { Electron } from './electron'
import type { ClientOptions } from './typings/client'
import type { Duration, NodeInfo, StrongholdStatus } from './typings/wallet'

Expand Down Expand Up @@ -225,8 +226,26 @@ export const api: {
),
} = window['__WALLET_API__']

export const getStoragePath = (appPath: string, profileName: string): string => {
return `${appPath}/${WALLET_STORAGE_DIRECTORY}/${profileName}`
export const getStoragePathParts = async (profileName: string): Promise<{
path: string
sep: string
}> => {
const userDataPath = await Electron.getUserDataPath()
const os = await Electron.getOS()
let profileDirName = profileName
let sep;

if (os === 'win32') {
sep = '\\'
profileDirName = encodeNonASCII(profileDirName)
} else {
sep = '/'
}

return {
path: `${userDataPath}${sep}${WALLET_STORAGE_DIRECTORY}${sep}${profileDirName}`,
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to do this for the userDataPath too? For example if the path is C:\Users\üsername\AppData\Roaming\Firefly

Copy link
Contributor Author

@obany obany Apr 26, 2021

Choose a reason for hiding this comment

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

Oh yeah, actually that means this PR won't work as we don't have control over the replacing the characters in a user name, it has to be fixed with the RocksDB version :(

Copy link
Member

Choose a reason for hiding this comment

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

Ah that's annoying. I've opened rust-rocksdb/rust-rocksdb#512 to see if there was a reason that flag was removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the issue is in wallet.rs or stronghold then 🤔

Copy link
Contributor Author

@obany obany Apr 26, 2021

Choose a reason for hiding this comment

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

I traced through the code and its definitely in RocksDB its failing, specifically this line https://github.com/facebook/rocksdb/blob/cc1c3ee54eace876ad18c39f931e8e5039823930/port/win/env_win.cc#L1179

I am going to build a version locally to see if I can get it to work with the correct options.

Copy link
Contributor Author

@obany obany Apr 26, 2021

Choose a reason for hiding this comment

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

Managed to build a version 0.16.0 with the ROCKSDB_WINDOWS_UTF8_FILENAMES flag set, built wallet.rs to reference, and then used the new version with FF, and it works 🎉

Although there was some C++ code in rocksdb that wouldn't compile with the flag set, so had to update it 😅

image

Copy link
Member

Choose a reason for hiding this comment

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

Nice! 🎉

sep
}
}

export const initialise = (id: string, storagePath: string): void => {
Expand Down Expand Up @@ -1617,7 +1636,7 @@ export const findAccountWithAddress = (address: string): WalletAccount | undefin
* @param excludeFirst A wallet to exclude on first pass
* @returns The wallet account matching the address or undefined if not found
*/
export const findAccountWithAnyAddress = (addresses: string[], excludeFirst?: WalletAccount): WalletAccount | undefined => {
export const findAccountWithAnyAddress = (addresses: string[], excludeFirst?: WalletAccount): WalletAccount | undefined => {
if (!addresses || addresses.length === 0) {
return
}
Expand Down
37 changes: 20 additions & 17 deletions packages/shared/routes/login/views/EnterPin.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@
import { showAppNotification } from 'shared/lib/notifications'
import { activeProfile } from 'shared/lib/profile'
import { validatePinFormat } from 'shared/lib/utils'
import { api, getStoragePath, initialise } from 'shared/lib/wallet'
import { api, getStoragePathParts, initialise } from 'shared/lib/wallet'
import { createEventDispatcher, onDestroy } from 'svelte'
import { get } from 'svelte/store'

export let locale
export let mobile
export let mobile

let attempts = 0
let pinCode = ''
let isBusy = false
Expand Down Expand Up @@ -72,21 +72,24 @@
Electron.PincodeManager.verify(profile.id, pinCode)
.then((verified) => {
if (verified === true) {
return Electron.getUserDataPath().then((path) => {
initialise(profile.id, getStoragePath(path, profile.name))
api.setStoragePassword(pinCode, {
onSuccess() {
dispatch('next')
},
onError(err) {
isBusy = false
showAppNotification({
type: 'error',
message: locale(err.error),
})
},
return getStoragePathParts(profile.name)
.then((storagePathParts) => {
initialise(profile.id, storagePathParts.path)
})
.then(() => {
api.setStoragePassword(pinCode, {
onSuccess() {
dispatch('next')
},
onError(err) {
isBusy = false
showAppNotification({
type: 'error',
message: locale(err.error),
})
},
})
})
})
} else {
shake = true
shakeTimeout = setTimeout(() => {
Expand Down
10 changes: 4 additions & 6 deletions packages/shared/routes/setup/Congratulations.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
import { LOG_FILE_NAME, migration, resetMigrationState, totalMigratedBalance } from 'shared/lib/migration'
import { activeProfile, newProfile, profileInProgress, saveProfile, setActiveProfile } from 'shared/lib/profile'
import { formatUnitBestMatch } from 'shared/lib/units'
import { getStoragePath } from 'shared/lib/wallet'
import { getStoragePathParts } from 'shared/lib/wallet'
import { createEventDispatcher, onDestroy, onMount } from 'svelte'
import { get } from 'svelte/store'

Expand Down Expand Up @@ -50,11 +50,9 @@

const handleContinueClick = () => {
if (wasMigrated) {
Electron.getUserDataPath()
.then((path) => {
const source = getStoragePath(path, $activeProfile.name)

return Electron.exportMigrationLog(`${source}/${LOG_FILE_NAME}`, `${$activeProfile.name}-${LOG_FILE_NAME}`)
getStoragePathParts($activeProfile.name)
.then((storagePathParts) => {
return Electron.exportMigrationLog(`${storagePathParts.path}${storagePathParts.sep}${LOG_FILE_NAME}`, `${$activeProfile.name}-${LOG_FILE_NAME}`)
})
.then((result) => {
if (result) {
Expand Down
6 changes: 3 additions & 3 deletions packages/shared/routes/setup/Profile.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
profileInProgress,
profiles,
} from 'shared/lib/profile'
import { destroyActor, getStoragePath, initialise, MAX_PROFILE_NAME_LENGTH } from 'shared/lib/wallet'
import { destroyActor, getStoragePathParts, initialise, MAX_PROFILE_NAME_LENGTH } from 'shared/lib/wallet'
import { createEventDispatcher } from 'svelte'
import { get } from 'svelte/store'

Expand Down Expand Up @@ -72,8 +72,8 @@
profile = createProfile(trimmedProfileName, false)
profileInProgress.set(trimmedProfileName)

const userDataPath = await Electron.getUserDataPath()
initialise($newProfile.id, getStoragePath(userDataPath, $newProfile.name))
const storagePathParts = await getStoragePathParts($newProfile.name)
initialise($newProfile.id, storagePathParts.path)

initialiseMigrationListeners()
}
Expand Down