feat!: Add support for credit cancelling (#385) #391

ruhatch · 2021-06-17T16:53:31Z

Description

This PR adds support for cancelling ecocredits. An account holder can cancel credits that they own from any credit batch. Cancelled credits are removed from the holders tradeable balance and from the tradeable supply. Cancelling credits has no effect on the retirement balances and supplies. After cancellation it is as if the credit was never issued, except that it is still accounted for in the total_amount in the BatchInfo for the batch.

Closes: #385

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 · 2021-06-17T16:58:31Z

Codecov Report

Merging #391 (ec8e478) into master (3b7c517) will decrease coverage by 3.50%.
The diff coverage is 55.14%.

@@            Coverage Diff             @@
##           master     #391      +/-   ##
==========================================
- Coverage   59.87%   56.36%   -3.51%     
==========================================
  Files          56       59       +3     
  Lines        3404     3717     +313     
==========================================
+ Hits         2038     2095      +57     
- Misses       1102     1351     +249     
- Partials      264      271       +7

Flag	Coverage Δ
experimental-codecov	`56.36% <55.14%> (-3.51%)`	⬇️
stable-codecov	`56.36% <55.14%> (-3.51%)`	⬇️

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

proto/regen/ecocredit/v1alpha1/events.proto

proto/regen/ecocredit/v1alpha1/tx.proto

aaronc

LGTM other than YAML in the CLI and my small thought about naming. What alternatives do we have to YAML? Could we do something like coins does (ex 100atom,1000regen)?

x/ecocredit/client/tx.go

ruhatch · 2021-06-18T10:01:09Z

@aaronc @blushi I found this as well:

// BatchInfo represents the high-level on-chain information for a credit batch.
message BatchInfo {

  // class_id is the unique ID of credit class.
  string class_id = 1 [ (gogoproto.moretags) = "yaml:\"class_id\"" ];

  // batch_denom is the unique ID of credit batch.
  string batch_denom = 2 [ (gogoproto.moretags) = "yaml:\"batch_denom\"" ];

  // issuer is the issuer of the credit batch.
  string issuer = 3;

  // total_amount is the total number of credits in the credit batch and is
  // immutable.
  string total_amount = 4 [ (gogoproto.moretags) = "yaml:\"total_amount\"" ];

  // metadata is any arbitrary metadata to attached to the credit batch.
  bytes metadata = 5;
}

It states in the comment of total_amount (which I just renamed), that this field is immutable. Does this change with the new cancellation logic and do we need to update the batch info during cancellation? Can you ask Ron if you're unsure?

ruhatch · 2021-06-18T11:52:16Z

LGTM other than YAML in the CLI and my small thought about naming. What alternatives do we have to YAML? Could we do something like coins does (ex 100atom,1000regen)?

@aaronc I've implemented with the form 100:ABC/123,256.974:123/XYZ, so please review and let me know what you think. It should be easy to adjust that format if you think we can do something better there.

aaronc · 2021-06-18T12:32:12Z

LGTM other than YAML in the CLI and my small thought about naming. What alternatives do we have to YAML? Could we do something like coins does (ex 100atom,1000regen)?

@aaronc I've implemented with the form 100:ABC/123,256.974:123/XYZ, so please review and let me know what you think. It should be easy to adjust that format if you think we can do something better there.

That sounds good. Separately we should find a way for credit batch names to always start with a letter so a colon isn't needed. I propose eco/ as a prefix for all credit classes and batches as that's what we'll be doing when we integrate with bank. What do you think?

ruhatch · 2021-06-18T13:45:51Z

That sounds good. Separately we should find a way for credit batch names to always start with a letter so a colon isn't needed. I propose eco/ as a prefix for all credit classes and batches as that's what we'll be doing when we integrate with bank. What do you think?

Yeah, I think that seems reasonable and makes it interoperable with Coin types. I'll open a separate issue for that. Is that something we'll want to get into v1.1?

aaronc · 2021-06-18T15:00:13Z

That sounds good. Separately we should find a way for credit batch names to always start with a letter so a colon isn't needed. I propose eco/ as a prefix for all credit classes and batches as that's what we'll be doing when we integrate with bank. What do you think?

Yeah, I think that seems reasonable and makes it interoperable with Coin types. I'll open a separate issue for that. Is that something we'll want to get into v1.1?

Yes 👍

likhita-809

looks neat, just a small nit

proto/regen/ecocredit/v1alpha1/events.proto

blushi

LGTM, just small comments

proto/regen/ecocredit/v1alpha1/events.proto

blushi · 2021-06-23T08:31:06Z

x/ecocredit/client/tx.go

@@ -172,6 +173,34 @@ Parameters:
 	}
 }

+func txCancel() *cobra.Command {


it doesn't have to be part of this PR but it would be good to add some integration tests for the ecocredit CLI for next release
I believe #150 should be added to v1.1 - Baikal Upgrade milestone, wdyt @clevinson @aaronc?

Yeah, sounds good - how do we test the CLI currently?

see for instance in x/group/client/testsuite

x/ecocredit/client/tx.go

blushi · 2021-06-23T08:59:21Z

@aaronc @blushi I found this as well:

// BatchInfo represents the high-level on-chain information for a credit batch.
message BatchInfo {

  // class_id is the unique ID of credit class.
  string class_id = 1 [ (gogoproto.moretags) = "yaml:\"class_id\"" ];

  // batch_denom is the unique ID of credit batch.
  string batch_denom = 2 [ (gogoproto.moretags) = "yaml:\"batch_denom\"" ];

  // issuer is the issuer of the credit batch.
  string issuer = 3;

  // total_amount is the total number of credits in the credit batch and is
  // immutable.
  string total_amount = 4 [ (gogoproto.moretags) = "yaml:\"total_amount\"" ];

  // metadata is any arbitrary metadata to attached to the credit batch.
  bytes metadata = 5;
}

It states in the comment of total_amount (which I just renamed), that this field is immutable. Does this change with the new cancellation logic and do we need to update the batch info during cancellation? Can you ask Ron if you're unsure?

Not sure, will ask Ron.
Any thoughts on that @aaronc?

aaronc · 2021-06-23T13:09:38Z

I think we should update the docs to mention cancellation.

ruhatch · 2021-06-29T12:50:49Z

I think we should update the docs to mention cancellation.

@aaronc And do we need to change the logic so that when we cancel credits, the total_amount changes in the batch info?

blushi · 2021-06-29T13:59:17Z

I think we should update the docs to mention cancellation.

@aaronc And do we need to change the logic so that when we cancel credits, the total_amount changes in the batch info?

I asked Ron about that he responded:

It might make sense to track the “active”, “retired” and “canceled” and “initial issuance” separately. Where active = initial - canceled - retired.

We do already keep track of active and retired credits as part of the tradable and retired balances / supplies. Canceled credits can be indexed based on cancel events. So I believe it would make sense to keep the total_amount unchanged.

ruhatch · 2021-06-29T14:05:36Z

We do already keep track of active and retired credits as part of the tradable and retired balances / supplies. Canceled credits can be indexed based on cancel events. So I believe it would make sense to keep the total_amount unchanged.

Okay, sounds good! I'll adjust some docs that suggest cancel is as if they're never issued, because that's not the case here. Thanks for checking that!

aaronc · 2021-06-29T15:16:50Z

I think language which distinguishes issued from active/valid at the batch level makes sense. I think leaving total amount as is could be confusing

ruhatch · 2021-06-29T15:26:05Z

I think language which distinguishes issued from active/valid at the batch level makes sense. I think leaving total amount as is could be confusing

@aaronc hmm, okay, so perhaps renaming to total_issued or even total_amount_issued? Would that be enough?

aaronc · 2021-06-29T15:37:06Z

I think language which distinguishes issued from active/valid at the batch level makes sense. I think leaving total amount as is could be confusing

@aaronc hmm, okay, so perhaps renaming to total_issued or even total_amount_issued? Would that be enough?

Sure, but then I would also add a total_valid field or something. Honestly, I think it would be cleaner to just change the total_amount to reflect the cancellation. My concern is that people will use these numbers in analytics and misinterpret the number of valid carbon credits.

ruhatch · 2021-06-29T15:49:03Z

Sure, but then I would also add a total_valid field or something. Honestly, I think it would be cleaner to just change the total_amount to reflect the cancellation. My concern is that people will use these numbers in analytics and misinterpret the number of valid carbon credits.

@aaronc Hmm, yeah, I had similar concerns.

Ron's requirement above only needs us to track 3 of those 4 values, so we can decide which one to leave for reconstruction, or just accept the redundancy and store them all?

What about adjusting total_amount and adding something like amount_cancelled, so that someone can reconstruct the total_issued, but is unlikely to use the wrong values?

amaury1093 · 2021-07-06T13:07:55Z

@aaronc Could you have a look at Ru's proposal above to get this PR move forward?

aaronc · 2021-07-06T17:05:49Z

Sure, but then I would also add a total_valid field or something. Honestly, I think it would be cleaner to just change the total_amount to reflect the cancellation. My concern is that people will use these numbers in analytics and misinterpret the number of valid carbon credits.

@aaronc Hmm, yeah, I had similar concerns.

Ron's requirement above only needs us to track 3 of those 4 values, so we can decide which one to leave for reconstruction, or just accept the redundancy and store them all?

What about adjusting total_amount and adding something like amount_cancelled, so that someone can reconstruct the total_issued, but is unlikely to use the wrong values?

I think that makes sense and is likely least error prone.

ruhatch · 2021-07-07T14:26:59Z

@aaronc could you give this another review with the BatchInfo updates? Hoping that it should be basically there now.

likhita-809

lgtm

ruhatch requested review from aaronc, blushi, robert-zaremba, aleem1314, amaury1093, anilcse, atheeshp, cyberbono3, likhita-809, ryanchristo and technicallyty June 17, 2021 16:54

aaronc reviewed Jun 17, 2021

View reviewed changes

proto/regen/ecocredit/v1alpha1/events.proto Outdated Show resolved Hide resolved

aaronc reviewed Jun 17, 2021

View reviewed changes

proto/regen/ecocredit/v1alpha1/tx.proto Outdated Show resolved Hide resolved

aaronc reviewed Jun 17, 2021

View reviewed changes

x/ecocredit/client/tx.go Outdated Show resolved Hide resolved

ruhatch changed the title ~~feat: Add support for credit cancelling (#385)~~ feat!: Add support for credit cancelling (#385) Jun 18, 2021

cyberbono3 assigned blushi and likhita-809 Jun 21, 2021

likhita-809 reviewed Jun 22, 2021

View reviewed changes

proto/regen/ecocredit/v1alpha1/events.proto Outdated Show resolved Hide resolved

blushi reviewed Jun 23, 2021

View reviewed changes

ruhatch added 6 commits June 29, 2021 15:46

feat: Add support for credit cancelling (#385)

53d388e

Add to CHANGELOG.md

d6b9565

Rename units to amount throughout ecocredit module

b0fa972

Replace YAML with custom parsing on command line

31eceba

Add tests for parseCancelCreditsList

745a478

Address PR review comments

15bcd85

ruhatch force-pushed the ruhatch/cancel-credit branch from 71393f9 to 15bcd85 Compare June 29, 2021 14:48

ruhatch requested review from aaronc and blushi June 29, 2021 14:48

Merge branch 'master' into ruhatch/cancel-credit

e63600d

Update BatchInfo when cancelling credits

effeec4

ruhatch requested a review from likhita-809 July 7, 2021 14:26

ruhatch enabled auto-merge (squash) July 7, 2021 14:27

aleem1314 disabled auto-merge July 7, 2021 14:28

ruhatch enabled auto-merge (squash) July 7, 2021 14:57

likhita-809 approved these changes Jul 9, 2021

View reviewed changes

Merge branch 'master' into ruhatch/cancel-credit

ec8e478

aaronc approved these changes Jul 12, 2021

View reviewed changes

ruhatch merged commit c339103 into master Jul 12, 2021

ruhatch deleted the ruhatch/cancel-credit branch July 12, 2021 16:44

clevinson mentioned this pull request Jul 27, 2021

Ecocredit Module on Mainnet #78

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Add support for credit cancelling (#385) #391

feat!: Add support for credit cancelling (#385) #391

ruhatch commented Jun 17, 2021 •

edited by zube bot

Loading

codecov bot commented Jun 17, 2021 •

edited

Loading

aaronc left a comment

ruhatch commented Jun 18, 2021

ruhatch commented Jun 18, 2021 •

edited

Loading

aaronc commented Jun 18, 2021

ruhatch commented Jun 18, 2021

aaronc commented Jun 18, 2021

likhita-809 left a comment

blushi left a comment

blushi Jun 23, 2021

ruhatch Jun 29, 2021

blushi Jun 29, 2021

blushi commented Jun 23, 2021 •

edited

Loading

aaronc commented Jun 23, 2021

ruhatch commented Jun 29, 2021

blushi commented Jun 29, 2021

ruhatch commented Jun 29, 2021

aaronc commented Jun 29, 2021

ruhatch commented Jun 29, 2021

aaronc commented Jun 29, 2021

ruhatch commented Jun 29, 2021 •

edited

Loading

amaury1093 commented Jul 6, 2021

aaronc commented Jul 6, 2021

ruhatch commented Jul 7, 2021

likhita-809 left a comment

feat!: Add support for credit cancelling (#385) #391

feat!: Add support for credit cancelling (#385) #391

Conversation

ruhatch commented Jun 17, 2021 • edited by zube bot Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Jun 17, 2021 • edited Loading

Codecov Report

aaronc left a comment

Choose a reason for hiding this comment

ruhatch commented Jun 18, 2021

ruhatch commented Jun 18, 2021 • edited Loading

aaronc commented Jun 18, 2021

ruhatch commented Jun 18, 2021

aaronc commented Jun 18, 2021

likhita-809 left a comment

Choose a reason for hiding this comment

blushi left a comment

Choose a reason for hiding this comment

blushi Jun 23, 2021

Choose a reason for hiding this comment

ruhatch Jun 29, 2021

Choose a reason for hiding this comment

blushi Jun 29, 2021

Choose a reason for hiding this comment

blushi commented Jun 23, 2021 • edited Loading

aaronc commented Jun 23, 2021

ruhatch commented Jun 29, 2021

blushi commented Jun 29, 2021

ruhatch commented Jun 29, 2021

aaronc commented Jun 29, 2021

ruhatch commented Jun 29, 2021

aaronc commented Jun 29, 2021

ruhatch commented Jun 29, 2021 • edited Loading

amaury1093 commented Jul 6, 2021

aaronc commented Jul 6, 2021

ruhatch commented Jul 7, 2021

likhita-809 left a comment

Choose a reason for hiding this comment

ruhatch commented Jun 17, 2021 •

edited by zube bot

Loading

codecov bot commented Jun 17, 2021 •

edited

Loading

ruhatch commented Jun 18, 2021 •

edited

Loading

blushi commented Jun 23, 2021 •

edited

Loading

ruhatch commented Jun 29, 2021 •

edited

Loading