BEP70 - Support busd pair listing and trading #710

forcodedancing · 2020-03-18T03:53:46Z

Description

support busd pair listing and trading

Rationale

we will have a BEP for supporting BUSD pairs, to bring more market liquidity and trading choices for users.

Example

BUSD-BD1 --- XYZ-000
or
XYZ000 --- BUSD-BD1
pairs can be listed and traded.

Changes

Notable changes:

add configuration to app.toml for indicating which BUSD is the correct one we are going to support. Considering there could be different BUSD tokens in one chain or cross chains.
add a flag for upgrade [the BEP_BUSD flag will be renamed once we have the final BEP NO.]
change listing/proposing logic a bit
change fee calculation

Preflight checks

build passed (make build)
tests passed (make test)
integration tests passed (make integration_test)
manual transaction test passed (cli invoke)

Already reviewed by

...

Related issues

... reference related issue #'s here ...

add busd pair support, following changes not implmented yet 1) a method to define the standard BUSD token 2) BEP number change

add configuration to indicate the correct BUSD token

EnderCrypto · 2020-03-26T12:54:53Z

plugins/dex/order/fee.go

+
+					if foundFirstPair {
+						var intermediateTmp int64
+						if intermediateAmount.IsInt64() {


This conversion may be omitted since big.Int can be passed to below calculations

Maybe, i don't get your point. This sentence "amount = utils.CalBigNotional(market.LastTradePrice, intermediateTmp)" needs an int64.

EnderCrypto · 2020-03-26T13:13:04Z

plugins/dex/order/keeper_test.go

+	require.Nil(t, err)
+}
+
+func TestKeeper_CanDelistTradingPair_SupportBUSD(t *testing.T) {


Need test a negative case: If only ABC_XYZ and XYZ_BUSD are listed, then delisting XYZ_BUSD before delisting ABC_XYZ should throw error
I think keeper.CanDelistTradingPair need to be modified to pass this case.

The latest design is that "for listing ABC_XYZ there should be ABC/BNB and XYZ/BNB pairs, BUSD pairs do not help on this". To make this clear, new test cases are added for both listing and delisting.

rickyyangz · 2020-03-30T03:24:29Z

app/config/config.go

@@ -358,7 +362,9 @@ type UpgradeConfig struct {
 	// Hubble Upgrade
 	BEP12Height int64 `mapstructure:"BEP12Height"`
 	// Archimedes Upgrade
-	BEP3Height int64 `mapstructure:"BEP3Height"`
+	BEP3Height     int64 `mapstructure:"BEP3Height"`
+	BEP_BUSDHeight int64 `mapstructure:"BEP_BUSDHeight"`


please follow the naming convension

sure, will rename it after we have the BEP finalized. Or, can we have the BEP NO. now?

rickyyangz · 2020-03-30T03:26:59Z

app/config/config.go

@@ -392,6 +399,16 @@ func defaultQueryConfig() *QueryConfig {
 	}
 }

+type GovConfig struct {


I think it's not a gov related config. can you create a DexConfig instead and remember the update the config template above.

rickyyangz · 2020-03-30T03:29:56Z

app/config/config.go

@@ -392,6 +399,16 @@ func defaultQueryConfig() *QueryConfig {
 	}
 }

+type GovConfig struct {
+	SupportedListAgainstSymbols []string `mapstructure:"SupportedListAgainstSymbols"`


I suggest we just simplify the config to BusdSymbol. I don't think we would support multiple assets, or if we want to support multiple assets, we need to have a more reasonable solution instead of the config way.

rickyyangz · 2020-03-30T03:33:47Z

besides the fee change, the most important part is the listing rule. I do not see any related changes.

forcodedancing · 2020-03-30T03:39:06Z

besides the fee change, the most important part is the listing rule. I do not see any related changes.

This file: plugins/dex/order/keeper.go
Func: CanListTradingPair

plugins/dex/order/keeper.go

rickyyangz · 2020-04-06T17:19:19Z

plugins/dex/order/fee.go

+		} else {
+			// for BUSD pairs, it is possible that there is no trading pair between BNB and inAsset, e.g., BUSD -> XYZ
+			if sdk.IsUpgrade(upgrade.BEP_BUSD) {
+				for _, symbol := range m.GovSupportedSymbols {


no need to check all the symbols. Each TradeTransfer has both in and out asset symbol and quantity, you just need to use the opposite quantity to calc the native fee. we can talk offline

rickyyangz · 2020-04-06T17:19:40Z

plugins/dex/order/fee.go

+		} else {
+			// for BUSD pairs, it is possible that there is no trading pair between BNB and inAsset, e.g., BUSD -> XYZ
+			if sdk.IsUpgrade(upgrade.BEP_BUSD) {
+				for _, symbol := range m.GovSupportedSymbols {


rickyyangz · 2020-04-08T12:35:41Z

plugins/dex/order/fee.go

@@ -143,15 +147,44 @@ func (m *FeeManager) calcNativeFee(inSymbol string, inQty int64, engines map[str
 		if engine, ok := engines[utils.Assets2TradingPair(inSymbol, types.NativeTokenSymbol)]; ok {
 			// XYZ_BNB
 			nativeNotional = utils.CalBigNotional(engine.LastTradePrice, inQty)
-		} else {
+		} else if engine, ok := engines[utils.Assets2TradingPair(types.NativeTokenSymbol, inSymbol)]; ok {


could you help extract such code into a method, like getEngine, I see we have so many places using that.

rickyyangz · 2020-04-08T12:42:11Z

plugins/dex/order/keeper.go

+		if sdk.IsUpgrade(upgrade.BEP_BUSD) {
+			if baseAsset == kp.BUSDSymbol || quoteAsset == kp.BUSDSymbol {
+				if kp.PairMapper.Exists(ctx, types.NativeTokenSymbol, kp.BUSDSymbol) ||
+					kp.PairMapper.Exists(ctx, kp.BUSDSymbol, types.NativeTokenSymbol) {


could you help refactor these codes

if kp.PairMapper.Exists(ctx, symbolA, symbolB) || kp.PairMapper.Exists(ctx, symbolB, symbolA)

into a method like kp.PairExists(ctx, symbolA, symbolB). we have many code pieces like that

rickyyangz · 2020-04-19T14:31:19Z

plugins/dex/order/keeper.go

@@ -66,7 +67,7 @@ func CreateMatchEng(pairSymbol string, basePrice, lotSize int64) *me.MatchEng {

 // NewKeeper - Returns the Keeper
 func NewKeeper(key sdk.StoreKey, am auth.AccountKeeper, tradingPairMapper store.TradingPairMapper, codespace sdk.CodespaceType,
-	concurrency uint, cdc *wire.Codec, collectOrderInfoForPublish bool) *Keeper {
+	concurrency uint, busdSymbol string, cdc *wire.Codec, collectOrderInfoForPublish bool) *Keeper {


I suggest not adding a parameter to the keeper. Actually a keeper can work without a busdsymbol.
so adding a method like SetBusdSymbol to set it may be better.

busd fee refactor

rickyyangz · 2020-04-27T12:18:46Z

app/config/config.go

@@ -360,7 +368,9 @@ type UpgradeConfig struct {
 	// Hubble Upgrade
 	BEP12Height int64 `mapstructure:"BEP12Height"`
 	// Archimedes Upgrade
-	BEP3Height int64 `mapstructure:"BEP3Height"`
+	BEP3Height  int64 `mapstructure:"BEP3Height"`
+	BEP70Height int64 `mapstructure:"BEP70Height"`


could you put the config after bep67, we group these configs by upgrade name

rickyyangz · 2020-04-27T12:19:08Z

app/config/config.go

@@ -379,6 +389,7 @@ func defaultUpgradeConfig() *UpgradeConfig {
 		BEP19Height:                1,
 		BEP12Height:                1,
 		BEP3Height:                 1,
+		BEP70Height:                1,


rickyyangz · 2020-04-27T12:19:12Z

common/upgrade/upgrade.go

@@ -17,6 +17,9 @@ const (
 	BEP12 = "BEP12" // https://github.com/binance-chain/BEPs/pull/17
 	// Archimedes Upgrade
 	BEP3 = "BEP3" // https://github.com/binance-chain/BEPs/pull/30
+	// BUSD Pair Upgrade
+	BEP70 = "BEP70" // supporting listing and trading BUSD pairs


George added 2 commits March 17, 2020 16:52

add busd pair support

57b3caa

add busd pair support, following changes not implmented yet 1) a method to define the standard BUSD token 2) BEP number change

support busd pairs for listing and trading

fc1ffd4

add configuration to indicate the correct BUSD token

forcodedancing requested a review from rickyyangz March 18, 2020 03:53

forcodedancing changed the title ~~Support busd pair listing and trading~~ [WIP] Support busd pair listing and trading Mar 18, 2020

forcodedancing requested a review from EnderCrypto March 23, 2020 06:57

rickyyangz requested review from ackratos and yutianwu March 23, 2020 06:57

forcodedancing changed the title ~~[WIP] Support busd pair listing and trading~~ Support busd pair listing and trading Mar 26, 2020

EnderCrypto reviewed Mar 26, 2020

View reviewed changes

add more test cases

ddbeca3

forcodedancing requested a review from EnderCrypto March 27, 2020 03:36

rickyyangz reviewed Mar 30, 2020

View reviewed changes

EnderCrypto approved these changes Apr 2, 2020

View reviewed changes

rickyyangz reviewed Apr 6, 2020

View reviewed changes

plugins/dex/order/keeper.go Show resolved Hide resolved

rickyyangz reviewed Apr 6, 2020

View reviewed changes

change SupportedListSymbols[] to single BUSDSymbol

8ed1e06

rickyyangz reviewed Apr 8, 2020

View reviewed changes

George added 2 commits April 9, 2020 11:32

extract methods; update BEP number

0e46d38

refactor

018598e

rickyyangz reviewed Apr 19, 2020

View reviewed changes

George and others added 4 commits April 24, 2020 10:08

refactor keeper

65ae54a

busd fee refactor

53e2c6e

Merge pull request #722 from binance-chain/busd

7b9e938

busd fee refactor

add check

ba142c2

Merge branch 'develop' into support_busd

c1fa20a

rickyyangz reviewed Apr 27, 2020

View reviewed changes

George added 2 commits April 27, 2020 20:52

order and group upgrade config

72f09d9

order and group upgrade config

5d9010b

rickyyangz approved these changes Apr 27, 2020

View reviewed changes

forcodedancing merged commit 3e4d4c0 into develop Apr 27, 2020

EnderCrypto changed the title ~~Support busd pair listing and trading~~ BEP70 - Support busd pair listing and trading Jun 2, 2020

EnderCrypto mentioned this pull request Jun 2, 2020

0.7 Upgrade #744

Merged

unclezoro deleted the support_busd branch May 10, 2022 06:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BEP70 - Support busd pair listing and trading #710

BEP70 - Support busd pair listing and trading #710

forcodedancing commented Mar 18, 2020 •

edited

Loading

EnderCrypto Mar 26, 2020

forcodedancing Mar 27, 2020 •

edited

Loading

EnderCrypto Mar 26, 2020

forcodedancing Mar 27, 2020

rickyyangz Mar 30, 2020

forcodedancing Mar 30, 2020

rickyyangz Mar 30, 2020

rickyyangz Mar 30, 2020

rickyyangz commented Mar 30, 2020

forcodedancing commented Mar 30, 2020

rickyyangz Apr 6, 2020

rickyyangz Apr 6, 2020

rickyyangz Apr 8, 2020

forcodedancing Apr 9, 2020

rickyyangz Apr 8, 2020

forcodedancing Apr 9, 2020

rickyyangz Apr 19, 2020

forcodedancing Apr 24, 2020

rickyyangz Apr 27, 2020

rickyyangz Apr 27, 2020

rickyyangz Apr 27, 2020

forcodedancing Apr 27, 2020

BEP70 - Support busd pair listing and trading #710

BEP70 - Support busd pair listing and trading #710

Conversation

forcodedancing commented Mar 18, 2020 • edited Loading

Description

Rationale

Example

Changes

Preflight checks

Already reviewed by

Related issues

Choose a reason for hiding this comment

forcodedancing Mar 27, 2020 • edited Loading

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

rickyyangz commented Mar 30, 2020

forcodedancing commented Mar 30, 2020

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

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

forcodedancing commented Mar 18, 2020 •

edited

Loading

forcodedancing Mar 27, 2020 •

edited

Loading