feat(marketplace): escrow sell order credits #878

technicallyty · 2022-03-12T01:22:20Z

Description

adds Escrowed field to both BatchBalance and BatchSupply
updates Sell to escrow the credits in the order
updates Sell test to check balances/supplies are updated
updates Sell to emit EventSell

Closes: #821

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)

ryanchristo

The escrowed credits need to be returned to the seller upon expiration: https://github.com/regen-network/regen-ledger/blob/master/x/ecocredit/server/expiration.go#L23-L35.

Also, we should add a message for cancel sell order, allowing the seller to cancel their order at any time to reclaim their escrowed credits, which we can do in a separate pull request (#880).

Looking good otherwise. I'll give it a closer look after the expired sell orders update.

robert-zaremba

utACK. Left few suggestions.

x/ecocredit/server/marketplace/sell.go

robert-zaremba · 2022-03-15T13:09:53Z

x/ecocredit/server/marketplace/sell.go

+	// subtract the desired sell amount from tradable balance
+	newTradable, err := math.SafeSubBalance(tradable, sellQty)
+	if err != nil {
+		return fmt.Errorf("tradable balance: %s, sell order request: %s - %w", tradable.String(), sellQty.String(), err)


let's use .ErrInvalidRequest.Wrapf in all validation errors.

Also, if we use %v then we don't need to call String()

done, although i had to change the inner error to %s", err.Error(). the sdkerrors.Wrapf uses Sprintf under the hood which doesn't support %w :(

robert-zaremba · 2022-03-15T13:11:35Z

x/ecocredit/server/marketplace/sell.go

+		return err
+	}
+	supply.EscrowedAmount = supEscrow.String()
+	supply.TradableAmount = supTradable.String()


@aaronc shouldn't we have another type to serialize big numbers in the state? One type for response, another type for state.

I don't think so. State is directly exposed to clients and in the absence of any standard that clients can decode, string is the best choice. If someone defines a standard that's well specified this could change in the future

aaronc · 2022-03-15T13:48:23Z

proto/regen/ecocredit/v1/state.proto

@@ -195,6 +195,9 @@ message BatchBalance {

    // retired is the retired amount of credits
    string retired = 4;
+
+    // escrowed is the amount of credits locked up in escrow for the marketplace.
+    string escrowed = 5;


Is escrowed there right user facing name? Maybe in_sell_orders?

is there a specific concern you have regarding users and the word escrowed? i felt like it was a good fit - in_sell_orders seems a bit verbose

I could go either way. in_sell_orders is more specific and potentially more user friendly but provided the supporting documentation, escrowed seems ok to me as well.

How about you make a call @ryanchristo

Ok, escrowed it is. More aligned with tradable and retired.

aaronc · 2022-03-15T13:48:46Z

proto/regen/ecocredit/v1/state.proto

+
+    // escrowed_amount is the amount of credits in the batch that have been
+    // locked up in escrow for use in the marketplace.
+    string escrowed_amount = 5;


Do we really need to have a separate supply here? I guess maybe that's a useful invariant?

it seemed right to have it as the credits aren't necessarily tradable nor retired. if anything, it might be useful to be able to query a batch to see how many credits are in the market?

aaronc · 2022-03-15T13:53:23Z

x/ecocredit/server/marketplace/sell.go

+		return err
+	}
+	supply.EscrowedAmount = supEscrow.String()
+	supply.TradableAmount = supTradable.String()


I don't think so. State is directly exposed to clients and in the absence of any standard that clients can decode, string is the best choice. If someone defines a standard that's well specified this could change in the future

codecov · 2022-03-15T21:50:01Z

Codecov Report

Merging #878 (c7b4bdb) into master (f0faf4c) will decrease coverage by 0.13%.
The diff coverage is 48.93%.

❗ Current head c7b4bdb differs from pull request most recent head 3ab6cf8. Consider uploading reports for the commit 3ab6cf8 to get more accurate results

@@            Coverage Diff             @@
##           master     #878      +/-   ##
==========================================
- Coverage   73.99%   73.85%   -0.14%     
==========================================
  Files         172      173       +1     
  Lines       22142    22233      +91     
==========================================
+ Hits        16383    16421      +38     
- Misses       4640     4671      +31     
- Partials     1119     1141      +22

Flag	Coverage Δ
experimental-codecov	`73.82% <48.93%> (-0.14%)`	⬇️
stable-codecov	`67.02% <48.93%> (-0.12%)`	⬇️

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

ryanchristo

PruneOrders was not updated and I believe we want to include pruning of expired buy and sell orders in BeginBlocker not EndBlocker. We are still deleting sell orders without returning escrowed funds in the current implementation.

ryanchristo · 2022-03-15T21:45:44Z

proto/regen/ecocredit/v1/state.proto

@@ -195,6 +195,9 @@ message BatchBalance {

    // retired is the retired amount of credits
    string retired = 4;
+
+    // escrowed is the amount of credits locked up in escrow for the marketplace.
+    string escrowed = 5;


I could go either way. in_sell_orders is more specific and potentially more user friendly but provided the supporting documentation, escrowed seems ok to me as well.

ryanchristo · 2022-03-15T21:52:13Z

x/ecocredit/server/marketplace/prune.go

+// PruneSellOrders is an EndBlock function that moves escrowed credits back into their tradable balance and deletes orders
+// that have expired.
+func (k Keeper) PruneSellOrders(ctx context.Context) error {


Why is this separate from the existing PruneOrders that includes both buy and sell orders? And this should be for BeginBlocker not EndBlocker, correct? This is how PruneOrders is wired up at the moment. This is only being called in the added test and we are still deleting and not properly returning escrowed credits.

regen-ledger/x/ecocredit/server/expiration.go

Lines 23 to 35 in 70d0d40

for {

_, err := sellOrdersIter.LoadNext(&sellOrder)

if err != nil {

if orm.ErrIteratorDone.Is(err) {

break

}

return err

}

err = s.sellOrderTable.Delete(ctx, sellOrder.OrderId)

if err != nil {

return err

}

}

not properly returning escrowed credits.

regen-ledger/x/ecocredit/server/marketplace/prune.go

Line 29 in 70d0d40

if err = k.unescrowCredits(ctx, sellOrder.Seller, sellOrder.BatchId, sellOrder.Quantity); err != nil {

?

It's not wired up in BeginBlocker like PruneOrders is. We are still deleting sell orders without returning escrowed funds because of this. This should be called within PruneOrders or the way BeginBlocker is currently implemented needs to be updated (i.e. to include the PruneSellOrders method you created).

Why is this separate from the existing PruneOrders

not sure im following, is this a concern with where the function is placed?

that includes both buy

buy orders are not stored in state at the moment, so im not sure i see the value in adding them at this time.

This is only being called in the added test and we are still deleting and not properly returning escrowed credits.

well, none of these PR's are truly wired in yet. This is just impl + unit tests. I think that concern should be addressed when we tackle integration tests.

And this should be for BeginBlocker not EndBlocker

updated the comment

is this a concern with where the function is placed?

Not a concern about where the function is placed. PruneOrders is currently wired up in BeginBlocker.

well, none of these PR's are truly wired in yet

PruneOrders is already wired up in BeginBlocker.

buy orders are not stored in state at the moment

Correct but the pruning buy orders was already implemented. We can delete if need be or comment out but also does not hurt to have it included as it currently is.

Thanks for the clarification in discord. Sounds like we have more migrations to do for what I'm requesting.

ryanchristo

Ok, unblocking and approving now that we are tracking in #898. Nice work!

ryanchristo · 2022-03-15T23:07:44Z

proto/regen/ecocredit/v1/state.proto

@@ -195,6 +195,9 @@ message BatchBalance {

    // retired is the retired amount of credits
    string retired = 4;
+
+    // escrowed is the amount of credits locked up in escrow for the marketplace.
+    string escrowed = 5;


Ok, escrowed it is. More aligned with tradable and retired.

technicallyty added 3 commits March 11, 2022 17:16

feat: escrow credits

501ed4d

fix: revert mock times

cb104de

fix: typo

0ed55f4

technicallyty requested review from ryanchristo, aaronc, clevinson and robert-zaremba March 12, 2022 01:26

technicallyty added 2 commits March 11, 2022 18:48

chore: add event emission to sell

1f901eb

fix: assert precision of sell order qty

ad01737

ryanchristo mentioned this pull request Mar 12, 2022

Add message for cancel sell order #880

Closed

4 tasks

ryanchristo requested changes Mar 12, 2022

View reviewed changes

feat: pruning

d9576a8

robert-zaremba approved these changes Mar 15, 2022

View reviewed changes

aaronc reviewed Mar 15, 2022

View reviewed changes

technicallyty added 2 commits March 15, 2022 08:53

chore: fmt.Errorf -> sdkerrors.ErrInvalidRequest

cf7f9d3

Merge branch 'master' into ty/821-escrow

06f65b7

technicallyty requested a review from ryanchristo March 15, 2022 19:18

Merge branch 'master' into ty/821-escrow

70d0d40

ryanchristo requested changes Mar 15, 2022

View reviewed changes

chore: change comment

c7b4bdb

ryanchristo approved these changes Mar 15, 2022

View reviewed changes

chore: move prune test to its own file

3ab6cf8

technicallyty enabled auto-merge (squash) March 15, 2022 23:22

technicallyty merged commit 3f1dff4 into master Mar 15, 2022

technicallyty deleted the ty/821-escrow branch March 15, 2022 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(marketplace): escrow sell order credits #878

feat(marketplace): escrow sell order credits #878

technicallyty commented Mar 12, 2022 •

edited by zube bot

Loading

ryanchristo left a comment •

edited

Loading

robert-zaremba left a comment

robert-zaremba Mar 15, 2022

robert-zaremba Mar 15, 2022

technicallyty Mar 15, 2022

robert-zaremba Mar 15, 2022

aaronc Mar 15, 2022

aaronc Mar 15, 2022

technicallyty Mar 15, 2022

ryanchristo Mar 15, 2022

aaronc Mar 15, 2022

ryanchristo Mar 15, 2022

aaronc Mar 15, 2022

technicallyty Mar 15, 2022

aaronc Mar 15, 2022

codecov bot commented Mar 15, 2022 •

edited

Loading

ryanchristo left a comment •

edited

Loading

ryanchristo Mar 15, 2022

ryanchristo Mar 15, 2022 •

edited

Loading

ryanchristo Mar 15, 2022

technicallyty Mar 15, 2022

ryanchristo Mar 15, 2022 •

edited

Loading

technicallyty Mar 15, 2022 •

edited

Loading

ryanchristo Mar 15, 2022 •

edited

Loading

ryanchristo Mar 15, 2022

ryanchristo left a comment

ryanchristo Mar 15, 2022

	for {
	_, err := sellOrdersIter.LoadNext(&sellOrder)
	if err != nil {
	if orm.ErrIteratorDone.Is(err) {
	break
	}
	return err
	}
	err = s.sellOrderTable.Delete(ctx, sellOrder.OrderId)
	if err != nil {
	return err
	}
	}

feat(marketplace): escrow sell order credits #878

feat(marketplace): escrow sell order credits #878

Conversation

technicallyty commented Mar 12, 2022 • edited by zube bot Loading

Description

Author Checklist

Reviewers Checklist

ryanchristo left a comment • edited Loading

Choose a reason for hiding this comment

robert-zaremba left a 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

Choose a reason for hiding this comment

codecov bot commented Mar 15, 2022 • edited Loading

Codecov Report

ryanchristo left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

technicallyty Mar 15, 2022 • edited Loading

Choose a reason for hiding this comment

ryanchristo Mar 15, 2022 • edited Loading

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

Choose a reason for hiding this comment

technicallyty commented Mar 12, 2022 •

edited by zube bot

Loading

ryanchristo left a comment •

edited

Loading

codecov bot commented Mar 15, 2022 •

edited

Loading

ryanchristo left a comment •

edited

Loading

ryanchristo Mar 15, 2022 •

edited

Loading

ryanchristo Mar 15, 2022 •

edited

Loading

technicallyty Mar 15, 2022 •

edited

Loading

ryanchristo Mar 15, 2022 •

edited

Loading