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

List on main market #831

Closed
wants to merge 8 commits into from
Closed

List on main market #831

wants to merge 8 commits into from

Conversation

fletcher142
Copy link
Contributor

@fletcher142 fletcher142 commented Mar 22, 2021

Description

The trading pairs will be directly listed once the listing proposals get passed after voting

Rationale

Simplifying the listing process to facilitate the listing on Dex

Example

Changes

Notable changes:

  • Add hooks OnProposalPassed
  • Add more validation when submitting the listing proposal

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

None

return errors.New("quote token does not exist")
}
}
if pair, err := hooks.orderKeeper.PairMapper.GetTradingPair(ctx, listParams.BaseAssetSymbol, types.NativeTokenSymbol); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we move the two 'if' logics into CanListTradingPair, I see both list growth and main need this logic

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 guess no, it is quite different. This validation depends on the market the pair going to be listed. If we move it to CanListTradingPair, then we need refactor this function and pass 'market' or something to this function. Moreover, this 'One market' validation above now is not completed, it needs to verify the quote asset as well , I will later add this.

return errors.New("init price should larger than zero")
}

if types.IsMiniTokenSymbol(baseAssetSymbol) {
Copy link
Contributor

Choose a reason for hiding this comment

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

please do not copy and paste code. it's hard to maintain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

optimized


if listParams.QuoteAssetSymbol == "" {
return errors.New("quote asset symbol should not be empty")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need such check when the proposal is passed? These conditions are checked when the proposal submitted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

return nil
}

if proposal.GetProposalType() != gov.ProposalTypeListTradingPair {
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need this check if this Listhook is invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Theoretically we don't, as the invoker won't invoke this function when proposal type is not ProposalTypeListTradingPair. But is that better to add this check as the function itself to prevent the unexpected invocation?

proposal.GetProposalType(), gov.ProposalTypeListTradingPair)
}

if proposal.GetStatus() != gov.StatusPassed {
Copy link
Contributor

Choose a reason for hiding this comment

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

same as the above, we may not need this check

baseAssetSymbol := strings.ToUpper(listParams.BaseAssetSymbol)
quoteAssetSymbol := strings.ToUpper(listParams.QuoteAssetSymbol)

if err := checkListingPairOnMainMarket(hooks, ctx, strings.ToUpper(listParams.BaseAssetSymbol), strings.ToUpper(listParams.QuoteAssetSymbol)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

use the two variables defined above instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

app/app.go Outdated
@@ -402,6 +402,7 @@ func (app *BinanceChain) initDex() {
app.publicationConfig.ShouldPublishAny())
app.DexKeeper.SubscribeParamChange(app.ParamHub)
app.DexKeeper.SetBUSDSymbol(app.dexConfig.BUSDSymbol)
types.BUSDSymbol = app.dexConfig.BUSDSymbol
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -28,6 +28,8 @@ const (
NativeTokenTotalSupply = 2e16
)

var BUSDSymbol string
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@@ -25,7 +29,49 @@ func NewListHooks(orderKeeper *order.DexKeeper, tokenMapper tokens.Mapper) ListH
}
}

var _ gov.GovHooks = ListHooks{}
var _ gov.ExtGovHooks = ListHooks{}
Copy link
Contributor

Choose a reason for hiding this comment

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

can we just use the GovHooks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we use the GovHooks, then we need implement the OnProposalPassed method for all the places where implement this hook.

@unclezoro unclezoro closed this Apr 24, 2022
@unclezoro unclezoro deleted the list_main_market branch May 10, 2022 06:15
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.

3 participants