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

Store x/ecocredit balances in x/bank and issue credits in kg or grams #407

Closed
aaronc opened this issue Jun 29, 2021 · 8 comments
Closed
Labels
Scope: x/ecocredit Issues that update the x/ecocredit module Status: Nice To Have

Comments

@aaronc
Copy link
Member

aaronc commented Jun 29, 2021

Update: As discussed on July 20th 2021, we decided to keep Decimal support inside the x/ecocredit module. To integrate with x/bank we'll have a precision per credit type (#424) and we'll multiply Decimal values by the precision to create integers to store in x/bank.

In order for credits to be transferable using IBC and/or an existing AMM/liquidity module, they will need to be expressed in integer amounts. Even if we were to add decimals to x/bank, it will be a while before IBC and AMM/liquidity modules support decimals.

So likely the best path forward is to drop decimal support for credits and issue credit batches in either kilograms or grams so there is good fractionalization for buying and selling.

This will mean we don't do #242 and instead postpone that work till its integration in the SDK.

@aaronc aaronc added the Scope: x/ecocredit Issues that update the x/ecocredit module label Jun 29, 2021
@aaronc
Copy link
Member Author

aaronc commented Jun 29, 2021

/cc @clevinson ?

@clevinson
Copy link
Member

This seems like a pretty big refactor. Would it involve passing in the bank Keeper or rather using ADR33... wouldn't the latter also require the minting functionality to be built out in an internal msg server for bank?

I see the direction / reasoning here, but it feels pretty tight if we're trying to have this all done for v1.1 release.

Would you consider an intermediate step where we drop decimal support but keep tracking it in the ecocredit module for now? That seems like the slimmest refactor IMO, and we could then later migrate to the bank module later when ADR33 is ready in bank and release it alongside any AMM/liquidity module at the same time?

@aaronc
Copy link
Member Author

aaronc commented Jun 29, 2021

We would just need to pass in the bank keeper. We could store balances in both bank and ecocredit, but in bank we would need to issue in kg or grams which seems like a weird mental model...

@aaronc
Copy link
Member Author

aaronc commented Jun 29, 2021

It's possible we could issue in tons in ecocredit and grams in bank if we used the precision of the issuance to determine the multiplier. But again that seems like it could be a strange UX and likely we should just issue in grams everywhere...

@robert-zaremba
Copy link
Collaborator

In general I'm in support in integers overall. Number formatting shouldn't be a responsibility of a node. All blockchains are using integers.
We can go for grams. No need to limit ourselves to kg.

@clevinson
Copy link
Member

OK, I'm in favor of moving forward with this approach, but this probably requires a more detailed estimation of how this will affect ecocredit readiness timeline for v1.1.

@clevinson clevinson added this to the v1.1 - Baikal Upgrade milestone Jul 7, 2021
@aaronc
Copy link
Member Author

aaronc commented Jul 7, 2021

OK, I'm in favor of moving forward with this approach, but this probably requires a more detailed estimation of how this will affect ecocredit readiness timeline for v1.1.

My gut feeling on this is that it's a similar level of complexity as #242, which means it wouldn't really alter the timeline. We would drop #242 and replace it with this. Now, if we also want to add a layer of decimal support as kind of a UX enhancement (where 1.000000 in ecocredits gets converted to 1000000 in bank) then it's a bit more complex - I would say the size of this issue + #242.

@aaronc
Copy link
Member Author

aaronc commented Sep 29, 2021

The current thinking is to pause this work and maybe not do it because:

  • generic DEX functionality for coins is seen as a poor fit for eco-credits (see Ability to List/Buy/Sell Ecocredits #505 (comment))
  • moving credits over IBC as coins likely doesn't satisfy most user requirements, specifically if someone wanted to move a credit from a registry chain to a marketplace chain, ICS20 doesn't move any credit metadata which is likely relevant for marketplace functionality

@aaronc aaronc closed this as completed Sep 29, 2021
@ryanchristo ryanchristo removed this from the v4.0 - Llangorse Upgrade milestone Sep 4, 2022
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 Status: Nice To Have
Projects
None yet
Development

No branches or pull requests

5 participants