feat(ecocredit): specify fee in `CreateClass` #912

technicallyty · 2022-03-19T05:35:26Z

Description

adds fee to CreateClass

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 · 2022-03-19T06:07:19Z

Codecov Report

Merging #912 (82e3368) into master (fc3df53) will increase coverage by 0.19%.
The diff coverage is 69.23%.

❗ Current head 82e3368 differs from pull request most recent head e5f5df7. Consider uploading reports for the commit e5f5df7 to get more accurate results

@@            Coverage Diff             @@
##           master     #912      +/-   ##
==========================================
+ Coverage   72.39%   72.59%   +0.19%     
==========================================
  Files         194      185       -9     
  Lines       22880    22754     -126     
==========================================
- Hits        16565    16518      -47     
+ Misses       5072     5026      -46     
+ Partials     1243     1210      -33

Flag	Coverage Δ
experimental-codecov	`72.47% <69.23%> (+0.18%)`	⬆️
stable-codecov	`65.65% <53.84%> (+0.13%)`	⬆️

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

technicallyty · 2022-03-21T20:17:03Z

x/ecocredit/server/core/create_class.go

 	// Charge the admin a fee to create the credit class
-	err = k.chargeCreditClassFee(sdkCtx, adminAddress, params.CreditClassFee)
+	err = k.chargeCreditClassFee(sdkCtx, adminAddress, sdk.Coins{sdk.Coin{Denom: req.Fee.Denom, Amount: feeAmt}})


force the fee to the amount specified by the params, rather than the amount passed, seemed like the proper thing to do?

Correct. We don't want to charge more than the amount in the params but we also don't want to fail if more is provided by the user so we charge the amount specified in the params.

yes, related to my earlier review. I think we should adjust.

ryanchristo

Looks good to me. A couple minor suggestions. Pre-approving.

ryanchristo · 2022-03-21T22:54:10Z

x/ecocredit/server/core/create_class.go

 	// Charge the admin a fee to create the credit class
-	err = k.chargeCreditClassFee(sdkCtx, adminAddress, params.CreditClassFee)
+	err = k.chargeCreditClassFee(sdkCtx, adminAddress, sdk.Coins{sdk.Coin{Denom: req.Fee.Denom, Amount: feeAmt}})


Correct. We don't want to charge more than the amount in the params but we also don't want to fail if more is provided by the user so we charge the amount specified in the params.

proto/regen/ecocredit/v1/tx.proto

x/ecocredit/server/core/create_class.go

Co-authored-by: Ryan Christoffersen <[email protected]>

robert-zaremba

I would prefer that we use use a bare denom instead of Fee/ Coin. And definitely let's remove the Fee type.

robert-zaremba · 2022-03-22T22:51:28Z

proto/regen/ecocredit/v1/tx.proto

+
+  // fee specifies the fee to pay for the creation of the credit class.
+  // acceptable fees for creating a credit class can be found in the governance parameters for the ecocredit module.
+  Fee fee = 5;


why someone would pay a different fee than the required one? Maybe we can use only string denom here and charge the required fee.

If we really need to use both denom and amount, then let's use Coin type and remove Fee message. We can make it required (it's very small, so we in fact optimize the performance) to avoid null checks.

i believe the intent was to ensure users aren't being charged fees that they are not explicitly setting. i had shared your concern while impl this PR, so might be nice to get more opinions in here. cc: @aaronc @clevinson @ryanchristo

i changed it to coin, but im not sure about the nullable thing. pulsar doesn't support that (not sure if thats planned either?). it might be best to leave out the option knowing we we will switch fully to pulsar eventually

why someone would pay a different fee than the required one? Maybe we can use only string denom here and charge the required fee.

The intention was to have the user explicitly set the fee so that they are aware of the charges incurred for creating a credit class. This currently happens in the background and in some cases the user might overlook the cost of creating a credit class, so having them explicitly set the fee prevents this.

since we are automatically adjusting, then maybe let's update the docs if we want to keep this attribute.

robert-zaremba · 2022-03-22T22:51:50Z

proto/regen/ecocredit/v1/types.proto

+
+  // amount is the amount of fees to pay in the specified denom.
+  string amount = 2;
+}


We don't need that type. Let's use Coin instead.

robert-zaremba · 2022-03-22T22:52:53Z

x/ecocredit/core/msg_create_class.go

@@ -46,6 +46,17 @@ func (m *MsgCreateClass) ValidateBasic() error {
 		}
 	}

+	if m.Fee != nil {


ok, if it's required, and a struct is small (even if we use Coin), then we can add option for nullable = false.

technicallyty and others added 12 commits March 18, 2022 21:11

chore: sdk.Msg impl for v1, fmts

4a5a4bf

Merge branch 'master' into ty/900-msg_impl

3728a55

chore: imports

633f322

feat: add fee to proto msg

377cccf

Merge branch 'ty/900-msg_impl' into ty/728-speicy_fee

6d33e7b

feat: add fee to state machine

3634d35

chore: add t.Parallel to all static tests

1e7f774

chore: add msg test

1496b8e

chore: CreditTypeAbbreviation alignment

45cf559

Merge branch 'ty/900-msg_impl' into ty/728-specify_class_fee

d30b115

feat: unit test

324fe75

fix: charge the required amt, not the given

4207ec1

Merge branch 'master' into ty/728-specify_class_fee

d444254

technicallyty commented Mar 21, 2022

View reviewed changes

technicallyty marked this pull request as ready for review March 21, 2022 20:17

Merge branch 'master' into ty/728-specify_class_fee

8415424

ryanchristo approved these changes Mar 21, 2022

View reviewed changes

technicallyty and others added 3 commits March 21, 2022 16:50

Update proto/regen/ecocredit/v1/tx.proto

a7554a1

Co-authored-by: Ryan Christoffersen <[email protected]>

chore: specify denom in less than error

67d36d0

Merge branch 'master' into ty/728-specify_class_fee

e234e74

robert-zaremba reviewed Mar 22, 2022

View reviewed changes

technicallyty and others added 2 commits March 22, 2022 16:58

refactor: Fee -> sdk.Coin

56c8e30

Merge branch 'master' into ty/728-specify_class_fee

82e3368

robert-zaremba approved these changes Mar 25, 2022

View reviewed changes

Merge branch 'master' into ty/728-specify_class_fee

e5f5df7

technicallyty enabled auto-merge (squash) March 25, 2022 19:30

technicallyty merged commit 1df8b21 into master Mar 25, 2022

technicallyty deleted the ty/728-specify_class_fee branch March 25, 2022 19:32

ryanchristo mentioned this pull request Mar 27, 2022

ecocredit refactor follow-ups #728

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ecocredit): specify fee in `CreateClass` #912

feat(ecocredit): specify fee in `CreateClass` #912

technicallyty commented Mar 19, 2022 •

edited by zube bot

Loading

codecov bot commented Mar 19, 2022 •

edited

Loading

technicallyty Mar 21, 2022

ryanchristo Mar 21, 2022

robert-zaremba Mar 25, 2022

ryanchristo left a comment

ryanchristo Mar 21, 2022

robert-zaremba left a comment

robert-zaremba Mar 22, 2022

technicallyty Mar 23, 2022 •

edited

Loading

ryanchristo Mar 24, 2022

robert-zaremba Mar 25, 2022

robert-zaremba Mar 22, 2022

robert-zaremba Mar 22, 2022

feat(ecocredit): specify fee in CreateClass #912

feat(ecocredit): specify fee in CreateClass #912

Conversation

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

Description

Author Checklist

Reviewers Checklist

codecov bot commented Mar 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

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

robert-zaremba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technicallyty Mar 23, 2022 • edited Loading

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

feat(ecocredit): specify fee in `CreateClass` #912

feat(ecocredit): specify fee in `CreateClass` #912

technicallyty commented Mar 19, 2022 •

edited by zube bot

Loading

codecov bot commented Mar 19, 2022 •

edited

Loading

technicallyty Mar 23, 2022 •

edited

Loading