Binance Smart Chain support #2659

kkosiorowska · 2022-11-22T07:43:34Z

Ref #2569

This PR adds initial Binance Smart Chain support.

UI

To Test
Set SUPPORT_BINANCE_SMART_CHAIN to true.

Check that Binance Smart Chain is available on the network list to choose.
Choose Binance Smart Chain and try to send and receive BNB coin.
Try to connect to dapp.

Latest build: extension-builds-2659 (as of Mon, 05 Dec 2022 09:42:41 GMT).

greg-nagy

This is looking good so far :)

Excitedly waiting for this to move out of Draft :)

Note: since you have added swap endpoints you might want to add a token list as well.

greg-nagy · 2022-11-22T10:59:12Z

background/constants/currencies.ts

@@ -87,7 +99,7 @@ export const BTC: NetworkBaseAsset = {
  },
 }

-export const BASE_ASSETS = [ETH, BTC, MATIC, RBTC, OPTIMISTIC_ETH, AVAX]
+export const BASE_ASSETS = [ETH, BTC, MATIC, RBTC, OPTIMISTIC_ETH, AVAX, BNB]


Love to see this list expand :)

0xDaedalus · 2022-11-22T13:28:04Z

Small nit - description is wrong

This PR adds initial Avalanche support.

Asset Price Point should be taken for the correct network. The issue is still on the Overview page.

kkosiorowska · 2022-11-28T14:00:17Z

@jagodarybacka @0xDaedalus @hyphenized @greg-nagy

I have a small problem with a BSC chain. The problem is related to the number of decimals for the selected chain. What I mean is that when we want to get the price point in the selectAssetPricePoint selector (code) instead of finding the pricedAsset for BSC we take the first one found which is for ETH. This results in the wrong price point. See the photo below.

I made some changes to take the price point for the right chain. After the changes, I was able to display the data correctly in the swap and wallet views. I did some tests and noticed that the data is displayed correctly however in the overview view we don't find the price point for ETH for the BSC chain. The total balance is lower which looks strange. I'm not sure if this is the correct way to go and if this solution is acceptable. Please take a look at my solution What do you think about it?

Shadowfiend

Couple of small thoughts.

Shadowfiend · 2022-11-28T16:30:55Z

background/redux-slices/accounts.ts

+        if (assetDecimals > decimals) {
+          amount /= BigInt(`1${"0".repeat(assetDecimals - decimals)}`)
+        }


convertFixedPoint should be your friend here I think.

Great suggestion!

Shadowfiend · 2022-11-28T16:31:34Z

background/redux-slices/accounts.ts

+      let { amount } = combinedAssetAmount
+
+      if (acc[assetSymbol]?.asset) {
+        const { decimals } = acc[assetSymbol].asset as FungibleAsset


This forced cast seems dangerous, no?

Let's change it and make it simpler.

0xDaedalus · 2022-11-28T23:17:38Z

I think that explicitly passing around the network an asset is related to is an acceptable solution here - but I also wonder if we would benefit from attaching an asset's network to the asset object itself - meaning we make homeNetwork a required attribute on AnyAsset.

I believe we have everything we need to do this and it feels like a reasonable enough approach. I guess something I'd like to get people's opinion on is if the following mental model makes sense:

tokens with the same symbol and contract code on different networks are different assets
tokens bridged across multiple networks (like USDC) are different assets even though they represent the same underlying
therefore - an asset can only belong to one network - and must have a homeNetwork.

jagodarybacka · 2022-11-30T11:18:53Z

From the user perspective I agree with @0xDaedalus points - different network should mean it is a different asset even if symbol, name, contract are the same.

I can't find more in Figma but I see that @VladUXUI tried adding network icon to assets. We can think about what is the best way to display it (sum similar assets balance across networks, group them together but show balance on each chain separately...).

This would need some more work on the Portfolio page so I think it's not in the BSC scope.

jagodarybacka

Quick comment + I think it doesn't look the best multiline, can we make the label here shorter or maybe add ellipsis? cc @VladUXUI

jagodarybacka · 2022-11-30T12:29:46Z

ui/utils/constants.ts

@@ -23,6 +24,10 @@ export const scanWebsite = {
  [GOERLI.chainID]: { title: "Etherscan", url: "https://goerli.etherscan.io/" },
  [ARBITRUM_ONE.chainID]: { title: "Arbiscan", url: "https://arbiscan.io/" },
  [AVALANCHE.chainID]: { title: "Snowtrace", url: "https://snowtrace.io/" },
+  [BINANCE_SMART_CHAIN.chainID]: {
+    title: "Bscscan",


Suggested change

title: "Bscscan",

title: "BscScan",

…e-smart-chain

hyphenized

Superb work @kkosiorowska 💯! We're so close to shipping this!

I think it's a good idea to include the network in the asset, adding chainId should be enough but this change might be more involved than just fixing the price point handling. Left some comments on the issue.

hyphenized · 2022-12-01T21:35:03Z

background/constants/networks.ts

@@ -142,6 +154,7 @@ export const CHAIN_ID_TO_RPC_URLS: {
  [ARBITRUM_NOVA.chainID]: ["https://nova.arbitrum.io/rpc	"],
  [GOERLI.chainID]: ["https://ethereum-goerli-rpc.allthatnode.com"],
  [AVALANCHE.chainID]: ["https://api.avax.network/ext/bc/C/rpc"],
+  [BINANCE_SMART_CHAIN.chainID]: ["https://rpc.ankr.com/bsc"],


Let's add the official one as well https://bsc-dataseed.binance.org

hyphenized · 2022-12-02T02:36:54Z

background/redux-slices/accounts.ts

+        if (targetDecimals > decimals) {
+          amount = convertFixedPoint(
+            combinedAssetAmount?.amount,
+            decimals,
+            targetDecimals
+          )
+        }


We should be converting here from newAmountDecimals to existingDecimals right? Otherwise the balance is wrong after the object spread below replaces the existing asset (and it's decimals).

Suggested change

if (targetDecimals > decimals) {

amount = convertFixedPoint(

combinedAssetAmount?.amount,

decimals,

targetDecimals

)

}

if (newDecimals !== existingDecimals) {

amount = convertFixedPoint(amount, newDecimals, existingDecimals)

}

Then we can change the existing logic to not overwrite the existing asset

if (acc[assetSymbol]) { acc[assetSymbol].amount += amount } else { acc[assetSymbol] = { ...combinedAssetAmount, } }

Good catch!

hyphenized · 2022-12-02T02:42:33Z

background/redux-slices/assets.ts

-    const pricedAsset = assets.find(
+  [
+    selectAssetsState,
+    selectAssetSymbol,


We can pass the asset object here instead of the symbol and chainId separately. I suspect we were using the symbol because it's easier to find an existing price point, but, that's now causing issues for assets with different decimals because this is a best-effort match. For base network assets, it should be enough to find a price point with the symbol alone.

If we pass the asset here we should be able to match by the symbol and use convertFixedPoint to transform the pricePoint amount to be more accurate for the wanted asset. We can improve this to take into account BaseNetworkAssets and SmartContractFungible assets too but I think it would be better to address this in another issue.

I agree with you that maybe we should pass on the whole asset here. However, we will probably want to expand the assets to include the home network. Let's fix it in another issue.

This is def strange here. I think creating a new ticket was a good call for this work.

…e-smart-chain

kkosiorowska · 2022-12-02T09:46:44Z

@jagodarybacka @0xDaedalus @hyphenized @greg-nagy

I created a separate task to distinguish assets. Please take a look at it and let me know if you agree. #2709

VladUXUI · 2022-12-05T08:54:50Z

Quick comment + I think it doesn't look the best multiline, can we make the label here shorter or maybe add ellipsis? cc @VladUXUI

This might be a noob question, but as far as i know it's been called BNB Chain for some time now, since BSC merged with BNB? So maybe let's call it BNB Chain?
https://www.binance.com/en/blog/ecosystem/introducing-bnb-chain-the-evolution-of-binance-smart-chain-421499824684903436

VladUXUI · 2022-12-05T08:59:20Z

@0xDaedalus and @jagodarybacka good call-out.
Yes each asset should have an network icon attached to it.
Idea is that if you have the same asset on different chains, we would show you the total + a breakdown per chain as an expandable menu.
The network icons for assets have been designed for some time, but i did not touch the "same asset on multiple chains" yet

Because Today and Tomorrow are sort of full, i think we can go ahead and show each asset separately with network icon. Even if they are same asset on different network.
We can improve the UX of this in the future

greg-nagy · 2022-12-05T17:08:53Z

background/redux-slices/assets.ts

-    const pricedAsset = assets.find(
+  [
+    selectAssetsState,
+    selectAssetSymbol,


This is def strange here. I think creating a new ticket was a good call for this work.

greg-nagy · 2022-12-05T17:19:47Z

background/redux-slices/selectors/accountsSelectors.ts

@@ -82,7 +82,8 @@ const computeCombinedAssetAmountsData = (
  assets: AssetsState,
  mainCurrencySymbol: string,
  currentNetwork: EVMNetwork,
-  hideDust: boolean
+  hideDust: boolean,
+  useCurrentNetwork = false


The API naming is misleading here because it comes from the selector api.
Here it means every network every asset sum or current network every asset sum

I would have used something like onlyForCurrentNetwork

greg-nagy

looking good, and this is a big win 👸🚀

let's fix the getAssetPricePoint, chainID weirdness in #2709

Karolina Kosiorowska added 9 commits November 21, 2022 11:33

Add feature flag for Binance Smart Chain

5754ab6

Add Binance Smart Chain constants

05f96a0

Add Binance Smart Chain to the list of protocols in the top menu

2aa6920

Rename icons for Binance Smart Chain

57a4b74

Add icon for BNB coin

8ac10df

Add Binance Smart Chain to supported networks

797869e

Merge branch 'main' into add-binance-smart-chain

599a9b2

Rename title for Binance Smart Chain scan website

2f9191d

Fix the issue for getting gas prices for Binance Smart Chain

fdfb95f

greg-nagy reviewed Nov 22, 2022

View reviewed changes

Karolina Kosiorowska added 12 commits November 22, 2022 16:04

Add a token token list for BSC

7a7c9f2

Merge branch 'main' into add-binance-smart-chain

cc25336

Update coin type for BNB

379717c

Merge branch 'main' into add-binance-smart-chain

a7cb97b

Add network chart color for Binance Smart Chain

16da743

Fix the issue with displaying the asset amount for BSC

07fe5ba

Asset Price Point should be taken for the correct network. The issue is still on the Overview page.

Fix the issue with displaying the asset amount for BSC for overview page

fe6c8c5

Merge branch 'main' into add-binance-smart-chain

9265b9c

Merge branch 'main' into add-binance-smart-chain

1476c03

Merge branch 'main' into add-binance-smart-chain

3bf8cb0

Delete unnecessary comment

a508ad8

Merge branch 'main' into add-binance-smart-chain

65db03a

kkosiorowska marked this pull request as ready for review November 28, 2022 14:00

kkosiorowska requested review from hyphenized, greg-nagy, 0xDaedalus and jagodarybacka November 28, 2022 14:06

Shadowfiend reviewed Nov 28, 2022

View reviewed changes

kkosiorowska added 3 commits November 29, 2022 10:39

Merge branch 'main' into add-binance-smart-chain

1304ccb

Refactor for updating combined data function

5f1bb26

Merge branch 'main' into add-binance-smart-chain

8ed47c9

jagodarybacka reviewed Nov 30, 2022

View reviewed changes

Karolina Kosiorowska added 3 commits November 30, 2022 15:29

Rename title for BSC scan website

695715a

Merge branch 'main' into add-binance-smart-chain

f3673b0

Merge branch 'main' of github.com:tallycash/extension into add-binanc…

7cb4384

…e-smart-chain

hyphenized assigned kkosiorowska Dec 1, 2022

hyphenized reviewed Dec 2, 2022

View reviewed changes

Karolina Kosiorowska added 3 commits December 2, 2022 08:43

Merge branch 'main' of github.com:tallycash/extension into add-binanc…

d40bd51

…e-smart-chain

Refactor for update combined data function

017c09e

Add a new RPC url for BSC

45a2a1f

kkosiorowska mentioned this pull request Dec 2, 2022

Distinguish assets through home network #2709

Open

3 tasks

kkosiorowska requested review from hyphenized and jagodarybacka December 2, 2022 09:47

Rename BNB Smart Chain to BNB Chain

6cd98f5

greg-nagy reviewed Dec 5, 2022

View reviewed changes

greg-nagy approved these changes Dec 5, 2022

View reviewed changes

greg-nagy merged commit b9abfe6 into main Dec 5, 2022

greg-nagy deleted the add-binance-smart-chain branch December 5, 2022 17:32

greg-nagy mentioned this pull request Dec 6, 2022

v0.18.4 #2727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Binance Smart Chain support #2659

Binance Smart Chain support #2659

kkosiorowska commented Nov 22, 2022 •

edited by github-actions bot

Loading

greg-nagy left a comment

greg-nagy Nov 22, 2022

0xDaedalus commented Nov 22, 2022

kkosiorowska commented Nov 28, 2022

Shadowfiend left a comment

Shadowfiend Nov 28, 2022

kkosiorowska Nov 29, 2022

Shadowfiend Nov 28, 2022

kkosiorowska Nov 29, 2022

0xDaedalus commented Nov 28, 2022 •

edited

Loading

jagodarybacka commented Nov 30, 2022

jagodarybacka left a comment

jagodarybacka Nov 30, 2022

hyphenized left a comment

hyphenized Dec 1, 2022

hyphenized Dec 2, 2022

kkosiorowska Dec 2, 2022

hyphenized Dec 2, 2022

kkosiorowska Dec 2, 2022

greg-nagy Dec 5, 2022

kkosiorowska commented Dec 2, 2022

VladUXUI commented Dec 5, 2022

VladUXUI commented Dec 5, 2022

greg-nagy Dec 5, 2022

greg-nagy Dec 5, 2022

greg-nagy left a comment

Binance Smart Chain support #2659

Binance Smart Chain support #2659

Conversation

kkosiorowska commented Nov 22, 2022 • edited by github-actions bot Loading

greg-nagy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xDaedalus commented Nov 22, 2022

kkosiorowska commented Nov 28, 2022

Shadowfiend left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xDaedalus commented Nov 28, 2022 • edited Loading

jagodarybacka commented Nov 30, 2022

jagodarybacka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyphenized left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkosiorowska commented Dec 2, 2022

VladUXUI commented Dec 5, 2022

VladUXUI commented Dec 5, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg-nagy left a comment

Choose a reason for hiding this comment

kkosiorowska commented Nov 22, 2022 •

edited by github-actions bot

Loading

0xDaedalus commented Nov 28, 2022 •

edited

Loading