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

Jump to bottom

GABA: Integrate CurrencyRateController #5844

Closed
wants to merge 1 commit into from
Closed

GABA: Integrate CurrencyRateController #5844

wants to merge 1 commit into from

Conversation

bitpshr
Copy link
Contributor

@bitpshr bitpshr commented Nov 27, 2018

This pull request adds gaba/CurrencyRateController to the extension.

bitpshr
bitpshr commented Nov 27, 2018
View reviewed changes
app/scripts/lib/ComposableObservableStore.js
@@ -40,7 +40,9 @@ class ComposableObservableStore extends ObservableStore {
getFlatState () {
let flatState = {}
for (const key in this.config) {
flatState = { ...flatState, ...this.config[key].getState() }
const controller = this.config[key]
const state = controller.getState ? controller.getState() : controller.state
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a temporary patch to ComposableObservableStore to provide GABA API compatibility. Once we're fully on GABA, this entire module goes away and we'll be using gaba/ComposableController instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment here outlining the two cases we're handling and noting the conditions under which we'd remove this logic?

@bitpshr bitpshr force-pushed the gaba-currencyratecontroller branch 2 times, most recently from ef32ea7 to 6038879 Compare November 27, 2018 22:36
@metamaskbot
Copy link
Collaborator

Builds ready [6038879]: mascara, chrome, firefox, edge, opera

brunobar79
brunobar79 previously approved these changes Nov 27, 2018
View reviewed changes
Copy link
Contributor

@brunobar79 brunobar79 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM!

app/scripts/lib/getObjStructure.js Outdated
@@ -7,7 +7,7 @@ module.exports = getObjStructure

// {
// "data": {
// "CurrencyController": {
// "CurrencyRatesController": {
Copy link
Contributor

@brunobar79 brunobar79 Nov 27, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I assume this is part of a global search and replace, but in that case shouldn't be CurrencyRateController, without the s ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch, this was a manual update. Fixed.

brunobar79
brunobar79 reviewed Nov 27, 2018
View reviewed changes
Copy link
Contributor

@brunobar79 brunobar79 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Ping me once the tests are passing and I'll approve again.

@bitpshr bitpshr force-pushed the gaba-currencyratecontroller branch from 6038879 to ec8285d Compare November 27, 2018 22:58
brunobar79
brunobar79 approved these changes Nov 27, 2018
View reviewed changes
Copy link
Contributor

@brunobar79 brunobar79 left a comment

Choose a reason for hiding this comment

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

LGTM!

@metamaskbot
Copy link
Collaborator

Builds ready [ec8285d]: mascara, chrome, firefox, edge, opera

whymarrh
whymarrh reviewed Nov 29, 2018
View reviewed changes
app/scripts/lib/ComposableObservableStore.js
@@ -40,7 +40,9 @@ class ComposableObservableStore extends ObservableStore {
getFlatState () {
let flatState = {}
for (const key in this.config) {
flatState = { ...flatState, ...this.config[key].getState() }
const controller = this.config[key]
const state = controller.getState ? controller.getState() : controller.state
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a comment here outlining the two cases we're handling and noting the conditions under which we'd remove this logic?

app/scripts/migrations/030.js
@@ -0,0 +1,22 @@
const clone = require('clone')

const version = 30
Copy link
Contributor

Choose a reason for hiding this comment

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

It's worth noting that #5843 is concurrently introducing migration 30

cc @danjm

whymarrh
whymarrh suggested changes Nov 29, 2018
View reviewed changes
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

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

Two small things:

  1. I think we should pin the version of GABA we're pulling in
  2. Can we add a few quick tests for the migration we're introducing?

package.json
@@ -145,6 +145,7 @@
"fast-levenshtein": "^2.0.6",
"file-loader": "^1.1.11",
"fuse.js": "^3.2.0",
"gaba": "^1.0.0-beta.45",
Copy link
Contributor

Choose a reason for hiding this comment

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

We should pin this exactly so that we're not automatically pulling in new prereleases

@bitpshr bitpshr mentioned this pull request Jan 22, 2019
Closed
@danjm danjm assigned danjm and whymarrh and unassigned danjm Feb 12, 2019
@danfinlay danfinlay mentioned this pull request Feb 28, 2019
Merged
@whymarrh
Copy link
Contributor

I'm closing this for now as this will require some work to update (might be worth creating a new PR)

@whymarrh whymarrh closed this Apr 30, 2019
@whymarrh whymarrh deleted the gaba-currencyratecontroller branch January 15, 2020 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@whymarrh whymarrh whymarrh requested changes

@brunobar79 brunobar79 brunobar79 approved these changes

@danfinlay danfinlay Awaiting requested review from danfinlay

@danjm danjm Awaiting requested review from danjm

Assignees

@whymarrh whymarrh

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bitpshr @metamaskbot @whymarrh @brunobar79 @danjm