Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Eco-credit Module Proof of Concept Init #125
Eco-credit Module Proof of Concept Init #125
Changes from all commits
44f810d
a9a3718
c276cf4
8bec8c2
69c3050
3e5778d
d756d1f
b838256
c957d22
f2106ee
6ab3d09
113e704
711744c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 forclass_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-incrementingThere 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.
An integer converted to a string and so that it's obviously part of denom. We can reassess later
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.
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 notbatch_id
? I know that we usedenom
for an asset name, but here it's meaning (without a comment) is not very clear.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.
Yep I can add some clarifying language on denominations
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.
why do we use
units
(here and below)? I think the termamount
is coined almost everywhere where we talk about tokens.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.
for now I'm trying to stay consistent with the RFC that uses the term
units
and went through a review process. I'd rather this be addressed in the RFC PR #107There 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.
There is only one
issuer
, so we don't need to code it here. In Ethereum, the standard is to putsender=0
(zero value) for minting / issuance transfers. Maybe we can use the same thing here (sender = ""
)?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.
Other idea, instead of
oneof
would be to add a boolean flag here.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 don't have to use oneof true, but they are mutually exclusive. I can just make
sender = ""
if that's preferredThere 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.
The whole thing is to eliminate the
oneof
and normalize the data in the RDBMS sense (so to not use theissuer
field at all here). Adding a boolean flag is only cosmetic if we want to be explicit.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.
well i'm trying to distinguish between issuer and sender as roles...
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 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.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 I'm removing and just leaving
sender = ""
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.
ok, so I think this answers my comment above:
batch_denom
is unique globally.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.
yep
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 should also add a Claim Subject or Claim Reference - which is used to link the credits retire events with real world event.
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.
ditto, this should be part of RFC process
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.
Shouldn't all assets from the same asset class have the same precision? If yes, then it's better to have a compact info about the asset and precision
ClassInfo
instead of having this method.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.
not necessarily, they correspond to different projects which have wildly different sizes
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.
Two things to consider:
Ad 1. Natural numbers are very fast and they works. There are very good libraries for doing 128-bit arithmetic. 128 bit max value is
3.4e38
. And even if we will have 1e9 denomination (like in bitcoin), we could easily multiply numbers up to 1e19 (1e10 denominated in 1e9) without doing any conversion. Otherwise we can easily convert tobig.Int
for more complex operations.Ad 2. Having fixed decimals works very well with natural numbers and tooling. All tools will know that they need to put a comma at a certain position, without querying an asset. We could still limit the tradeable amounts by defining
granularity
- this is what ERC777 is doing (which is also implemented by OpenZeppelin).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.
Under the hood this is basically
big.Int
. It simply allows configurable granularity so that doesn't need to be decided up front. It would be good to have these discussions in the context of cosmos/cosmos-sdk#7113. This is simply intended to show a viable PoC pathway towards thatThere 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.
Do you think that there will be an
url
string in all classes metedata? If some, maybe it's worth to lift it up?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.
likely, but here I'm trying to postpone dealing with data formats and just deal with asset mechanics. I'm assuming people will put some json here so this basically is the decision to start schema-less and iterate
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.
There is one very important field in all issuance requests: a certificate. I would prefer to have it in the top scope, rather hidden in metadata. Certificate is basically a string with url or a proof. Probably we should even formalize it - and have additional service which will be able to prove the certificates.
At minimum, I would add a comment with TODO to consider it.
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.
let's open a separate issue to discuss metadata. it's a big topic that I'd rather specify off-chain first and then put on-chain. we would need to discuss with Ron on the customer side
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.
Why issuing retired credits? We can keep it for flexibility, but I'm curious if this makes sense.
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.
Also, shouldn't we use
oneof
for tradeable and retired units / credits?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.
It's a fundamental part of the use case. It's not mutually exclusive either. You could issue both. In some cases it is desirable or even required to issue credits as retired (i.e. non-tradable) on creation. This is more a business question, happy to share more context at some point
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 above, retired units are not tradeable - so doesn't it make sense to send it over?
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.
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
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.
Indeed, there are cases where sold credits should be "auto-retired".
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.
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?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.
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
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.
agreed cf. #125 (comment)
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.
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)
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.
related to the comment above - let's consider adding a
certificate
field.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.
again let's discuss metadata elsewhere. we don't have context on the engineering team to make these sorts of decisions