-
Notifications
You must be signed in to change notification settings - Fork 5k
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
Tokens per account & network basis #4884
Conversation
Fixes #3521 |
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.
I have one change I'd like made on the migration. Otherwise, just a few style comments, but otherwise good to go from me!
const accountTokens = this.store.getState().accountTokens | ||
const providerType = this.network.providerStore.getState().type | ||
accountTokens[selectedAddress][providerType] = tokens | ||
this.store.updateState({ accountTokens, tokens }) |
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.
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).
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) |
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.
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
?
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.
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.
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] = {} |
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.
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...
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.
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
.
app/scripts/migrations/028.js
Outdated
const tokens = newState.TransactionController.tokens | ||
const selectedAddress = newState.PreferencesController.selectedAddress | ||
newState.PreferencesController.tokens = [] | ||
newState.PreferencesController.accountTokens = {[selectedAddress]: {'mainnet': tokens}} |
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.
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.
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.
Sounds good, then users can remove/add tokens as they want.
dispatch(actions.hideLoadingIndication()) | ||
if (err) { | ||
return dispatch(actions.displayWarning(err.message)) | ||
} | ||
dispatch(updateTokens(tokens)) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Code looks good to me and I QA'd in the browser. I am approving, but @danfinlay should verify that the changes he requested to the migration are good to go. |
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.
Looks good, thank you!!
Related to #3521
This PR add a feature to have tokens per account and network.