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

refactor: refactor x/exchange module #217

Closed
wants to merge 23 commits into from
Closed

refactor: refactor x/exchange module #217

wants to merge 23 commits into from

Conversation

kingcre
Copy link
Contributor

@kingcre kingcre commented Sep 27, 2023

Description

  • Use sdk.Int for quantities
  • Simplify fee calculation and ceiling/truncation for order amounts
  • Move min_order_quantity, min_order_quote from x/amm Pool into x/exchange Market and also add max_order_quantity, max_order_quote
  • Move fees and quote coin dust into Market's fee_collector
  • Change MaxPrice to 10^24
  • Store price as tick indexes

- use sdk.Int for quantities
- change MaxPrice to 10^24
- store price as tick index for OrderBookOrderIndex
- apply ceiling/truncation per order, not per orderer
also add MaxOrderQty, MaxOrderQuote
proto/crescent/amm/v1beta1/params.proto Outdated Show resolved Hide resolved
app/upgrades/mainnet/v5/upgrade.go Outdated Show resolved Hide resolved
app/upgrades/mainnet/v5/upgrade.go Outdated Show resolved Hide resolved
proto/crescent/exchange/v1beta1/event.proto Show resolved Hide resolved
x/amm/keeper/source.go Outdated Show resolved Hide resolved
x/exchange/keeper/proposal_handler.go Outdated Show resolved Hide resolved
x/exchange/simulation/proposals.go Outdated Show resolved Hide resolved
x/exchange/types/proposal.go Outdated Show resolved Hide resolved
x/exchange/types/market.go Outdated Show resolved Hide resolved
x/exchange/types/keys.go Show resolved Hide resolved
Copy link
Member

@richard-bachman richard-bachman left a comment

Choose a reason for hiding this comment

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

Thank you for drafting this PR.
After reviewing the overall codebase, the followings are the highlights.

  • Add test suites and simulations for order source orders and their matching
    • This test for order source orders is very important since this is the part to interaction with other module for order like amm.
  • Can we check the matched price and matched amount in Batch_test?
    • Currently, only last price is checked.
  • Much more comments above the functions/variables to explain what they are (even though the explanation is short and brief.
  • Please remove all // TODO: ... part.
    • Others cannot know if it is not completed parts or to be completed or to be discussed further.
    • Please make the discussion in order to resolve all TODO: part ASAP.
  • What if the max value of life span of limit order?
  • Please complete spec.
    • Need to explain in detail about mid-block processing since this is the new concept.
    • Events, README need to be completed

Other detailed comments are added as inline comments.

@richard-bachman
Copy link
Member

@kingcre In L47 of exchange/keeper/batch.go, we have the following now.

if !matched {
	return
}

But, due to the code in L20 as follow,

if bestBuyPrice.LT(bestSellPrice) {
	return nil
}

we might not have the case of !matched. Is this for sanity check? Or, is there other case of not matching?

@kingcre
Copy link
Contributor Author

kingcre commented Oct 12, 2023

@kingcre In L47 of exchange/keeper/batch.go, we have the following now.

if !matched {
	return
}

But, due to the code in L20 as follow,

if bestBuyPrice.LT(bestSellPrice) {
	return nil
}

we might not have the case of !matched. Is this for sanity check? Or, is there other case of not matching?

Yes, as you said matched should always be true. It'd be better to transform it into a sanity check. Thanks.

x/exchange/types/keys.go Dismissed Show dismissed Hide dismissed
@kingcre kingcre marked this pull request as ready for review October 17, 2023 07:02
@kingcre
Copy link
Contributor Author

kingcre commented Oct 24, 2023

Closing. See #223

@kingcre kingcre closed this Oct 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants