feat(x/ecocredit): introduce core params API #960
Conversation
technicallyty
requested review from
aaronc,
robert-zaremba,
ryanchristo and
aleem1314
| "github.com/cosmos/cosmos-sdk/x/auth/legacy/legacytx" | ||
| ) | ||
|
|
||
| var _, _, _, _ legacytx.LegacyMsg = &MsgAddCreditType{}, &MsgUpdateClassCreatorAllowlist{}, &MsgToggleAllowlist{}, &MsgUpdateClassFee{} |
There was a problem hiding this comment.
each of these can be placed in their own file, just lumping them in here for now to appease the CI
Codecov Report
@@ Coverage Diff @@
## master #960 +/- ##
=========================================
Coverage ? 72.93%
=========================================
Files ? 198
Lines ? 23375
Branches ? 0
=========================================
Hits ? 17049
Misses ? 5010
Partials ? 1316
Flags with carried forward coverage won't be shown. Click here to find out more. |
There was a problem hiding this comment.
This is looking good and I'm excited for the gov module updates.
That being said, 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.
I think we should avoid batch updates to these parameters and each update should warrant its own proposal. Curious to hear what @clevinson and @aaronc think.
Also, make sure you run make proto-format.
| // CreditClassFee is a valid coin denom and amount that may be used as the fee | ||
| // for credit class creation. | ||
| message CreditClassFee { | ||
| // 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 credit class. | ||
| string denom = 1; | ||
|
|
||
| // amount is the amount of the fee required to create a credit class. | ||
| string amount = 2; | ||
| } |
There was a problem hiding this comment.
Do we want to support multiple fees? Should this be a singleton table?
I known I already brought this up in the previous pull request and the two of us moved on because the current live implementation uses repeated but I'm pretty sure we only want to support a single set fee (and we want the fee to be in REGEN).
Even though the current implementation uses repeated, the name of the field is credit_class_fee and I think this is actually an oversight. Looking back at the original issue and pull request, I don't see any mention of multiple fees (#351 and #375) nor do I recall conversations about supporting multiple fees.
Also, we should use cosmos.base.v1beta1.Coin.
| // CreditClassFee is a valid coin denom and amount that may be used as the fee | |
| // for credit class creation. | |
| message CreditClassFee { | |
| // 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 credit class. | |
| string denom = 1; | |
| // amount is the amount of the fee required to create a credit class. | |
| string amount = 2; | |
| } | |
| // CreditClassFee is the fee required to create a credit class. | |
| message CreditClassFee { | |
| // 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 credit class. | |
| cosmos.base.v1beta1.Coin fee = 1 | |
| } |
| // AddCreditType is a governance method that allows the addition of new credit types to the network. | ||
| rpc AddCreditType(MsgAddCreditType) returns (MsgAddCreditTypeResponse); |
There was a problem hiding this comment.
| // AddCreditType is a governance method that allows the addition of new credit types to the network. | |
| rpc AddCreditType(MsgAddCreditType) returns (MsgAddCreditTypeResponse); | |
| // AddCreditType is a governance method that adds new credit types to the network. | |
| rpc AddCreditType(MsgAddCreditType) returns (MsgAddCreditTypeResponse); |
| // UpdateClassCreatorAllowlist is a governance method that allows for the addition and removal of addresses from the class creation allowlist. | ||
| // note: the allowlist is only effective when the allowlist parameter is set to false. | ||
| rpc UpdateClassCreatorAllowlist(MsgUpdateClassCreatorAllowlist) returns (MsgUpdateClassCreatorAllowlistResponse); |
There was a problem hiding this comment.
In line with AllowedAskDenoms and the current allowed_class_creators param, I think we should update the name here to include AllowedClassCreator or AllowedCreditClassCreator.
I also think we should have two methods here, one for adding a credit class creator and one for removing a credit class creator. Adding and removing credit class creators are significant actions and I think each credit class creator added or removed warrants its own proposal.
| // UpdateClassCreatorAllowlist is a governance method that allows for the addition and removal of addresses from the class creation allowlist. | |
| // note: the allowlist is only effective when the allowlist parameter is set to false. | |
| rpc UpdateClassCreatorAllowlist(MsgUpdateClassCreatorAllowlist) returns (MsgUpdateClassCreatorAllowlistResponse); | |
| // AddAllowedCreditClassCreator is a governance method that adds an address to the list of allowed credit class creators. | |
| rpc AddAllowedCreditClassCreator(MsgAddAllowedCreditClassCreator) returns (MsgAddAllowedCreditClassCreatorResponse); | |
| // RemoveAllowedCreditClassCreator is a governance method that removes an address from the list of allowed credit class creators. | |
| rpc RemoveAllowedCreditClassCreator(MsgRemoveAllowedCreditClassCreator) returns (MsgRemoveAllowedCreditClassCreatorResponse); |
| // ToggleAllowlist is a governance method that toggles the network allowlist to on or off. | ||
| // when on, the class creator allowlist is used to enforce which addresses may create classes. | ||
| // when off, any address may create classes. | ||
| rpc ToggleAllowlist(MsgToggleAllowlist) returns (MsgToggleAllowlistResponse); |
There was a problem hiding this comment.
| // ToggleAllowlist is a governance method that toggles the network allowlist to on or off. | |
| // when on, the class creator allowlist is used to enforce which addresses may create classes. | |
| // when off, any address may create classes. | |
| rpc ToggleAllowlist(MsgToggleAllowlist) returns (MsgToggleAllowlistResponse); | |
| // ToggleCreditClassAllowlist is a governance method that toggles the network allowlist to on or off. | |
| // when on, the class creator allowlist is used to enforce which addresses may create classes. | |
| // when off, any address may create classes. | |
| rpc ToggleCreditClassAllowlist(MsgToggleCreditClassAllowlist) returns (MsgToggleCreditClassAllowlistResponse); |
| // UpdateClassFee is a governance method that allows for the addition and removal of fees to be used for the | ||
| // class creation fee. | ||
| rpc UpdateClassFee(MsgUpdateClassFee) returns (MsgUpdateClassFeeResponse); |
There was a problem hiding this comment.
| // UpdateClassFee is a governance method that allows for the addition and removal of fees to be used for the | |
| // class creation fee. | |
| rpc UpdateClassFee(MsgUpdateClassFee) returns (MsgUpdateClassFeeResponse); | |
| // UpdateCreditClassFee is a governance method that updates the credit class creation fee. | |
| rpc UpdateCreditClassFee(MsgUpdateCreditClassFee) returns (MsgUpdateCreditClassFeeResponse); |
| // MsgAddCreditType is the Msg/AddCreditType request type. | ||
| message MsgAddCreditType { | ||
|
|
||
| // 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; | ||
|
|
||
| // credit_types defines a credit type to add to the credit types parameter. | ||
| repeated CreditType credit_types = 2; | ||
| } |
There was a problem hiding this comment.
Either we should rename this message to MsgAddCreditTypes or we should only allow one credit type to be updated at a time. Similar to adding and removing credit class creators, adding a credit types is a significant action and I think each credit type added warrants its own proposal.
| // MsgAddCreditType is the Msg/AddCreditType request type. | |
| message MsgAddCreditType { | |
| // 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; | |
| // credit_types defines a credit type to add to the credit types parameter. | |
| repeated CreditType credit_types = 2; | |
| } | |
| // MsgAddCreditType is the Msg/AddCreditType request type. | |
| message MsgAddCreditType { | |
| // 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; | |
| // credit_types defines a credit type to add to the credit types parameter. | |
| CreditType credit_type = 2; | |
| } |
| // MsgUpdateClassCreatorAllowlist is the Msg/UpdateClassCreatorAllowlist request type. | ||
| message MsgUpdateClassCreatorAllowlist{ | ||
|
|
||
| // 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_creators is a list of addresses to add to the allowed class creator list | ||
| repeated string add_creators = 2; | ||
|
|
||
| // remove_creators is a list of addresses to remove from the allowed class creator list | ||
| repeated string remove_creators = 3; | ||
| } |
There was a problem hiding this comment.
As mentioned above, I think this should be split into two methods, one for adding a single credit class creator and one for removing a single credit class creator.
| // setting defines the boolean value to set the allowlist on or off. | ||
| bool setting = 2; |
There was a problem hiding this comment.
| // setting defines the boolean value to set the allowlist on or off. | |
| bool setting = 2; | |
| // enabled determines whether the credit class creator allowlist is enabled. | |
| bool enabled = 2; |
| // MsgUpdateClassFee is the Msg/UpdateClassFee request type. | ||
| message MsgUpdateClassFee { | ||
|
|
||
| // 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 class creation fee. | ||
| repeated cosmos.base.v1beta1.Coin add_fees = 2; | ||
|
|
||
| // remove_fee_denoms defines a list of denoms to remove from the list of class creation fees. | ||
| repeated string remove_fee_denoms = 3; | ||
| } |
There was a problem hiding this comment.
As mentioned above, I think we want to support a single credit class fee and this should update that single fee.
ryanchristo
changed the title
|
closing in favor of #1354 |
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.mdReviewers 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