feat(x/ecocredit): Msg Server implementation for buy/sell functionality #613

clevinson · 2021-11-02T02:09:57Z

Description

This pull request implements the following:

proto type definitions for SellOrder, BuyOrder, and AskDenom
proto event definitions for EventSell, EventUpdateSellOrder, EventBuyOrderCreated, EventBuyOrderFilled, and EventAllowAskDenom
message service methods for Sell, UpdateSellOrders, Buy, and AllowAskDenom
ValidateBasic for MsgSell, MsgUpdateSellOrders, MsgBuy, and MsgAllowAskDenom

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...

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 · 2021-11-04T05:35:02Z

Codecov Report

Merging #613 (6cc74fd) into master (8994951) will decrease coverage by 1.14%.
The diff coverage is 17.15%.

@@            Coverage Diff             @@
##           master     #613      +/-   ##
==========================================
- Coverage   75.23%   74.09%   -1.15%     
==========================================
  Files         104      104              
  Lines       14628    14899     +271     
==========================================
+ Hits        11006    11039      +33     
- Misses       2902     3131     +229     
- Partials      720      729       +9

Flag	Coverage Δ
experimental-codecov	`74.09% <17.15%> (-1.15%)`	⬇️
stable-codecov	`65.88% <17.15%> (-1.14%)`	⬇️

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

Co-authored-by: Cory <cjlevinson@gmail.com>

x/ecocredit/msgs.go

technicallyty · 2021-11-08T18:57:56Z

x/ecocredit/msgs.go

+
+		m.Orders[i].AskPrice.Validate()
+
+		if !m.Orders[i].AskPrice.Amount.IsPositive() {


coins.Validate checks for negatives. just wondering, are we doing this to avoid 0 amt bids?

Correct. The intention here was to prevent sell orders from being created with a 0 amount ask price and therefore buy orders with a 0 amount bid price (bid price must be ask price at minimum). I think this behavior is desired but let me know if you think otherwise.

@dtkong thoughts here? @ryanchristo and I had thought that we didn't want to allow buy/sell orders w/ a 0 ask price. If there's a valid use case for it we can change this.

x/ecocredit/msgs.go

technicallyty · 2021-11-08T19:00:20Z

x/ecocredit/msgs.go

+
+		m.Updates[i].NewAskPrice.Validate()
+
+		if !m.Updates[i].NewAskPrice.Amount.IsPositive() {


not sure if anyone would ever do this, but would we want to allow credits to sell for 0 here?

My initial thought was that we would not want to support 0 amount ask price but maybe we should?

x/ecocredit/server/msg_server.go

technicallyty · 2021-11-08T20:48:48Z

x/ecocredit/server/msg_server.go

@@ -577,3 +516,346 @@ func (s serverImpl) isCreatorAllowListed(ctx types.Context, allowlist []string,
 	}
 	return false
 }
+
+// Sell creates new sell orders for credits
+func (s serverImpl) Sell(goCtx context.Context, req *ecocredit.MsgSell) (*ecocredit.MsgSellResponse, error) {


im curious what will happen if a user creates multiple sell orders around the same credits. i think there is a concern with flooding the chain with a bunch of seemingly valid sell orders that are actually all but one invalid.

This is a valid concern. We discussed an escrow model as a potential alternative but at the moment we are only checking the credit balance at the time the sell order is created.

Well maybe we should revisit the pros and cons and escrow. Can you guys think a little more about the pros and cons and the threat model here?

x/ecocredit/server/testsuite/suite.go

x/ecocredit/msgs.go

aaronc · 2021-11-09T15:40:46Z

proto/regen/ecocredit/v1alpha1/events.proto

+}
+
+// EventBuy is an event emitted when a buy order is created.
+message EventBuy {


Should we have separate events when buy orders are created and filled?

How would this work? We're not storing buy orders in state, so the created event would emit if partial or filled and the filled event would emit only if the buy order was filled?

i think this would work for orders with filters, where buy orders can be made for a credit with X characteristics, but no direct sell order ID listed.. but sounds like it wont work for orders with direct SellOrderId's as with those you either fill it or you don't.

Is there any spec on how the matching engine will work in this scheme for floating buy/sell orders?

In the current implementation, it is possible to do a partial fill with a buy order that specifies a sell order id but I'm not sure what we should do with the buy order if the order was only partially filled.

At the moment, if partial fill is enabled, the buyer purchases the available credits from the given sell order and that's it (the buy order does not persist to state and there is no fulfilling the remainder of the order at a later point in time); if the buyer specifies more credits than what is available in the sell order, the buyer ends up purchasing only what is available.

When we implement the filtered option (#623), the same question applies. If partial fill is enabled and an order is only partially filled given the filter criteria, how will the buy order be fulfilled at a later point in time?

how will the buy order be fulfilled at a later point in time?

Just like in regular exchanges, if the buy order is partially filled it will stay open and way for new or updated sell orders. Basically in every processing batch an attempt will be made to match it with sell orders.

We should probably have expiration times for both sell and buy orders btw.

At this point we havn't implemented any of the ORM tables for storing buy orders in state, but sounds like we would need this in order to actually process partial-filling correctly (where we keep a buy order open). If we don't implement that in this PR we could also just add it in a follow-up. When persisting the partially filled buy order to state, how would we store the selection ? Would it still reference the original sell_order_id ? Afaiu, the sell order would be exhausted and then closed (removed from state), so there wouldn't be any way for a seller to top up their supply on an existing sell order. They would rather create a new sell order.

If we allow for partial fills I'm not really sure how we can keep those buy orders open seeing that they are against sell order which would have already closed..

If we address in a follow-up, it might make sense to include with #623 assuming that we would only need to store a buy order if it was using the filter option (not a direct buy order).

I added a second event for filling buy orders to distinguish from creating buy orders. At the moment the second event seems unnecessary given that only direct buy orders are implemented in this pull request but it makes sense when we implement the filter option. I think this goes in line with #623 as well.

I also added a couple commits to update the remaining quantity if the buy order does not purchase all credits within the sell order and to delete the sell order if there are no remaining credits.

Maybe we should break out a separate issue for adding and handling expiration times..?

Co-authored-by: MD Aleem <72057206+aleem1314@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

clevinson · 2021-11-09T23:08:10Z

x/ecocredit/server/msg_server.go

+	if req.RootAddress != authtypes.NewModuleAddress(govtypes.ModuleName).String() {
+		return nil, sdkerrors.ErrUnauthorized.Wrap("root address must be governance module address")
+	}


@aaronc @blushi Can one of you confirm that this is how the root address stuff is supposed to work?

ryanchristo · 2021-11-10T19:46:24Z

Ready for another round of reviews.

Aside from remaining items that already have issues, here are the remaining items that I can see for this pull request that could either be addressed here or in follow-up pull requests:

support 0 amount sell orders? @dtkong
~~create table to store buy orders~~
update buy order after processing --> Implement filtered buy orders #623
add expiration times to buy and sell orders --> Implement buy/sell order expiration #636

Let me know if you think any of these should be addressed here, otherwise I think it might be nice to include them in follow up issues.

technicallyty

regarding #613 (comment), i think it would make sense to include the buy order table here, just to set it up for the processing PR

x/ecocredit/server/msg_server.go

Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

technicallyty

great work! 💯 💯 💯 💯 💯 💯 💯

clevinson

Few small nits, otherwise giving this an ACK 🎉

@ryanchristo can you give an approval for me, since I authored the original PR ?

clevinson · 2021-11-13T01:24:06Z

proto/regen/ecocredit/v1alpha1/events.proto

+  // total_price is the total price for the purchased credits.
+  cosmos.base.v1beta1.Coin total_price = 5;


Suggested change

// total_price is the total price for the purchased credits.

cosmos.base.v1beta1.Coin total_price = 5;

// price is the total price for the purchased credits.

cosmos.base.v1beta1.Coin price = 5;

I like price instead of total_price, since its not like prices are summed (creating a "total")

The price here is the amount of coin sent (not the price per unit but the calculated amount of coin needed to fill the order). I thought it might help to use total to distinguish from bid price and ask price (per unit) more clearly.

clevinson · 2021-11-13T01:26:31Z

proto/regen/ecocredit/v1alpha1/events.proto

+  // sell_order_id is the sell order ID against which the buyer is trying to buy.
+  uint64 sell_order_id = 2;


This is fine for now, but we'll eventually want to upgrade this to be either a sell_order_id or some selection criteria, right? I think we can just do so once we add buy orders based on a selection criteria.

No todo in this PR, I think we should just keep in mind to update the event when we add the rest of the buy order functionality.

Correct. This will need to be updated when we introduce filtered buy orders. #623

ryanchristo

@ryanchristo can you give an approval for me, since I authored the original PR ?

ryanchristo and others added 2 commits November 1, 2021 19:06

first edits for msg server

31a7ce7

begin msg server implementation for buy / sell functionality

ef5d5e9

clevinson marked this pull request as draft November 2, 2021 02:10

ryanchristo added 8 commits November 3, 2021 13:59

buy order sequence

5a2fd0a

add buy/sell events

1845b53

update sell order

c5e56f4

Merge branch 'master' into clevinson/buy-sell-msg-server

d6563ef

buy order processing

12353d8

go mod tidy

c1a22bd

cleanup and msgs

f874171

go mod tidy

76677d4

ryanchristo added 3 commits November 4, 2021 12:55

validate basic

1078a83

allow ask denom

1f4678e

fix and cleanup

60aae2e

clevinson mentioned this pull request Nov 5, 2021

Handle auto-retire in ecocredit buy/sell functionality #621

Closed

2 tasks

ryanchristo and others added 3 commits November 5, 2021 15:04

process buy order

5a4185e

Co-authored-by: Cory <cjlevinson@gmail.com>

verify root address

c5cc992

Merge branch 'master' into clevinson/buy-sell-msg-server

d034f04

ryanchristo marked this pull request as ready for review November 6, 2021 00:27

ryanchristo assigned technicallyty and aleem1314 Nov 8, 2021

technicallyty suggested changes Nov 8, 2021

View reviewed changes

aleem1314 reviewed Nov 9, 2021

View reviewed changes

x/ecocredit/msgs.go Outdated Show resolved Hide resolved

aaronc reviewed Nov 9, 2021

View reviewed changes

ryanchristo and others added 5 commits November 9, 2021 08:25

Apply suggestions from code review

08bfa04

Co-authored-by: MD Aleem <72057206+aleem1314@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

improve validate

36e7972

Apply suggestions from code review

206bbb5

Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

address review comments

2326117

address review comments

3813340

clevinson commented Nov 9, 2021

View reviewed changes

ryanchristo added 3 commits November 9, 2021 20:32

add filled event

21cd26e

update sell order

40b2bf1

complete sell order

4216f9e

technicallyty reviewed Nov 11, 2021

View reviewed changes

ryanchristo and others added 3 commits November 11, 2021 15:06

Apply suggestions from code review

09bce2d

Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>

address review comment

9c62008

buy order table

8b7cd67

ryanchristo requested a review from technicallyty November 11, 2021 23:30

technicallyty approved these changes Nov 12, 2021

View reviewed changes

fix primary key

5dee66a

clevinson commented Nov 15, 2021

View reviewed changes

Merge branch 'master' into clevinson/buy-sell-msg-server

6cc74fd

ryanchristo mentioned this pull request Nov 15, 2021

Implement filtered buy orders #623

Open

4 tasks

ryanchristo approved these changes Nov 15, 2021

View reviewed changes

ryanchristo merged commit 081ae07 into master Nov 15, 2021

ryanchristo deleted the clevinson/buy-sell-msg-server branch November 15, 2021 18:43

ryanchristo mentioned this pull request Nov 15, 2021

feat: marketplace Query API #632

Merged

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(x/ecocredit): Msg Server implementation for buy/sell functionality #613

feat(x/ecocredit): Msg Server implementation for buy/sell functionality #613

clevinson commented Nov 2, 2021 •

edited by zube bot

codecov bot commented Nov 4, 2021 •

edited

technicallyty Nov 8, 2021

ryanchristo Nov 9, 2021

clevinson Nov 10, 2021 •

edited

technicallyty Nov 8, 2021

ryanchristo Nov 9, 2021

technicallyty Nov 8, 2021

ryanchristo Nov 9, 2021

aaronc Nov 12, 2021

aaronc Nov 9, 2021

ryanchristo Nov 9, 2021

technicallyty Nov 9, 2021

ryanchristo Nov 9, 2021

aaronc Nov 9, 2021

clevinson Nov 10, 2021

ryanchristo Nov 10, 2021

clevinson Nov 9, 2021

ryanchristo commented Nov 10, 2021 •

edited

technicallyty left a comment

technicallyty left a comment

clevinson left a comment •

edited

clevinson Nov 13, 2021

ryanchristo Nov 15, 2021

clevinson Nov 13, 2021 •

edited

ryanchristo Nov 15, 2021

ryanchristo left a comment


		m.Orders[i].AskPrice.Validate()

		if !m.Orders[i].AskPrice.Amount.IsPositive() {


		m.Updates[i].NewAskPrice.Validate()

		if !m.Updates[i].NewAskPrice.Amount.IsPositive() {

		// total_price is the total price for the purchased credits.
		cosmos.base.v1beta1.Coin total_price = 5;

		// sell_order_id is the sell order ID against which the buyer is trying to buy.
		uint64 sell_order_id = 2;

feat(x/ecocredit): Msg Server implementation for buy/sell functionality #613

feat(x/ecocredit): Msg Server implementation for buy/sell functionality #613

Conversation

clevinson commented Nov 2, 2021 • edited by zube bot

Description

Author Checklist

Reviewers Checklist

codecov bot commented Nov 4, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clevinson Nov 10, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo commented Nov 10, 2021 • edited

technicallyty left a comment

Choose a reason for hiding this comment

technicallyty left a comment

Choose a reason for hiding this comment

clevinson left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clevinson Nov 13, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo left a comment

Choose a reason for hiding this comment

clevinson commented Nov 2, 2021 •

edited by zube bot

codecov bot commented Nov 4, 2021 •

edited

clevinson Nov 10, 2021 •

edited

ryanchristo commented Nov 10, 2021 •

edited

clevinson left a comment •

edited

clevinson Nov 13, 2021 •

edited