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.

Sign up for GitHub

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

Tokens per account & network basis #4884

Merged
merged 21 commits into from
Aug 7, 2018
Merged

Tokens per account & network basis #4884

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
2770df8
add & delete tokens per account
estebanmino Jul 25, 2018
adee549
preferences test fixes for tokens per account
estebanmino Jul 25, 2018
1d3ad7c
tokens per network for individual account
estebanmino Jul 26, 2018
2827e13
add & delete tokens per network
estebanmino Jul 26, 2018
f79a249
modify tests according to tokens by network
estebanmino Jul 27, 2018
f5107fa
migration for preferences controller tokens
estebanmino Jul 27, 2018
5b9725d
refactor to accountTokens
estebanmino Jul 27, 2018
228f48c
Merge branch 'develop' into TokensPerAccountBasis
estebanmino Jul 27, 2018
e687bc5
fix migration for preferences controller tokens
estebanmino Jul 27, 2018
dd6a725
unit tests for tokens per account and network
estebanmino Jul 27, 2018
0757f47
passing tokens to all accounts in migration
estebanmino Jul 30, 2018
3128667
isolation of tokens related methods in preferences
estebanmino Jul 30, 2018
fbcba8f
create tokens storage when setadresses
estebanmino Jul 30, 2018
822c173
fix double state update when account changed
estebanmino Jul 31, 2018
22cf5a1
improve comments
estebanmino Jul 31, 2018
ccae79d
fix undefined for tokens on tests
estebanmino Jul 31, 2018
3124e8c
tokens related functions refactor
estebanmino Jul 31, 2018
d8817b2
fix doc tag
estebanmino Jul 31, 2018
c0cdda8
Merge branch 'develop' into TokensPerAccountBasis
estebanmino Aug 7, 2018
b3af9cd
Merge branch 'develop' into TokensPerAccountBasis
estebanmino Aug 7, 2018
66c76c0
CHANGELOG
estebanmino Aug 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/scripts/controllers/detect-tokens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class DetectTokensController {
set preferences (preferences) {
if (!preferences) { return }
this._preferences = preferences
preferences.store.subscribe(({ tokens }) => { this.tokenAddresses = tokens.map((obj) => { return obj.address }) })
preferences.store.subscribe(({ tokens = [] }) => { this.tokenAddresses = tokens.map((obj) => { return obj.address }) })
preferences.store.subscribe(({ selectedAddress }) => {
if (this.selectedAddress !== selectedAddress) {
this.selectedAddress = selectedAddress
Expand Down
82 changes: 68 additions & 14 deletions app/scripts/controllers/preferences.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class PreferencesController {
* @property {array} store.frequentRpcList A list of custom rpcs to provide the user
* @property {string} store.currentAccountTab Indicates the selected tab in the ui
* @property {array} store.tokens The tokens the user wants display in their token lists
* @property {object} store.accountTokens The tokens stored per account and then per network type
* @property {boolean} store.useBlockie The users preference for blockie identicons within the UI
* @property {object} store.featureFlags A key-boolean map, where keys refer to features and booleans to whether the
* user wishes to see that feature
Expand All @@ -24,6 +25,7 @@ class PreferencesController {
const initState = extend({
frequentRpcList: [],
currentAccountTab: 'history',
accountTokens: {},
tokens: [],
useBlockie: false,
featureFlags: {},
Expand All @@ -33,8 +35,10 @@ class PreferencesController {
}, opts.initState)

this.diagnostics = opts.diagnostics

this.network = opts.network
this.store = new ObservableStore(initState)
this._defineTokens()
this._subscribeProviderType()
}
// PUBLIC METHODS

Expand Down Expand Up @@ -77,12 +81,17 @@ class PreferencesController {
*/
setAddresses (addresses) {
const oldIdentities = this.store.getState().identities
const accountTokens = this.store.getState().accountTokens

const identities = addresses.reduce((ids, address, index) => {
const oldId = oldIdentities[address] || {}
ids[address] = {name: `Account ${index + 1}`, address, ...oldId}
return ids
}, {})
this.store.updateState({ identities })
for (const address in identities) {
if (!(address in accountTokens)) accountTokens[address] = {}
}
this.store.updateState({ identities, accountTokens })
}

/**
Expand All @@ -93,11 +102,13 @@ class PreferencesController {
*/
removeAddress (address) {
const identities = this.store.getState().identities
const accountTokens = this.store.getState().accountTokens
if (!identities[address]) {
throw new Error(`${address} can't be deleted cause it was not found`)
}
delete identities[address]
this.store.updateState({ identities })
delete accountTokens[address]
this.store.updateState({ identities, accountTokens })

// If the selected account is no longer valid,
// select an arbitrary other account:
Expand All @@ -117,14 +128,17 @@ class PreferencesController {
*/
addAddresses (addresses) {
const identities = this.store.getState().identities
const accountTokens = this.store.getState().accountTokens
addresses.forEach((address) => {
// skip if already exists
if (identities[address]) return
// add missing identity
const identityCount = Object.keys(identities).length

if (!(address in accountTokens)) accountTokens[address] = {}
identities[address] = { name: `Account ${identityCount + 1}`, address }
})
this.store.updateState({ identities })
this.store.updateState({ identities, accountTokens })
}

/*
Expand Down Expand Up @@ -179,11 +193,16 @@ class PreferencesController {
*
*/
setSelectedAddress (_address) {
return new Promise((resolve, reject) => {
const address = normalizeAddress(_address)
this.store.updateState({ selectedAddress: address })
resolve()
})
const address = normalizeAddress(_address)
const accountTokens = this.store.getState().accountTokens
const providerType = this.network.providerStore.getState().type

if (!(address in accountTokens)) accountTokens[address] = {}
if (!(providerType in accountTokens[address])) accountTokens[address][providerType] = []
const tokens = accountTokens[address][providerType]
this.store.updateState({ selectedAddress: address, tokens })

return Promise.resolve(tokens)
}

/**
Expand Down Expand Up @@ -233,7 +252,11 @@ class PreferencesController {
tokens.push(newEntry)
}

this.store.updateState({ tokens })
const selectedAddress = this.store.getState().selectedAddress
const accountTokens = this.store.getState().accountTokens
const providerType = this.network.providerStore.getState().type
accountTokens[selectedAddress][providerType] = tokens
this.store.updateState({ accountTokens, tokens })
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like there's an opportunity to de-duplicate some logic between here and setAddress.

Or at least, this function is getting a little big, we could probably tuck more of the token managing code into its own methods (eventually might want its own controller).


return Promise.resolve(tokens)
}
Expand All @@ -246,11 +269,13 @@ class PreferencesController {
*
*/
removeToken (rawAddress) {
const tokens = this.store.getState().tokens
const accountTokens = this.store.getState().accountTokens
const selectedAddress = this.store.getState().selectedAddress
const providerType = this.network.providerStore.getState().type
const updatedTokens = accountTokens[selectedAddress][providerType].filter(token => token.address !== rawAddress)
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to normalize the rawAddress, to make sure we're using the same capitalization as the accountTokens object uses. Probably also applies to addToken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The normalization of rawAddress wasn't in the code. I think it is because when you remove a token you select the address from the tokens list on the controller, that is already normalized when it was added with addToken, method that normalize the address, so it may not necessary I believe.

accountTokens[selectedAddress][providerType] = updatedTokens
this.store.updateState({ accountTokens, tokens: updatedTokens })

const updatedTokens = tokens.filter(token => token.address !== rawAddress)

this.store.updateState({ tokens: updatedTokens })
return Promise.resolve(updatedTokens)
}

Expand Down Expand Up @@ -376,6 +401,35 @@ class PreferencesController {
//
// PRIVATE METHODS
//

/**
* Getter definition for the `tokens` property of store when controller is initialized
*
*
*/
_defineTokens () {
const selectedAddress = this.store.getState().selectedAddress
const accountTokens = this.store.getState().accountTokens
const providerType = this.network.providerStore.getState().type
if (!(selectedAddress in accountTokens)) accountTokens[selectedAddress] = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is in at least two other functions. We might want to DRY that up. Then again, if it's only one line, not much to reduce...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good one, this is not necessary since the entry is created for each account when it is added, so the only place where it should be is on addAddresses.

if (!(providerType in accountTokens[selectedAddress])) return []
this.tokens = accountTokens[selectedAddress][providerType]
}

/**
* Subscription to network provider type
*
*
*/
_subscribeProviderType () {
this.network.providerStore.subscribe(({ type }) => {
const selectedAddress = this.store.getState().selectedAddress
const accountTokens = this.store.getState().accountTokens
if (!(type in accountTokens[selectedAddress])) accountTokens[selectedAddress][type] = []
const tokens = accountTokens[selectedAddress][type]
this.store.updateState({ tokens })
})
}
}

module.exports = PreferencesController
1 change: 1 addition & 0 deletions app/scripts/metamask-controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ module.exports = class MetamaskController extends EventEmitter {
this.preferencesController = new PreferencesController({
initState: initState.PreferencesController,
initLangCode: opts.initLangCode,
network: this.networkController,
})

// currency controller
Expand Down
37 changes: 37 additions & 0 deletions app/scripts/migrations/028.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// next version number
const version = 28

/*

normalizes txParams on unconfirmed txs

*/
const clone = require('clone')

module.exports = {
version,

migrate: async function (originalVersionedData) {
const versionedData = clone(originalVersionedData)
versionedData.meta.version = version
const state = versionedData.data
const newState = transformState(state)
versionedData.data = newState
return versionedData
},
}

function transformState (state) {
const newState = state

if (newState.PreferencesController) {
if (newState.PreferencesController.tokens) {
const tokens = newState.TransactionController.tokens
const selectedAddress = newState.PreferencesController.selectedAddress
newState.PreferencesController.tokens = []
newState.PreferencesController.accountTokens = {[selectedAddress]: {'mainnet': tokens}}
Copy link
Contributor

Choose a reason for hiding this comment

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

So this migration removes tokens from all accounts except the main address. I think it would be more cautious/considerate to just keep all their tokens on all their accounts, so they don't notice any abrupt change.

This is so far the only thing I'd briefly block over.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, then users can remove/add tokens as they want.

}
}

return newState
}
1 change: 1 addition & 0 deletions app/scripts/migrations/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,5 @@ module.exports = [
require('./025'),
require('./026'),
require('./027'),
require('./028'),
]
21 changes: 4 additions & 17 deletions test/unit/app/controllers/detect-tokens-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,11 @@ const PreferencesController = require('../../../../app/scripts/controllers/prefe

describe('DetectTokensController', () => {
const sandbox = sinon.createSandbox()
let clock
let keyringMemStore
before(async () => {
let clock, keyringMemStore, network, preferences
beforeEach(async () => {
keyringMemStore = new ObservableStore({ isUnlocked: false})
network = new NetworkController({ provider: { type: 'mainnet' }})
preferences = new PreferencesController({ network })
})
after(() => {
sandbox.restore()
Expand All @@ -25,9 +26,7 @@ describe('DetectTokensController', () => {

it('should be called on every polling period', async () => {
clock = sandbox.useFakeTimers()
const network = new NetworkController()
network.setProviderType('mainnet')
const preferences = new PreferencesController()
const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
controller.isOpen = true
controller.isUnlocked = true
Expand All @@ -45,9 +44,7 @@ describe('DetectTokensController', () => {
})

it('should not check tokens while in test network', async () => {
const network = new NetworkController()
network.setProviderType('rinkeby')
const preferences = new PreferencesController()
const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
controller.isOpen = true
controller.isUnlocked = true
Expand All @@ -61,9 +58,7 @@ describe('DetectTokensController', () => {
})

it('should only check and add tokens while in main network', async () => {
const network = new NetworkController()
network.setProviderType('mainnet')
const preferences = new PreferencesController()
const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
controller.isOpen = true
controller.isUnlocked = true
Expand All @@ -80,9 +75,7 @@ describe('DetectTokensController', () => {
})

it('should not detect same token while in main network', async () => {
const network = new NetworkController()
network.setProviderType('mainnet')
const preferences = new PreferencesController()
preferences.addToken('0x0d262e5dc4a06a0f1c90ce79c7a60c09dfc884e4', 'J8T', 8)
const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
controller.isOpen = true
Expand All @@ -100,9 +93,7 @@ describe('DetectTokensController', () => {
})

it('should trigger detect new tokens when change address', async () => {
const network = new NetworkController()
network.setProviderType('mainnet')
const preferences = new PreferencesController()
const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
controller.isOpen = true
controller.isUnlocked = true
Expand All @@ -112,9 +103,7 @@ describe('DetectTokensController', () => {
})

it('should trigger detect new tokens when submit password', async () => {
const network = new NetworkController()
network.setProviderType('mainnet')
const preferences = new PreferencesController()
const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
controller.isOpen = true
controller.selectedAddress = '0x0'
Expand All @@ -124,9 +113,7 @@ describe('DetectTokensController', () => {
})

it('should not trigger detect new tokens when not open or not unlocked', async () => {
const network = new NetworkController()
network.setProviderType('mainnet')
const preferences = new PreferencesController()
const controller = new DetectTokensController({ preferences: preferences, network: network, keyringMemStore: keyringMemStore })
controller.isOpen = true
controller.isUnlocked = false
Expand Down
Loading