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

Showing all deposit addresses, memos in the Binance Widget. #5267

Merged
merged 2 commits into from
Apr 30, 2020
Merged

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Apr 16, 2020

Fixes: brave/brave-browser#9250
Fixes: brave/brave-browser#9535

Submitter Checklist:

Test Plan:

Defined in issues

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@ryanml ryanml added this to the 1.9.x - Nightly milestone Apr 16, 2020
@ryanml ryanml requested review from bbondy and emerick April 16, 2020 03:00
@ryanml ryanml self-assigned this Apr 16, 2020
const std::map<std::string, std::string>& balances,
const std::string& asset) {
std::string balance;
std::string GetValueFromStringMap(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figured this would just be better off as a generic prying function

@ryanml ryanml force-pushed the fix-9250 branch 2 times, most recently from ff1a7b9 to 5750d6b Compare April 16, 2020 03:14
chrome.binance.getCoinNetworks((networks: Record<string, string>) => {
const currencies = this.getCurrencyList()
for (let ticker in networks) {
if (currencies.includes(ticker)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll check for only what's in the deposit list, as there are hundreds of currencies in the network endpoints

Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

@ryanml
Copy link
Contributor Author

ryanml commented Apr 21, 2020

Note: The url property wasn't being used anywhere so memo took its place

@ryanml
Copy link
Contributor Author

ryanml commented Apr 30, 2020

Builds generated for all platforms, tests related to Binance passed, known failures for ads confirmations resulted in failure

@ryanml ryanml merged commit d0e88e8 into master Apr 30, 2020
@ryanml ryanml deleted the fix-9250 branch April 30, 2020 02:14
@ryanml ryanml changed the title Showing all deposit addresses in the Binance Widget. Showing all deposit addresses, memos in the Binance Widget. Apr 30, 2020
ryanml added a commit that referenced this pull request May 1, 2020
Showing all deposit addresses in the Binance Widget.
@kjozwiak
Copy link
Member

kjozwiak commented May 3, 2020

Reproduced the issue on macOS 10.15.4 x64 Catalina using the following build:

Brave | 1.8.86 Chromium: 81.0.4044.129 (Official Build) (64-bit)
--- | ---
Revision | 3d71af9f5704a40b85806f4d08925db24605ba25-refs/branch-heads/4044@{#979}
OS | macOS Version 10.15.4 (Build 19E287)
Issue #9250 Issue #9535
Screen Shot 2020-05-03 at 1 25 30 PM Screen Shot 2020-05-03 at 1 18 28 PM

Verification PASSED on macOS 10.15.5 x64 Catalina using the following build:

Brave | 1.10.24 Chromium: 81.0.4044.129 (Official Build) nightly (64-bit)
-- | --
Revision | 3d71af9f5704a40b85806f4d08925db24605ba25-refs/branch-heads/4044@{#979}
OS | macOS Version 10.15.4 (Build 19E287)
Issue #9250 Issue #9535
Screen Shot 2020-05-03 at 1 29 53 PM Screen Shot 2020-05-03 at 1 30 05 PM

Ran into brave/brave-browser#9250 (comment) and created brave/brave-browser#9600.

ryanml added a commit that referenced this pull request May 3, 2020
Showing all deposit addresses in the Binance Widget.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants