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

feat!: ecocredit genesis import and export #389

Merged
merged 58 commits into from
Aug 13, 2021

Conversation

aleem1314
Copy link
Contributor

@aleem1314 aleem1314 commented Jun 17, 2021

Description

Closes: #187


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...

  • included the correct type prefix in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • updated the relevant documentation or specification
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

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
Copy link

codecov bot commented Jun 17, 2021

Codecov Report

Merging #389 (c4ea83b) into master (2ddd722) will decrease coverage by 0.01%.
The diff coverage is 77.37%.

@@            Coverage Diff             @@
##           master     #389      +/-   ##
==========================================
- Coverage   76.96%   76.94%   -0.02%     
==========================================
  Files          99      101       +2     
  Lines       12111    12520     +409     
==========================================
+ Hits         9321     9634     +313     
- Misses       2230     2286      +56     
- Partials      560      600      +40     
Flag Coverage Δ
experimental-codecov 76.94% <77.37%> (-0.02%) ⬇️
stable-codecov 59.01% <75.05%> (+0.75%) ⬆️

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

@aleem1314 aleem1314 changed the title feat: ecocredit genesis import and export feat!: ecocredit genesis import and export Jun 19, 2021
@aleem1314 aleem1314 requested a review from blushi June 21, 2021 04:55
@aleem1314 aleem1314 requested review from ruhatch and aaronc June 21, 2021 10:30
@aleem1314 aleem1314 marked this pull request as ready for review June 21, 2021 12:23
@aleem1314 aleem1314 requested a review from amaury1093 June 21, 2021 12:24
@cyberbono3 cyberbono3 self-requested a review June 23, 2021 09:40
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked at the implementation yet, just some comments regarding the proto definitions

proto/regen/ecocredit/v1alpha1/genesis.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/v1alpha1/genesis.proto Outdated Show resolved Hide resolved
aleem1314 and others added 2 commits June 23, 2021 17:13
Co-authored-by: Marie Gauthier <marie.gauthier63@gmail.com>
@aleem1314
Copy link
Contributor Author

@aaronc could you have a final look?

@aleem1314 aleem1314 mentioned this pull request Jul 29, 2021
Copy link
Member

@aaronc aaronc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logic looks correct now.

Can we simplify this API? Won't block on it because I know this has been in progress for a while, so pre-approving. But I think we can merge tradable and retired balances and supplies.

proto/regen/ecocredit/v1alpha1/genesis.proto Outdated Show resolved Hide resolved
@clevinson clevinson self-requested a review August 10, 2021 20:02
Copy link
Member

@clevinson clevinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few comments on the formatting of the Supply / Balance messages. Can you take a look at this tomorrow @aleem1314 and then I think we're good to merge!

proto/regen/ecocredit/v1alpha1/genesis.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/v1alpha1/genesis.proto Outdated Show resolved Hide resolved
import "regen/ecocredit/v1alpha1/types.proto";
import "gogoproto/gogo.proto";

option go_package = "github.com/regen-network/regen-ledger/x/ecocredit";
Copy link
Collaborator

@robert-zaremba robert-zaremba Aug 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was posting similar comment to NFT: how about putting genesis into a separate package, eg x/ecocredit/genesis - it has different domain than RPC messages. Or even we can have two packages:

  • x/ecocredit/ -- for all things related to RPC
  • x/ecocredit/dal -- for all things related to storage (data access layer)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with this separation in principle, but don't want to block this PR on it. adding a new versioning scheme outside of the one for just modules (which is the current setup) is something that I'd like to make sure is the direction we want to go in across the SDK before doing it just on our own modules.

We will already need to be doing a version upgrade for ecocredit module proto files in regen ledger v3 as we expect the API to change possibly quite a bit. So seems fine IMO to not block the v2.0 upgrade on this question.

@aleem1314 aleem1314 requested a review from clevinson August 11, 2021 16:20
Copy link
Member

@clevinson clevinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few merge conflicts now, but pending those getting fixed, I think this is good to go.

There's some stuff where we may want to add some additional validation to InitGenesis, namely that all the CreditType values in any given genesis credit classes actually correspond to a CreditType in the ecocredit's genesis params.

Happy to move that to a separate issue and just continue a discussion there to see what makes the most sense after this PR gets merged.

Thanks @aleem1314!

@aleem1314 aleem1314 enabled auto-merge (squash) August 13, 2021 15:36
@clevinson clevinson disabled auto-merge August 13, 2021 16:51
@clevinson clevinson merged commit a06e533 into master Aug 13, 2021
@clevinson clevinson deleted the aleem/187-ecocredit-genesis branch August 13, 2021 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x/ecocredit Genesis Import and Export
9 participants