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

Use burner-core for token balances #223

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmihal
Copy link
Collaborator

@dmihal dmihal commented Jun 8, 2019

PR #211 integrated burner-core for reading the main 3 assets (xDai, Dai and Eth), this PR expands on that by using burner-core to read the balance of an additional token.

The largest impact on the codebase is the removal of the ERC20TOKEN and ERC20VENDOR variables, replacing them with data from assets.js, as well as hardcoded contract names.

Copy link
Collaborator

@TimDaub TimDaub left a comment

Choose a reason for hiding this comment

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

Uses burner-core 0.0.4

@dmihal dmihal force-pushed the burner-core-tokens branch 2 times, most recently from d037b1d to 22de168 Compare June 14, 2019 16:02
@dmihal
Copy link
Collaborator Author

dmihal commented Jun 14, 2019

Updated this to use the newest packages, and rebased to the recent changes with burner-core

@dmihal dmihal force-pushed the burner-core-tokens branch 2 times, most recently from 78a95b2 to 42650ba Compare July 1, 2019 13:45
src/App.js Outdated Show resolved Hide resolved
src/App.js Outdated Show resolved Hide resolved
src/assets.js Show resolved Hide resolved
src/components/Exchange.js Outdated Show resolved Hide resolved
@@ -23,6 +24,9 @@ if (process.env.REACT_APP_MODE === 'local') {
}

const assets = [mainAsset, dai, eth];
if (token) {
assets.unshift(token);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mind elaborating what this does?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Assets is an array of all tokens/coins we will display in the app. If the token variable is set here, then we want to add that to the array so we can display it in the list of tokens.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I believe a comment would clear that up nicely. But up to you.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually let's merge it and comment it later.

Copy link
Collaborator

@TimDaub TimDaub left a comment

Choose a reason for hiding this comment

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

I've just taken a look and left some comments. Looks good. Haven't tested locally.

@dmihal dmihal force-pushed the burner-core-tokens branch from 42650ba to 8273d41 Compare July 3, 2019 09:38
@dmihal dmihal force-pushed the burner-core-tokens branch from 8273d41 to 7f6aa5a Compare July 3, 2019 09:44
@dmihal
Copy link
Collaborator Author

dmihal commented Jul 3, 2019

Thanks for the review @TimDaub, you caught some good issues! I've addressed all the issues.

This PR hasn't actually been tested on a token in a bit, I'll try to do a local test soon.

@TimDaub
Copy link
Collaborator

TimDaub commented Jul 3, 2019

This PR hasn't actually been tested on a token in a bit, I'll try to do a local test soon.

OK let me know how it goes and feel free to merge after testing! Thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants