Eco-credit Module Proof of Concept Init #125

aaronc · 2020-11-04T15:42:12Z

Credit Module Proto Files

This includes protobuf files for the design outlined in #117, but it does so without leveraging the existing bank module. Instead the implementation (#119) does its own balance and supply tracking (IMHO more correctly than x/bank). As I've said before, we can definitely consider alternate designs (like #115 or whatever we find is needed by users). This is just the design I've had for over a year now (and originally PoC'ed in SF in Oct, 2019). It was pretty simple to implement and I'd just like to get it out there so we can iterate from there.

Decimal Balance Experiment

This design also includes an implementation of cosmos/cosmos-sdk#7113 done with https://github.com/cockroachdb/apd. This feels relevant especially concerning the problems we're having with sdk.Dec (cosmos/cosmos-sdk#7773. In particular, I wanted to try out apd, to see if it may be a suitable replacement for sdk.Dec.

Feel free to submit a Draft Pull Request as soon as you start working on something. Before you mark your PR as Ready For Review, make sure you've checked off the following boxes or indicated N/A (not applicable):

linked to a relevant issue or have written a clear description of the issue in the PR
wrote or updated tests
wrote or updated documentation
added an appropriate entry in CHANGELOG.md following https://keepachangelog.com

Thanks!

codecov · 2020-11-04T15:44:33Z

Codecov Report

Merging #125 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #125   +/-   ##
=======================================
  Coverage   37.34%   37.34%           
=======================================
  Files          11       11           
  Lines         905      905           
=======================================
  Hits          338      338           
  Misses        561      561           
  Partials        6        6

proto/regen/ecocredit/v1alpha1/tx.proto

proto/regen/ecocredit/v1alpha1/events.proto

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

aaronc · 2020-11-04T19:17:55Z

Other than those comments @blushi does this look okay to move forward with? You honestly have the most context from the registry side.

…' into aaronc/78-credit-module-poc-init

…aronc/78-credit-module-poc-init

proto/regen/ecocredit/v1alpha1/events.proto

robert-zaremba · 2020-11-04T21:37:01Z

proto/regen/ecocredit/v1alpha1/events.proto

+    string class_id = 1;
+
+    // batch_denom is the unique ID of credit batch.
+    string batch_denom = 2;


could you be more precise here: is the batch_denom unique inside the given class, or globally?

Also, why we are calling it batch_denom ? Why not batch_id? I know that we use denom for an asset name, but here it's meaning (without a comment) is not very clear.

Yep I can add some clarifying language on denominations

robert-zaremba · 2020-11-04T21:38:21Z

proto/regen/ecocredit/v1alpha1/events.proto

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


This will be an address, right? Maybe let's put that in a comment:

// credit batch issuer account address

robert-zaremba · 2020-11-04T21:40:33Z

proto/regen/ecocredit/v1alpha1/events.proto

+        // sender is the sender of the credits in the case that this event is the result of a transfer.
+        // It will not be set if credits were issued.
+        string sender = 2;
+    }


There is only one issuer, so we don't need to code it here. In Ethereum, the standard is to put sender=0 (zero value) for minting / issuance transfers. Maybe we can use the same thing here (sender = "")?

Other idea, instead of oneof would be to add a boolean flag here.

I don't have to use oneof true, but they are mutually exclusive. I can just make sender = "" if that's preferred

The whole thing is to eliminate theoneof and normalize the data in the RDBMS sense (so to not use the issuer field at all here). Adding a boolean flag is only cosmetic if we want to be explicit.

well i'm trying to distinguish between issuer and sender as roles...

I know. My point is that issuer is known and it must be the same as the batch issuer. By adding it here we denormalize the data.

Right, so I'm removing and just leaving sender = ""

robert-zaremba · 2020-11-04T21:42:33Z

proto/regen/ecocredit/v1alpha1/events.proto

+    string recipient = 3;
+
+    // batch_denom is the unique ID of credit batch.
+    string batch_denom = 4;


ok, so I think this answers my comment above: batch_denom is unique globally.

robert-zaremba · 2020-11-04T22:18:32Z

proto/regen/ecocredit/v1alpha1/tx.proto

+
+        // retired_units are the units of credits in this issuance that are effectively retired by the issuer on receipt.
+        // Decimal values are acceptable within the precision returned by Query/Precision.
+        string retired_units = 3;


As above, retired units are not tradeable - so doesn't it make sense to send it over?

Not sure, but likely yes. A broker may specifically sell credits that can't be traded as part of the contract. Again a business discussion to have with Ron

Indeed, there are cases where sold credits should be "auto-retired".

proto/regen/ecocredit/v1alpha1/tx.proto

robert-zaremba · 2020-11-04T22:23:13Z

proto/regen/ecocredit/v1alpha1/tx.proto

+    // credits are the credits being retired.
+    repeated RetireUnits credits = 2;
+
+    // RetireUnits are the units of the batch being retired.


this description should use the same language as the comment in line 134. Also, related to the comment above about changing a vocabulary for units (in proto/regen/ecocredit/v1alpha1/events.proto)

robert-zaremba · 2020-11-04T22:24:47Z

proto/regen/ecocredit/v1alpha1/types.proto

+    string total_units = 4;
+
+    // metadata is any arbitrary metadata to attached to the credit batch.
+    bytes metadata = 5;


related to the comment above - let's consider adding a certificate field.

again let's discuss metadata elsewhere. we don't have context on the engineering team to make these sorts of decisions

robert-zaremba · 2020-11-04T22:32:22Z

proto/regen/ecocredit/v1alpha1/events.proto

+
+    // units is the decimal number of credits that have been retired.
+    string units = 3;
+}


We should also add a Claim Subject or Claim Reference - which is used to link the credits retire events with real world event.

ditto, this should be part of RFC process

robert-zaremba · 2020-11-04T22:36:53Z

we can definitely consider alternate designs (like #115 or whatever we find is needed by users).

This model is almost compatible with #115 design (with small changes to Msg/Send and Msg/Retire).

clevinson

Few comments on the retiring functionality, and whether users can transfer retired credits with this implementation, or what the purpose is of having retired_units as part of the SendUnits msg.

Happy for these to be resolved in a followup and use this as a starting point though.

clevinson · 2020-11-04T23:01:56Z

proto/regen/ecocredit/v1alpha1/events.proto

+message EventCreateBatch {
+
+    // class_id is the unique ID of credit class.
+    string class_id = 1;


What is the reason that these identifiers (class_id) are string and not integer?

I guess for batch_denom its useful to have as a string, as that's the same type as denoms in the broader cosmos sdk, but for class_id how do you see these ID's being generated? In my head, this would have been an integer identifier, indexed (as in a relational database) and auto-incrementing

An integer converted to a string and so that it's obviously part of denom. We can reassess later

proto/regen/ecocredit/v1alpha1/events.proto

clevinson · 2020-11-04T23:15:53Z

proto/regen/ecocredit/v1alpha1/tx.proto

+
+        // retired_units are the units of credits in this issuance that are effectively retired by the issuer on receipt.
+        // Decimal values are acceptable within the precision returned by Query/Precision.
+        string retired_units = 3;


Seems strange to allow sending of pre-retired credits. IMO retiring should be a separate action done intentionally by the holder of credits.

We already cover the "retire on issuance" use case in the BatchIssuance message, so why is this needed here as well?

Not necessarily. I believe a broker may specifically only sell credits that are retired on sale. We need to discuss with Ron It may or may not be a use case but better to be on the safe side

agreed cf. #125 (comment)

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

Co-authored-by: Cory <cjlevinson@gmail.com>

robert-zaremba

Looks good to move forward. I will collect the questions / suggestion in an issue for review by a science and business team.

aaronc · 2020-11-05T00:06:17Z

Looks good to move forward. I will collect the questions / suggestion in an issue for review by a science and business team.

thanks @robert-zaremba

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

Add proto files

44f810d

docs

a9a3718

blushi reviewed Nov 4, 2020

View reviewed changes

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

blushi reviewed Nov 4, 2020

View reviewed changes

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

Update proto/regen/ecocredit/v1alpha1/tx.proto

c276cf4

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>

update docs

8bec8c2

aaronc requested review from clevinson and blushi November 4, 2020 19:40

aaronc added 5 commits November 4, 2020 15:11

Merge remote-tracking branch 'origin/aaronc/78-credit-module-poc-init…

69c3050

…' into aaronc/78-credit-module-poc-init

docs

3e5778d

Merge branch 'master' of github.com:regen-network/regen-ledger into a…

d756d1f

…aronc/78-credit-module-poc-init

proto-gen

b838256

lint

c957d22

robert-zaremba mentioned this pull request Nov 4, 2020

Balance/Denom credit design #117

Closed

robert-zaremba requested changes Nov 4, 2020

View reviewed changes

clevinson approved these changes Nov 4, 2020

View reviewed changes

aaronc and others added 2 commits November 4, 2020 18:41

Update proto/regen/ecocredit/v1alpha1/tx.proto

f2106ee

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

Update proto/regen/ecocredit/v1alpha1/events.proto

6ab3d09

Co-authored-by: Cory <cjlevinson@gmail.com>

robert-zaremba approved these changes Nov 5, 2020

View reviewed changes

aaronc and others added 2 commits November 4, 2020 19:08

Update proto/regen/ecocredit/v1alpha1/tx.proto

113e704

Co-authored-by: Robert Zaremba <robert@zaremba.ch>

docs

711744c

aaronc merged commit c1ab122 into master Nov 5, 2020

aaronc mentioned this pull request Nov 5, 2020

Eco-Credit Module Proof of Concept #119

Merged

4 tasks

ryanchristo deleted the aaronc/78-credit-module-poc-init branch July 2, 2022 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eco-credit Module Proof of Concept Init #125

Eco-credit Module Proof of Concept Init #125

aaronc commented Nov 4, 2020 •

edited by zube bot

Loading

codecov bot commented Nov 4, 2020 •

edited

Loading

aaronc commented Nov 4, 2020

robert-zaremba Nov 4, 2020

aaronc Nov 4, 2020

robert-zaremba Nov 4, 2020

aaronc Nov 4, 2020

robert-zaremba Nov 4, 2020

robert-zaremba Nov 4, 2020

aaronc Nov 4, 2020

robert-zaremba Nov 4, 2020

aaronc Nov 5, 2020

robert-zaremba Nov 5, 2020

aaronc Nov 5, 2020

robert-zaremba Nov 4, 2020

aaronc Nov 4, 2020

robert-zaremba Nov 4, 2020

aaronc Nov 4, 2020

blushi Nov 5, 2020

robert-zaremba Nov 4, 2020

robert-zaremba Nov 4, 2020

aaronc Nov 4, 2020

robert-zaremba Nov 4, 2020

aaronc Nov 4, 2020

robert-zaremba commented Nov 4, 2020

clevinson left a comment

clevinson Nov 4, 2020

aaronc Nov 4, 2020

clevinson Nov 4, 2020

aaronc Nov 4, 2020

blushi Nov 5, 2020

robert-zaremba left a comment

aaronc commented Nov 5, 2020

Eco-credit Module Proof of Concept Init #125

Eco-credit Module Proof of Concept Init #125

Conversation

aaronc commented Nov 4, 2020 • edited by zube bot Loading

Credit Module Proto Files

Decimal Balance Experiment

codecov bot commented Nov 4, 2020 • edited Loading

Codecov Report

aaronc commented Nov 4, 2020

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

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

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

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

Choose a reason for hiding this comment

robert-zaremba commented Nov 4, 2020

clevinson left a 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

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

aaronc commented Nov 5, 2020

aaronc commented Nov 4, 2020 •

edited by zube bot

Loading

codecov bot commented Nov 4, 2020 •

edited

Loading