-
Notifications
You must be signed in to change notification settings - Fork 103
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(x/ecocredit): introduce basket params API #962
Conversation
Codecov Report
@@ Coverage Diff @@
## master #962 +/- ##
=========================================
Coverage ? 72.96%
=========================================
Files ? 198
Lines ? 23365
Branches ? 0
=========================================
Hits ? 17048
Misses ? 5004
Partials ? 1313
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in #960, it looks like we may not update to Cosmos SDK v0.46 for Regen Ledger v4.0. We can still add the proto updates and the message implementations but we won't be able to migrate until after v4.0.
Depending on what we do for credit class fee, we should do the same here for basket fee.
Also, make sure you run make proto-format
.
// BasketFee is a valid coin denom and amount that may be used as the fee | ||
// for basket creation. | ||
message BasketFee { | ||
// This table is controlled via governance. | ||
option (cosmos.orm.v1alpha1.table) = { | ||
id : 13, | ||
primary_key : {fields : "denom"} | ||
}; | ||
|
||
// denom is the denom of the fee required to create a basket. | ||
string denom = 1; | ||
|
||
// amount is the amount of the fee required to create a basket. | ||
string amount = 2; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in #960 in relation to the credit class fee, I think we only want to support a single fee. We named the parameter basket_creation_fee
and copied the implementation for credit class fee but I don't actually recall any conversations about supporting multiple fees.
// BasketFee is a valid coin denom and amount that may be used as the fee | |
// for basket creation. | |
message BasketFee { | |
// This table is controlled via governance. | |
option (cosmos.orm.v1alpha1.table) = { | |
id : 13, | |
primary_key : {fields : "denom"} | |
}; | |
// denom is the denom of the fee required to create a basket. | |
string denom = 1; | |
// amount is the amount of the fee required to create a basket. | |
string amount = 2; | |
} | |
// BasketFee is a valid coin denom and amount that may be used as the fee | |
// for basket creation. | |
message BasketFee { | |
// This table is controlled via governance. | |
option (cosmos.orm.v1alpha1.singleton) = { | |
id : 13, | |
primary_key : {fields : "denom"} | |
}; | |
// fee is the fee required to create a basket. | |
cosmos.base.v1beta1.Coin fee = 1 | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using Coin works as long as it's not the primary key, ORM can't handle imported messages as PK's. i think the denom/amount separation is necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need a primary key with a singleton. it just has one instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right - so then we aren't concerned with fees in other denoms? if we're just going with the amount of $regen only, we could just store the amount string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just want one fee (could be multiple coins). So just a singleton with repeated Coins
// UpdateClassFee is a governance method that allows for the addition and removal of fees to be used for the | ||
// basket creation fee. | ||
rpc UpdateBasketFee(MsgUpdateBasketFee) returns (MsgUpdateBasketFeeResponse); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// UpdateClassFee is a governance method that allows for the addition and removal of fees to be used for the | |
// basket creation fee. | |
rpc UpdateBasketFee(MsgUpdateBasketFee) returns (MsgUpdateBasketFeeResponse); | |
// UpdateClassFee is a governance method that updates the basket creation fee. | |
rpc UpdateBasketFee(MsgUpdateBasketFee) returns (MsgUpdateBasketFeeResponse); |
// MsgUpdateBasketFee is the Msg/UpdateBasketFee request type. | ||
message MsgUpdateBasketFee { | ||
|
||
// root_address is the address of the signer. | ||
// This MUST equal the address of the gov module for the tx to succeed. | ||
string root_address = 1; | ||
|
||
// add_fees defines a list of coins to append to the list of fees that can be used in the basket creation fee. | ||
repeated cosmos.base.v1beta1.Coin add_fees = 2; | ||
|
||
// remove_fee_denoms defines a list of denoms to remove from the list of basket creation fees. | ||
repeated string remove_fee_denoms = 3; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned above, I think we want to support a single basket fee and this should update that single fee.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer if we avoid a -> a
pattern
|
||
// amount is the amount of the fee required to create a basket. | ||
string amount = 2; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally would be if we don't do a -> a
pattern (saving in a key concatenation of the value)
closing in favor of #1349 |
Description
Closes: n/a
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...
!
to the type prefix if API or client breaking changeCHANGELOG.md
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...
!
in the type prefix if API or client breaking change