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)!: implement independent projects #2167

Open
wants to merge 120 commits into
base: main
Choose a base branch
from

Conversation

aaronc
Copy link
Member

@aaronc aaronc commented Feb 13, 2024

Description

Closes #1313

Review instructions:

  • please check the CHANGELOG and modified .proto files first
  • to review new code, I suggest looking at the new .feature files first
  • changes to existing code have mostly involved removing the assumption that the class is implied by the project and that an enrollment is needed
  • you will need to look at some large diffs that github has hidden by default because they're so big, sorry

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 title feat(x/ecocredit):c independent projects feat(x/ecocredit): implement independent projects Feb 13, 2024
@aaronc
Copy link
Member Author

aaronc commented Jul 9, 2024

@paul121 I believe I have addressed all of the simple changes you have suggested.

Regarding:

There is no way for a credit class to signal that they have "received" an application and are beginning review, maybe give an expected timeframe, etc. I assume this could happen via enrollment metadata.
The credit class can only add metadata when setting the status to ACCEPTED or CHANGES_REQUESTED.
Would it make sense to add a project enrollment status for "PENDING" / "IN REVIEW" so there is an intermediary status?

I think two straightforward ways are either:

  1. allowing metadata to be added by the issuer in UNSPECIFIED state (without transitioning to ACCEPTED or CHANGES_REQUESTED)
  2. or adding a PENDING state like you suggest

Either will involve adding some new test cases, but option 1 seems probably simpler to implement at this stage. Would that be okay or do you think there's a strong argument for a PENDING case? If we add a pending case would we allow transitioning from CHANGES_REQUESTED to PENDING?

@paul121
Copy link
Contributor

paul121 commented Jul 9, 2024

Either will involve adding some new test cases, but option 1 seems probably simpler to implement at this stage. Would that be okay or do you think there's a strong argument for a PENDING case? If we add a pending case would we allow transitioning from CHANGES_REQUESTED to PENDING?

I don't have a strong argument for a PENDING case. I agree option 1 makes sense for now, it's more general and flexible than adding any specific state. Chatting with @clevinson today he said there might be cases when a credit class doesn't want to reveal they are starting a formal review anyways, so yeah, let’s keep it simple for now, allowing metadata in the UNSPECIFIED state.

@aaronc
Copy link
Member Author

aaronc commented Jul 10, 2024

@paul121 I updated UpdateProjectEnrollment to allow transitions from UNSPECIFIED or CHANGES_REQUESTED to UNSPECIFIED with updates to metadata in fd96f71. Please review that commit and let me know what you think.

Copy link
Contributor

@paul121 paul121 left a comment

Choose a reason for hiding this comment

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

@aaronc the changes to UpdateProjectEnrollment look good. Just noticing a test failure related to the CLI changes. Not sure if there are other integration tests to review as well

x/ecocredit/base/client/tx.go Show resolved Hide resolved
Copy link
Contributor

@paul121 paul121 left a comment

Choose a reason for hiding this comment

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

Added a commit to fix one remaining integration test. I think this is good now.

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.

I've gone through all the code and generally looks good.

Handful of small test / comment related suggestions. In general I think we should lean towards using the new project ID's in all of our feature files / comments where possible, unless there's a specific reason to keep the legacy format.

One thing I'm a bit unclear on is how this PR handles creation of new batches for legacy projects (whether or not those batches are under a legacy credit class). By legacy here I simply mean a project or credit class created prior to this upgrade.

In the state migration, it looks like we don't update any existing Project ID's to the new format (as stored in the ORM table). So if a project (C01-001) has a pre-existing batch (C01-001-20210101-20220101-001), and then has a new batch issued after this update, I think the new batch would be C01-C01-001-20220101-20230101-002, since the new formatting of batch denom explicitly adds credit class ID to the beginning, even if project ID is in the legacy format.

Similarly, if this project enrolls in a second credit class, and creates a batch, then that batch would be C02-C01-001-20210101-20220101-001.

I understand why it's preferable to preserve legacy Project IDs and not change them, as that may break compatibility with certain RDF datasets, Sanity, or credit batch linkages. But are there any alternatives where we could maybe keep the legacy format for legacy enrollments, and generate a new format project ID only when a legacy project creates a new enrollment? Another alternative may be just not allowing legacy projects to have multiple enrollments at all.

// ProjectEnrollments queries all credit class enrollments associated with a
// project.
// ProjectEnrollments queries all credit class enrollments and allows for filtering by
// project or credit class.
rpc ProjectEnrollments(QueryProjectEnrollmentsRequest) returns (QueryProjectEnrollmentsResponse) {
option (google.api.http) = {
get : "/regen/ecocredit/v1/project/{project_id}/enrollments"
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way for the GET request to support no project_id, such that it queries all project enrollments? Also, is there a way to optionally filter by class_id in the grpc-gateway path?

Would be nice to accommodate those use cases if possible.

proto/regen/ecocredit/v1/query.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/v1/query.proto Outdated Show resolved Hide resolved
x/ecocredit/base/client/tx.go Outdated Show resolved Hide resolved
Then expect batch sequence with project id "C01-001" and next sequence "1"
Given a batch sequence with project id "P001" and next sequence "1"
When alice attempts to create a batch with project id "P002" and class id "C01"
Then expect batch sequence with project id "P001" and next sequence "1"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Then expect batch sequence with project id "P001" and next sequence "1"
Then expect batch sequence with project id "P002" and next sequence "1"

@@ -33,14 +32,12 @@ func TestQuery_ProjectsByReferenceId(t *testing.T) {
assert.NilError(t, s.stateStore.ProjectTable().Insert(s.ctx, project))
assert.NilError(t, s.stateStore.ProjectTable().Insert(s.ctx, &api.Project{
Id: "C01-002",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Id: "C01-002",
Id: "P002",

Given the project
"""
{
"key": 1,
"id": "foo"
"id": "C01-001"
Copy link
Member

Choose a reason for hiding this comment

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

Can project id's throughout this feature file be updated to the new format (e.g. P001)?

x/ecocredit/migrations/v5/state_test.go Show resolved Hide resolved
Comment on lines +39 to +40
{Id: "C01-001", Admin: sdk.AccAddress("addr1"), Jurisdiction: "AQ", Metadata: "metadata"},
{Id: "C01-002", Admin: sdk.AccAddress("addr2"), Jurisdiction: "AQ", Metadata: "metadata"},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{Id: "C01-001", Admin: sdk.AccAddress("addr1"), Jurisdiction: "AQ", Metadata: "metadata"},
{Id: "C01-002", Admin: sdk.AccAddress("addr2"), Jurisdiction: "AQ", Metadata: "metadata"},
{Id: "P001", Admin: sdk.AccAddress("addr1"), Jurisdiction: "AQ", Metadata: "metadata"},
{Id: "P002", Admin: sdk.AccAddress("addr2"), Jurisdiction: "AQ", Metadata: "metadata"},

Comment on lines -160 to +162
// project with a matching reference id already exists within the scope of the
// credit class, otherwise a new project will be created.
// project with a matching reference id already exists, otherwise a new project
// will be created.
Copy link
Member

Choose a reason for hiding this comment

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

Why remove this text from the comment? Isn't it still true uniqueness of reference_id is only enforced within the scope of a credit class?

@aaronc
Copy link
Member Author

aaronc commented Aug 28, 2024

Here's my proposed solution for generating new batches IDs for existing projects:

  1. if they are emitting a batch in their original credit class, then just use the old batch format, i.e. C01-001...
  2. add an alt_batch_prefix field to Project that only can get set during the migration which gives them some alternate prefix to use if they emit a batch with a new credit class, that batch prefix could be something like L001 with L for legacy to distinguish them from the new P projects

How does that sound?

@clevinson
Copy link
Member

So just to confirm,

If credit legacy project C01-001 registers then with a credit class C02, the first batch under this new credit class would look like C02-L001-20210101-20220101-001 ?

I'm trying to think if there are any queries we should be considering that may have weird or potentially confusing UX with this.

One that comes to mind - when querying for ProjectEnrollments, one would query by the original project ID (eg C01-001) to get a list of all enrolled credit classes, right?

This doesn't seem the prettiest but allowing queries by the alt project id also seems confusing.

The only other option I can think of is preventing legacy projects from enrolling in new credit classes. Personally this path seems cleanest to me, but I'm happy to default to whichever approach you think is best.

Assuming we do want to allow new enrollments from legacy projects, I think the proposal above makes sense and would support it.

@paul121
Copy link
Contributor

paul121 commented Aug 28, 2024

The only other option I can think of is preventing legacy projects from enrolling in new credit classes. Personally this path seems cleanest to me, but I'm happy to default to whichever approach you think is best.

I think I would prefer this as a solution as well.

Assuming we do want to allow new enrollments from legacy projects, I think the proposal above makes sense and would support it.

Agree

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.

Independent Projects
3 participants