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

CoinZark Swap Plugin #15

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

CoinZark Swap Plugin #15

wants to merge 4 commits into from

Conversation

gertjaap
Copy link

@gertjaap gertjaap commented May 2, 2019

This pull request includes the changes for the integration of CoinZark as an Exchange provider in the Edge wallet. See also The related PR in edge-react-gui

swansontec
swansontec previously approved these changes May 2, 2019
Copy link
Contributor

@swansontec swansontec left a comment

Choose a reason for hiding this comment

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

Generally looks good. I just noticed a few minor style things, which you can either fix or leave at your discretion.

// CoinZark will return canDeposit / canReceive as status of the
// coins. The coin we want to exchange from should have canDeposit enabled
// and the coin we want to exchange to should have canReceive enabled.
if (!(currencies === null || currencies.result === null)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We prefer double-equals with null ( == null). This is the one exception to the === everywhere rule, since the == catches both null and undefined, and basically means "doesn't exist".

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, reading it again, (currencies != null && currencies.result != null) might be even more obvious.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed, i've changed this.

fromCorrect = true
}

if (curr.id === toCurrencyCode && curr.canReceive === 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we treat canReceive like a boolean, removing the === 1 part? Otherwise, if the API ever returned true in the future, this would break.

Copy link
Author

Choose a reason for hiding this comment

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

The API returns a number here. If the API ever changes, it will be published as a new version and will require a change in the plug-in regardless.

@swansontec
Copy link
Contributor

A note on scheduling: We are currently in QA on a release right now, so master will hopefully be open for merging early next week.

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