Skip to content
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

Add a prefix to ecocredit denominations & switch to colon separator #392

Closed
ruhatch opened this issue Jun 18, 2021 · 7 comments · Fixed by #413
Closed

Add a prefix to ecocredit denominations & switch to colon separator #392

ruhatch opened this issue Jun 18, 2021 · 7 comments · Fixed by #413
Assignees
Labels
Scope: x/ecocredit Issues that update the x/ecocredit module Type: Feature New feature or request

Comments

@ruhatch
Copy link
Contributor

ruhatch commented Jun 18, 2021

Right now ecocredit class IDs are alphanumeric, e.g. 18AV53K, and batch denominations are class id, followed by a slash, followed by the batch id, e.g. 18AV53K/1b1vrHF.

This makes it slightly tricky to parse values on the command line of the form <amount><denomination>, so we have had to add a : character to aid parsing. @aaronc has proposed adding a eco: prefix to the denominations to make it easier to parse and, additionally, to make it easier to integrate with x/bank.

Also, for IID/DID compatibility we want to switch from : to / separators, i.e. 18AV53K:1b1vrHF.

@ruhatch ruhatch added Type: Feature New feature or request Scope: x/ecocredit Issues that update the x/ecocredit module labels Jun 18, 2021
@aaronc aaronc added this to the v1.1 - Baikal Upgrade milestone Jun 18, 2021
@aaronc aaronc changed the title Add a prefix to ecocredit denominations Add a prefix to ecocredit denominations & switch to colon separator Jun 30, 2021
@ruhatch ruhatch self-assigned this Jul 12, 2021
@ruhatch
Copy link
Contributor Author

ruhatch commented Jul 23, 2021

So in Regen Slack @aaronc proposed the following:

Here’s what I’m currently thinking for credit batch serial numbers: type-program-class-project-dates-batch , where type, program, class, project and batch are all base10 or hex encoded unique IDs padded with zeros (the current base58 with slashes doesn’t look good). Ex: C01-01-001-20190101-20191231-1 for type C (carbon), program 1, class 1, project 1, 2019 vintage, batch 1. Program in this terminology refers to a specific registry program and is about the top-level governance.

We're unclear if we'll decide about projects and programs before v2.0, but we could at least implement type-class-dates-batch, with what we currently have. This would put in place some of the generation and parsing code for these serial numbers, making it fairly easy to add programs, projects or other entities when we're ready.

I'd also propose we keep the eco prefix or something similar, maybe ECO, so we'd end up with:

ECO-C001-20190101-20191231-01

@aaronc @clevinson any thoughts about doing this for v2.0?

@ruhatch
Copy link
Contributor Author

ruhatch commented Jul 28, 2021

In the Regen Ledger WG call we decided to do this as C001-20190101-20191231-01 without the prefix and to add the prefix ueco: when we're integrating with x/bank.

@ruhatch
Copy link
Contributor Author

ruhatch commented Jul 30, 2021

So, a few things come up while implementing this.

  1. Should we change ClassIDs?

Right now these are Base58-encoded strings of the sequence number of the class. So the batch would have this serial number like thing, but the class would have something like 1xkjsd5d. Should we change ClassID to be the prefix of the batch id, like C001 or something like that?

  1. Right now we're using a single sequence number for ClassID and BatchID

I think it might be better to have a sequence number per class, so we get incrementing batch ids in the class, rather than them incrementing together. We could even take this further and have a sequence number for each credit type, which means we can support more denominations over all. So you'd have e.g. C001 and B001, rather than C001 then B002 or something.

  1. What units shall we use in the ecocredit command-line?

We're using Dec in x/ecocredit, but we would translate to integers in x/bank. So in bank someone would use, e.g. 10000000ueco:C001-20190101-20200101-002 to refer to 10 credits in the batch.

In the cancel command and other x/ecocredit commands, we could just use eco: and decimals, or we could offer both ueco: and eco:. Thoughts?

@clevinson @aaronc @blushi

@aaronc
Copy link
Member

aaronc commented Jul 30, 2021

So, a few things come up while implementing this.

  1. Should we change ClassIDs?

Right now these are Base58-encoded strings of the sequence number of the class. So the batch would have this serial number like thing, but the class would have something like 1xkjsd5d. Should we change ClassID to be the prefix of the batch id, like C001 or something like that?

Yes 👍

  1. Right now we're using a single sequence number for ClassID and BatchID

I think it might be better to have a sequence number per class, so we get incrementing batch ids in the class, rather than them incrementing together. We could even take this further and have a sequence number for each credit type, which means we can support more denominations over all. So you'd have e.g. C001 and B001, rather than C001 then B002 or something.

Not sure I understand 🤔

  1. What units shall we use in the ecocredit command-line?

We're using Dec in x/ecocredit, but we would translate to integers in x/bank. So in bank someone would use, e.g. 10000000ueco:C001-20190101-20200101-002 to refer to 10 credits in the batch.

In the cancel command and other x/ecocredit commands, we could just use eco: and decimals, or we could offer both ueco: and eco:. Thoughts?

I was thinking in x/ecocredit we would accept without eco: for now. And just use those within bank. Does that seem acceptable? (Side note: we really need to document the idea of converting denom Metadata -> virtual denoms in the SDK somewhere and put in in the x/bank roadmap)

@ruhatch
Copy link
Contributor Author

ruhatch commented Jul 30, 2021

@aaronc

Not sure I understand 🤔

So right now there's a single sequence number in the ecocredit module called IdSeq. If we created a class, then a batch, then another class, we'd get class 1, batch 2, and then class 3. If we have 100 classes, each with 2 batches, then we created a new batch, it'd have ID 201. Does that make sense?

I'd propose that we have one sequence number for classes, and then a sequence number per-class for it's batches. So then we can identify e.g. batches 1 and 2 of the 100th class. As C100-20190101-20200101-01 and C100-201901001-20200101-02. Then we don't have this odd property of interleaving class and batch IDs.

I was thinking in x/ecocredit we would accept without eco: for now. And just use those within bank. Does that seem acceptable? (Side note: we really need to document the idea of converting denom Metadata -> virtual denoms in the SDK somewhere and put in in the x/bank roadmap)

Yeah, could do it like that, although 1000C001-20190101... looks a little confusing. Adding eco: makes it easier to parse visually (1000eco:C001-20190101...) and makes it more consistent with x/bank. Do we lose anything by adding the eco: prefix in x/ecocredit commands?

@ruhatch
Copy link
Contributor Author

ruhatch commented Jul 30, 2021

@clevinson and I just discussed a little and thought something like:

regen tx ecocredit cancel '1000 C01-20190101-20201010-02'

with a space might work well. Could then do multiple args with commas, or just as multiple string arguments.

@aaronc
Copy link
Member

aaronc commented Jul 30, 2021

That all makes sense to me 👍

clevinson pushed a commit that referenced this issue Aug 12, 2021
* feat: Add prefix and use colon separator in batch denoms (#392)

* Update class ID format and add space in CLI credit parsing

* Add sequence numbers for every credit class

* Use credit type abbreviations in batch denoms

* Add sequence numbers per credit type

* Address review comments

* Update tests after merge and add to CHANGELOG

* Add tests for invalid class IDs and batch denoms

Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Scope: x/ecocredit Issues that update the x/ecocredit module Type: Feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants