feat: ecocredit direct buy/sell proto definitions

[aaronc]

ref #505

This PR introduces a very simple store model for buying and selling credits. A seller can create a sell order for credits they own and a buyer can buy credits against any open sell order. The only buy orders supported are direct in that they reference a specific sell order ID and always settle at the ask price.

A future PR may introduce an order book model where buy orders can advertise bid prices and target credit batches generically filtered by credit class, geography, vintage dates and tags (#395).

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[codecov]

Codecov Report

Merging #560 (bee78fb) into master (53b6f00) will not change coverage.
The diff coverage is n/a.

❗ Current head bee78fb differs from pull request most recent head 324f66c. Consider uploading reports for the commit 324f66c to get more accurate results

@@           Coverage Diff           @@
##           master     #560   +/-   ##
=======================================
  Coverage   75.73%   75.73%           
=======================================
  Files         104      104           
  Lines       14236    14236           
=======================================
  Hits        10782    10782           
  Misses       2808     2808           
  Partials      646      646           

Flag	Coverage Δ
experimental-codecov	75.73% <ø> (ø)
stable-codecov	67.27% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[clevinson]

Could this just be one Coin field ask_coin ?

[aaronc]

Sure 👍

[aaronc]

I'm assuming that the ask_coin is the price per one unit right?

[clevinson]

yes that's what my assumption would be. Maybe better to name it ask_coin_price to make it more explicit?

[robert-zaremba]

let's add a doc string to specify the details.

[clevinson]

What does it mean for a buyer to provide an auto_retire flag? Isn't auto-retire something that would be set / determined by the seller only?

[aaronc]

I'm thinking we would have both default to auto-retire. Even if the seller checks dont_auto_retire, they'll still retire unless the buyer also checks dont_auto_retire. It biases the whole design towards the intended use (i.e. retirement) which I've gotten the sense is desirable, without making trading impossible

[clevinson]

@dtkong thoughts on this?

[aaronc]

Made some updates to this, adding the ability to update buy orders and allow ask denoms through governance. I also put comments everywhere.

[blushi]

API looks good overall, just one concern regarding sell orders update

[blushi]

What if it's not set? How would you know whether it should be updated to false or not? Or we need to assume that the user should just set this field as the current value if it doesn't need to be updated, but this can be error-prone.

[aaronc]

This API assumes everything needs to be explicitly set in the update message. We can have a more granular update API of course if that's desired

[blushi]

Ok could you add this assumption to the docs then?

[blushi]

Another thing is that we've started using a granular update API within the ecocredits module already for updating credit classes so I'm afraid that could result in inconsistencies across the ecocredits msg service and confusions for users.

[clevinson]

This is a fair point. @aaronc what do you think of having granular sub messages to be more aligned w/ the granular "update credit class" msg's already in place on ecocredit module?

[anilcse]

Can we just call it auto_retire and use it otherwise. dont prefix is bit odd

[aaronc]

My understanding is that we generally want to default to auto-retirement. Maybe disable_auto_retirement is clearer?

[anilcse]

Yes, that sounds better

[clevinson]

We discussed this today in our Regen Ledger Product call. One thing that was called out that we'd like is an explicit sign-off from @dtkong that this is satisfying the product requirements as you understand them - and then from there we can move forward with implementation.

Separately, I had the thought that maybe we should add functionality to cancel a sell order. From my understanding, its feasible in this model that a user may list multiple sell orders for a fixed set of ecocredits (with different asking denoms). Example is:

• I have 100 ecocredits and want to sell them
• I list 2 sell orders one priced in REGEN, another priced in ATOM, each order has a limit of 100 ecocredits
• A buyer may come and exhaust my credits, buying 50 credits in REGEN and 50 in ATOM
• the seller should have the ability to explicitly cancel the sell orders (each which have 50 credits for sale remaining in their Sell Order state)

Another use case is even more simple, where a seller of credits may choose that they want to cancel a listing bc they have changed their mind and want to hold all their ecocredits.

Technically this could be done through an MsgUpdateSellOrders call, setting new_quantity to 0. But I think its more UX friendly to just have an explicit MsgCancelSellOrders operation as part of the API.

[dtkong]

Looks good to me. Chatted with @clevinson about some of my questions. I think this is a good base to move forward as it provides a flexible base towards a true orderbook and is also compatible with the basket model (it's also not mutually exclusive with an AMM).

[clevinson]

Another thing that came up in a conversation w/ Ryan later today...

It seems like we're going with a non-escrow model approach in this current architecture (e.g. SellOrders are tracked separately from a person's balance, and when a person opens a sell order they could still theoretically transfer those ecocredits to another user without invalidating the existence of the sell order).

A nice principle of this is that a user can list multiple sell orders against the same batch of ecocredits (each sell order with a different asking price denom).

However, it means that the existence of a sell order on its own is not enough to guarantee that it can be executed on.

Let's say I'm a holder of 100 ecocredits and I make a sell order for 50 ecocredits at 5 REGEN/credit price. I could transfer 80 of these ecocredits to Alice, and then if a user tries to buy 50 ecocredits against my sell order, it would error out since I don't have sufficient ecocredits in my balance.

Similarly, if we look at the application developer trying to build a marketplace of sell orders. It's not enough to query all sell orders of a given credit class or batch and expose them to potential buyers. If you want to only show sell orders that can be executed at their full quantity, you have to be checking against each sell order to verify that the seller has those credits in their balance - right?

Or is your intention already that querying would show you not the sell order quanitty, but the minimum of sell order quantity and tradable credits owned by the seller?

@aaronc is the idea in this model that SellOrder quantities are tracked in state separately from credit balances?

If the main reason that we chose a non-escrow model was to allow for multiple ask denoms, couldn't we have a single SellOrder have a repeated Coin field for ask_price that gives a list of valid denoms that payment is accepted in (with their corresponding price)?

[clevinson]

Stepping back, I'm actually not 100% sure if we want to enable listing for multiple prices until we have more dynamic pricing available (via price feeds or oracles).

Explicitly setting prices in multiple denoms opens up an arbitrage opportunity (at least when auto-retire is disabled), that I'm not sure we want to encourage. Then again, this is only a 1-sided marketplace for now so its not like the arbitrage opportunity is that straightforward to exploit... but this could be the case when we have hte ability for buyers to place asks directly without corresponding to a specific sell order.

[aaronc]

Good questions @clevinson. Another question that comes to mind is whether we want to allow buy orders to be partially filled in the case that the full quantity isn't available. Maybe filling partially is a flag that can be set in the buy order?

In general I'm skeptical of enabling more than one denom at all for the credit marketplace because it reduces effective liquidity and also like you say could create weird arbitrage opportunities.

[clevinson]

@aaronc if we don't want to support multi denom sell orders in parallel, then what would you think of pivoting to an escrow based model where the credits are actually moved temporarily into a derived module account for that user (or something similar).

This would also remove the need for any sort of "partial order fills" on the buy side - as it would be guaranteed that the quantity of credits visible in the "sell order" is actually the exact amount of credits available for sale.

[aaronc]

But that still doesn't help because I imagine we'll do batch processing once there are other types of buy orders. Even without batches there's no way of guaranteeing someone else doesn't buy part of the remaining supply first in an earlier tx.

[dtkong]

@aaronc if we don't want to support multi denom sell orders in parallel, then what would you think of pivoting to an escrow based model where the credits are actually moved temporarily into a derived module account for that user (or something similar).

This would also remove the need for any sort of "partial order fills" on the buy side - as it would be guaranteed that the quantity of credits visible in the "sell order" is actually the exact amount of credits available for sale.

I think an escrow model is reasonable simplifying by not supporting partial fills.

In other words, I think it could be reasonable to start off that the seller "makes" orders in whatever allowed currency + quantity denomination, and the buyer only can "take" the order in full or not at all. This then settles and clears immediately.

I'm not sure what all goes into building a full matching engine, but it may not be something we want to tackle at the moment?

But that still doesn't help because I imagine we'll do batch processing once there are other types of buy orders. Even without batches there's no way of guaranteeing someone else doesn't buy part of the remaining supply first in an earlier tx.

Batch processing via buy orders could be moved to be a buy order of a particular basket, no?
As for the buy of the remaining supply first in early tx, I think that we could sidestep that as well.

[ryanchristo]

Looks good to me. A few comments and questions.

I can see how the granular approach to updates might be desired (1) for consistency with the update messages for credit classes and (2) to ensure the disable_auto_retire bool is not accidentally set incorrectly. That being said, the current implementation is consistent with the creation of sell and buy orders and I think we can leave it as is and revisit later if need be.

An explicit MsgCancelSellOrders might be nice to have but is not necessary given that the quantity can be set to zero using MsgUpdateSellOrders.

[ryanchristo]

I think the consensus was to not support partial fills in which case the order would not be adjusted downwards but rather it would fail. Does this sound correct? cc @clevinson @dtkong

[aaronc]

I thought we agreed on partial fills being the expected behavior

[robert-zaremba]

I would suggest to improve all Msg methods comments by describing a valid or error condition.

[robert-zaremba]

let's improve all msg comments by describing a valid or error condition.

[aaronc]

I added comments for error conditions I'm aware of in the relevant places I can think of. But if you have specific suggestions for improving this that would be appeciated.

[robert-zaremba]

do we need a new_ prefix?

[aaronc]

not necessarily, but it doesn't hurt

[aaronc]

I've addressed review comments as best I could. Is this ready to merge now?

I haven't specified the query service but let's do that in a separate PR

[aaronc]

Let's keep this moving. This will auto-merge if we can get one more approval. We can always refine in future PRs

[clevinson]

🎉