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

feat: ecocredit direct buy/sell proto definitions #560

Merged
merged 15 commits into from
Oct 28, 2021
Merged
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
144 changes: 143 additions & 1 deletion proto/regen/ecocredit/v1alpha1/tx.proto
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package regen.ecocredit.v1alpha1;

import "gogoproto/gogo.proto";
import "google/protobuf/timestamp.proto";
import "cosmos/base/v1beta1/coin.proto";

option go_package = "github.com/regen-network/regen-ledger/x/ecocredit";

Expand Down Expand Up @@ -39,6 +40,18 @@ service Msg {

// UpdateClassMetadata updates the credit class metadata
rpc UpdateClassMetadata(MsgUpdateClassMetadata) returns (MsgUpdateClassMetadataResponse);

// Sell creates new sell orders.
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

rpc Sell(MsgSell) returns (MsgSellResponse);
clevinson marked this conversation as resolved.
Show resolved Hide resolved

// UpdateSellOrders updates existing sell orders.
rpc UpdateSellOrders(MsgUpdateSellOrders) returns (MsgUpdateSellOrdersResponse);

// BuyDirect creates buy orders directly against sell orders.
rpc BuyDirect (MsgBuyDirect) returns (MsgBuyDirectResponse);
clevinson marked this conversation as resolved.
Show resolved Hide resolved

// AllowAskDenom is a governance operation which authorizes a new ask denom to be used in sell orders
rpc AllowAskDenom (MsgAllowAskDenom) returns (MsgAllowAskDenomResponse);
blushi marked this conversation as resolved.
Show resolved Hide resolved
}

// MsgCreateClass is the Msg/CreateClass request type.
Expand Down Expand Up @@ -274,4 +287,133 @@ message MsgUpdateClassMetadata {
}

// MsgUpdateClassMetadataResponse is the MsgUpdateClassMetadata response type.
message MsgUpdateClassMetadataResponse {}
message MsgUpdateClassMetadataResponse {}

// MsgSell is the Msg/Sell request type.
message MsgSell {

// owner is the address of the owner of the credits being sold.
string owner = 1;

// orders are the sell orders being created.
repeated Order orders = 2;

// Order is the content of a new sell order.
message Order {

// batch_denom is the credit batch being sold.
string batch_denom = 1;

// quantity is the quantity of credits being sold from this batch. If it is
// less then the balance of credits the owner has available, the order will
// be adjusted downwards to the owner's balance.
string quantity = 2;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought we agreed on partial fills being the expected behavior


// ask_price is the price the seller is asking for each unit of the
// batch_denom. Each credit unit of the batch will be sold for at least the
// ask_price or more.
cosmos.base.v1beta1.Coin ask_price = 3;

// dont_auto_retire disables auto-retirement of credits which allows a
// buyer to disable auto-retirement in their buy order enabling them to
// resell the credits to another buyer.
bool dont_auto_retire = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that sounds better

}
aaronc marked this conversation as resolved.
Show resolved Hide resolved
}

// MsgSellResponse is the Msg/Sell response type.
message MsgSellResponse {

// sell_order_ids are the sell order IDs of the newly created sell orders.
repeated uint64 sell_order_ids = 1;
}

// MsgUpdateSellOrders is the Msg/UpdateSellOrders request type.
message MsgUpdateSellOrders {

// owner is the owner of the sell orders.
string owner = 1;

// updates are updates to existing sell orders.
repeated Update updates = 2;

// Update is an update to an existing sell order.
message Update {

// sell_order_id is the ID of an existing sell order.
uint64 sell_order_id = 1;

// new_quantity is the updated quantity of credits available to sell, if it
// is set to zero then the order is cancelled.
string new_quantity = 2;

// new_ask_price is the new ask price for this sell order
cosmos.base.v1beta1.Coin new_ask_price = 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need a new_ prefix?

Copy link
Member Author

Choose a reason for hiding this comment

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

not necessarily, but it doesn't hurt


// dont_auto_retire updates the dont_auto_retire field in the sell order.
bool dont_auto_retire = 4;
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok could you add this assumption to the docs then?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

}
}

// MsgUpdateSellOrdersResponse is the Msg/UpdateSellOrders response type.
message MsgUpdateSellOrdersResponse {}
aaronc marked this conversation as resolved.
Show resolved Hide resolved

// MsgBuyDirect is the Msg/BuyDirect request type.
message MsgBuyDirect {

// buyer is the address of the credit buyer.
string buyer = 1;

// orders are the new buy orders.
repeated Order orders = 2;

// Order is a direct buy order.
message Order {

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

// quantity is the quanity of credits to buy.
aaronc marked this conversation as resolved.
Show resolved Hide resolved
string quantity = 2;
aaronc marked this conversation as resolved.
Show resolved Hide resolved

// bid price is the bid price for this buy order. A credit unit will be
// settled at a purchase price that is no more than the bid price.
cosmos.base.v1beta1.Coin bid_price = 3;

// dont_auto_retire allows auto-retirement to be disabled. If it is set to true
// the credits will not auto-retire and can be resold assuming that the
// corresponding sell order has auto-retirement disabled. If the sell order
// hasn't disabled auto-retirement and the buy order tries to disable it,
// that buy order will fail.
bool dont_auto_retire = 4;
blushi marked this conversation as resolved.
Show resolved Hide resolved
}
}

// MsgBuyDirectResponse is the Msg/BuyDirect response type.
message MsgBuyDirectResponse {

// buy_order_ids are the buy order IDs of the newly created buy order. Buy
aaronc marked this conversation as resolved.
Show resolved Hide resolved
// orders may not settle instantaneously, but rather in batches at specified
// batch epoch times.
repeated uint64 buy_order_ids = 1;
clevinson marked this conversation as resolved.
Show resolved Hide resolved
}


// MsgAllowAskDenom is the Msg/AllowAskDenom request type.
message MsgAllowAskDenom {
// root_address is the address of the governance account which can authorize ask denoms
string root_address = 1;
clevinson marked this conversation as resolved.
Show resolved Hide resolved

// denom is the denom to allow (ex. ibc/GLKHDSG423SGS)
string denom = 2;

// display_denom is the denom to display to the user and is informational
string display_denom = 3;

// exponent is the exponent that relates the denom to the display_denom and is
// informational
uint32 exponent = 4;
}

// MsgAllowAskDenomResponse is the Msg/AllowAskDenom response type.
message MsgAllowAskDenomResponse {}