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(x/ecocredit): add minimal basket proto #731

Merged
merged 32 commits into from
Feb 9, 2022

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Feb 8, 2022

Description

Minimal basket proto to align with super quick Regen Ledger 2.1 release. To be backported against v2.0.x.


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)

@aaronc aaronc changed the base branch from release/v2.0.x to master February 8, 2022 22:47
@aaronc aaronc changed the base branch from master to release/v2.0.x February 8, 2022 22:47
@aaronc aaronc force-pushed the aaronc/minimal-basket-proto branch from d27f707 to 3358f76 Compare February 8, 2022 22:52
@aaronc aaronc changed the base branch from release/v2.0.x to master February 8, 2022 22:52
@codecov
Copy link

codecov bot commented Feb 8, 2022

Codecov Report

Merging #731 (c339274) into master (63eb64d) will increase coverage by 0.41%.
The diff coverage is 70.22%.

❗ Current head c339274 differs from pull request most recent head 8b9f2c1. Consider uploading reports for the commit 8b9f2c1 to get more accurate results

@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
+ Coverage   74.74%   75.15%   +0.41%     
==========================================
  Files         118      107      -11     
  Lines       19542    14263    -5279     
==========================================
- Hits        14606    10719    -3887     
+ Misses       3963     2905    -1058     
+ Partials      973      639     -334     
Flag Coverage Δ
experimental-codecov 75.29% <70.32%> (+0.45%) ⬆️
stable-codecov 66.78% <70.22%> (-0.91%) ⬇️

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

Copy link
Contributor

@technicallyty technicallyty left a comment

Choose a reason for hiding this comment

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

lgtm, just comment nits

proto/regen/ecocredit/basket/v1alpha1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/tx.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/tx.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/tx.proto Outdated Show resolved Hide resolved
aaronc and others added 10 commits February 8, 2022 18:37
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com>
@aaronc aaronc mentioned this pull request Feb 9, 2022
68 tasks
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.

Looking good!

Few small suggestions/questions that I think we should address before merging. Nothing major.

proto/regen/ecocredit/basket/v1alpha1/state.proto Outdated Show resolved Hide resolved
// a cutoff for batch start dates when adding new credits to the basket.
// Based on the current block timestamp, credits whose start date is before
// block timestamp - batch_date_window will not be allowed into the basket.
google.protobuf.Duration batch_date_window = 8;
Copy link
Member

Choose a reason for hiding this comment

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

Speaking with @glandua, we should also have the option of the recency filter being a fixed date. This is what we will use for the initial basket RND is creating (2012 min vintage start date)

Copy link
Member Author

Choose a reason for hiding this comment

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

I talked with @glandua and we decided to just support fixed start date for now to support to minimal known requirements. Let's add an issue to add support for rolling date windows in the next release.

proto/regen/ecocredit/basket/v1alpha1/tx.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/tx.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1alpha1/types.proto Outdated Show resolved Hide resolved
@aaronc
Copy link
Member Author

aaronc commented Feb 9, 2022

Pushed several updates. Please re-review @clevinson @technicallyty

Copy link
Collaborator

@robert-zaremba robert-zaremba left a comment

Choose a reason for hiding this comment

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

not blocking, left few suggestions. I wonder if we should use basket.id instead of denom in all the mappings.

proto/regen/ecocredit/basket/v1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1/state.proto Show resolved Hide resolved
proto/regen/ecocredit/basket/v1/tx.proto Show resolved Hide resolved
proto/regen/ecocredit/basket/v1/tx.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1/tx.proto Show resolved Hide resolved
Copy link
Member

@ryanchristo ryanchristo left a comment

Choose a reason for hiding this comment

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

Looks good to me. A couple comments but nothing blocking.

proto/regen/ecocredit/basket/v1/state.proto Show resolved Hide resolved
proto/regen/ecocredit/basket/v1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/basket/v1/tx.proto Show resolved Hide resolved
@aaronc aaronc enabled auto-merge (squash) February 9, 2022 18:12
@aaronc
Copy link
Member Author

aaronc commented Feb 9, 2022

auto-merging to unblock the next pieces

@aaronc aaronc merged commit 817130a into master Feb 9, 2022
@aaronc aaronc deleted the aaronc/minimal-basket-proto branch February 9, 2022 18:13
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.

5 participants