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

Follow-ups for buy/sell proto definitions #605

Closed
3 of 10 tasks
Tracked by #818
clevinson opened this issue Oct 28, 2021 · 4 comments
Closed
3 of 10 tasks
Tracked by #818

Follow-ups for buy/sell proto definitions #605

clevinson opened this issue Oct 28, 2021 · 4 comments
Labels
Scope: x/ecocredit Issues that update the x/ecocredit module

Comments

@clevinson
Copy link
Member

clevinson commented Oct 28, 2021

Summary

Few followups from #560 that were left as open questions.

Some of these points may be left as is, but we should still make sure to have the conversations here or in WG meetings to align on them before releasing full marketplace functionality into the wild.

  • Change UpdateBuyOrders api to be more granular (see comment) Add granular buy/sell update messages #892
  • Add support for explicitly cancelling a sell order (see comment)
  • update ask_price field to repeated Coin, allowing for a sell order to accept multiple denoms. This may not be a feature we want to enable though, as it creates possibility for strange arbitrage opportunities when exchange rates of "ask denoms" diverge (see comment)
  • Rename Sell() & BuyDirect() rpc methods to CreateSellOrders() and (CreateBuyOrders() or DirectBuy())(see comment & here)

Not explicitly brought up in the review, but two additional pieces on my mind:

  • re-evaluate params for MsgAllowAskDenom (maybe metadata isn't necessary here if it can be stored in x/bank's DenomMetadata?)
  • distinguishing btw BuyDirect and CreateBuyOrder functionality
    • currently, BuyDirect creates a "buy order" that may have some pending state. I'm still not 100% sure of what use case we have for this, and rather think of CreateBuyOrder as a separate API call that would actually create a pending order in state, whereas BuyDirect would always be against an existing open sell order, and be immediately executable

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@clevinson clevinson added the Scope: x/ecocredit Issues that update the x/ecocredit module label Oct 28, 2021
@aaronc
Copy link
Member

aaronc commented Oct 29, 2021

Direct buy orders cannot be executed immediately when we have more complex order book matching as they would compete with other open orders and thus need to be subject to the same batching. Thus the logic for introducing order IDs in this iteration. It is a question, however, if we want to persist orders in state that are already complete - likely we don't, but they could still be referenced from events or historical state.

@clevinson
Copy link
Member Author

clevinson commented Nov 2, 2021

OK makes sense. @ryanchristo and I started looking into this today...

For now, should the fulfilling of MsgBuyDirect calls be done in batch (e.g. in EndBlocker()?), or should we be directly fulfilling orders as part of the msg server's BuyDirect() call?

@aaronc
Copy link
Member

aaronc commented Nov 2, 2021

Up to you. Maybe simplest to do it synchronously in the tx

@clevinson
Copy link
Member Author

From ledger product call today:

  • Add support for explicitly cancelling a sell order (will add this in existing msg server implementation work from @clevinson and @ryanchristo)
  • Let's not persist buy orders to state, but store a ORM seq for buy orders so we can auto increment and dispatch events with buy_order_id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: x/ecocredit Issues that update the x/ecocredit module
Projects
None yet
Development

No branches or pull requests

3 participants