feat(x/ecocredit): independent project API #2150
Conversation
|
Great work @aaronc ! Overall I think this looks good and it's exciting to see an independent projects PR. The key assumptions you made in the design are:
For the most part I agree with this framework, but I am curious about your reasoning for why they must specify the My other question around this is, what happens if an application is rejected. I think we want to allow for multiple applications on the same project, as a rejection doesn't always indicate that the project doens't meet the criteria. Rather it might be incomplete from a reporting standpoint or too premature. If the Again, great job. |
|
Thanks for the review @S4mmyb. Would it make sense to allow any issuer to request changes? Should there be some application period whereby any issuer can block an approval if they do so before say a 2 week time period? Also should we consider allowing multiple credit classes per project? |
|
The more I think about it, the more I think we should simply allow a project to be enrolled in multiple credit classes simultaneously. This would subsume the need for a separate API for pre-financing as in #2152 since these could all be modeled as credit classes themselves. It would also allow reuse of the credit class infrastructure including the anti-spam fee and basketing. Also, we've heard of potential cases of credit stacking. The main argument for not allowing multiple credit class enrollment seems to be more of an argument against stacking which although maybe correct is likely not something we should encode ideologically at the protocol call especially since pre-financing is a valid use case. |
I think we should have an |
|
So what if in the credit class project relationship, an issuer can at any time change the status between approved, changes requested and rejected? Also a project could unilaterally withdrawal from a credit class relationship. |
There was a problem hiding this comment.
Generally looks good. Excited to see how simple this is (at least at the proto level).
I (similarly to sam) think projects should apply just to the credit class (not a specific issuer), though how we handle the application review process & permissioning may make that a bit difficult (e.g. multiple issuers coordinating on a review... see my in-line comment below).
I'm also in favor of a project enrolling in multiple credit classes simultaneously. If we wanted any on-chain enforcement to restrict projects being enrolled in multiple credit classes, we probably only do that for credit classes of the same type. In other words, a project should not be able to be enrolled in two credit classes of the same type during the same time period.
proto/regen/ecocredit/v1/state.proto
Outdated
| // issuer is the issuer of the credit class which the application has been | ||
| // submitted to. | ||
| string issuer = 4; |
There was a problem hiding this comment.
I agree with @S4mmyb's feedback that we maybe don't need to specify the issuer as part of an application submission. I think in our existing use cases, we mostly have 1 issuer / registry agent org per credit class. If cases arise where we may have multiple issuers for a credit class, then I think it's safe to assume that they are sufficiently coordinated (seeing that all issuers have equal rights to issue credit batches on any project).
I'd prefer projects just apply to a credit class, and any issuer can pick it up.
One thing that we still need to talk through is whether a application review process (once initiated by an issuer), has to be fully completed by that same issuer, or if any issuer can change application status of any project application at any point in time.
Chatting w/ @S4mmyb briefly about this, I get the sense he prefers the former approach. So an "application review cycle" once opened by a single issuer, is owned by that issuer until the cycle completes.
There was a problem hiding this comment.
I think the simpler model is to assume that credit class issuers are sufficiently coordinated. What if an issuer were removed? Then this means that the project is stuck in an unreconcilable state.
proto/regen/ecocredit/v1/state.proto
Outdated
|
|
||
| // Application represents the evaluation status that a credit class issuer | ||
| // assigns to a credit class application. | ||
| enum ApplicationStatus { |
There was a problem hiding this comment.
So possibly we should add an APPLICATION_STATUS_ASSIGNED if we are treating applications as a first come first serve by the issuers?
Though in seeing this, I'm wondering if it overcomplicates the process and if we would be better off just choosing either:
- projects apply to a specific issuer
- project applies to the credit class, and any issuer can change the status at any point in time (e.g. issuer A can set it as 'change requested' and issuer B can later accept the project)
There was a problem hiding this comment.
Currently I'm leaning towards the latter for simplicity
proto/regen/ecocredit/v1/tx.proto
Outdated
| } | ||
|
|
||
| // MsgSubmitClassApplication is the Msg/SubmitClassApplication request type. | ||
| message MsgSubmitClassApplication { |
There was a problem hiding this comment.
Can we add a field representing the project term? In the context of a credit class, I believe a project will always have a defined project term (start date & end date), and it is then expected that the registry agent will issue credit batches for each vintage year during that project term.
Currently I don't think projects encode this data on-chain, but it might be useful to do if we want to restrict projects from enrolling from two credit classes of the same credit type during the same time period.
There was a problem hiding this comment.
Not sure this is well enough specified to include on chain yet. @S4mmyb ?
|
Makes sense and I agree with uniqueness around credit type. I'll update this PR to reflect that. |
|
I've updated this design based on the comments above and tried to go for as simple as possible. Would appreciate another review @clevinson @S4mmyb or maybe we chat live to resolve any remaining design issues. |
There was a problem hiding this comment.
Generally looks great. Only concrete suggestions are:
- changing
ProjectClassname toProjectEnrollmentorProjectClassEnrollment - similarly, where it says "project class relationship" I think it should instead say "project class enrollment" or "project's enrolled in credit class"
|
Few more general reflections after my call with @aaronc earlier today. There are 2 API design choices that IMO are OK for us to move forward with in their current form, but probably warrant a bit more conversation w/ @S4mmyb to make sure we are making the best choice that satisfies most of our design concerns while keeping implementation simple. Project Enrollment Statuses (& Right to Withdraw)Key questions:
Project Terms@S4mmyb Do you think we have a clear enough idea of the requirements of "Project Terms" that we should incorporate that into this PR? From my understanding, projects always have a defined term (project start date & end date), so it might make sense to add those to this PR as part of a "project enrollment". This way, when each project applies to a credit class it has a specified project term in advance. We could use this project term in a few ways:
FeesProbably for a future design, but we shouldn't forget that at some point we'd like to incorporate fixed fees into the project registration flow. I know Gregory has expressed interest in future implementations that support bonding / locking REGEN when enrolling a project in a credit class, and that REGEN being burned / slashed if a project prematurely terminates. @aaronc mentioned on our call that its probably best to wait until we have an MVP implemented and some actual usage before we put in more UX hurdles such on-chain / bonded REGEN fees. |
Co-authored-by: Cory <cjlevinson@gmail.com>
Co-authored-by: Cory <cjlevinson@gmail.com>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #2150 +/- ##
==========================================
- Coverage 73.11% 72.90% -0.22%
==========================================
Files 236 240 +4
Lines 13820 13860 +40
==========================================
Hits 10105 10105
- Misses 2976 3016 +40
Partials 739 739
|
|
@clevinson based on my discussion with @S4mmyb we decided that a project developer can only create, update or withdraw an application but cannot unilaterally withdraw from a credit class on chain. They need to coordinate this off-chain with the credit class issuer and then the credit class issuer can terminate. So I've added a TERMINATED status and made withdrawal only relevant to an application. I've also simplified and renamed the API so that there are now only two methods for enrollments:
These two methods have two corresponding events. We also agreed to not worry about checking enrollment dates or even double enrollment in two classes of the same credit type at all. We're creating transparency here, rather than trying to orchestrate some rule system that isn't fully defined yet. Generally, this current design feels pretty simple and flexible. From my conversation with @S4mmyb yesterday, it sounds like this general direction is good enough so I might begin some implementation work on the side. When you get a chance @clevinson and @S4mmyb, would appreciate another round of reviews so we can iron out any details and approve if this is ready. |
|
Although I said that I think more complex fees could be added later (if needed), I did add a governance-controlled project creation fee which seems important for spam prevention. |
There was a problem hiding this comment.
Overall this looks good. I don't have many opinions about the project applications and enrollments as I'm not as familiar with how these processes work today. But the concepts here make sense, and agree with making applications to a credit class generally and not specific issuers.
One thing that I don't see mentioned above - would it be desirable for credit classes to have a status of whether they are accepting applications for new projects? Perhaps credit classes should not be able to deny creation of applications to their credit class (promote open participation). But there may be scenarios when a credit class is no longer "active" or has been replaced by a new credit class that should be used instead.
Otherwise I'm more concerned with the technical changes and how this might affect existing credit classes and projects.
- What is happening to project IDs? And consistency/inconsistency of project IDs in batch IDs?
- This introduces the concept of "project termination" from a credit class via the project enrollment. But projects that are created directly to a credit class will not have an enrollment created, thus credit class issuers would not be able to "terminate" these projects, until an application/enrollment is created by the project admin. This would also affect existing projects on-chain today, but could potentially be updated in a migration.
| // MsgCreateOrUpdateApplication is the Msg/CreateOrUpdateApplication request type. | ||
| message MsgCreateOrUpdateApplication { |
There was a problem hiding this comment.
Why only one message for create and update? What fields can be updated after creation? This is different than Projects that have separate messages for creation and updating individual fields. These message make it very clear what can and cannot be updated. Their corresponding events also make it simple to subscribe to changes.
For applications it seems like only two fields should be updated: metadata and withdraw. It seems like you would make a new application instead of updating the project ID or credit class on an existing application. And for withdrawing, it isn't clear if that value can be updated multiple times. If you withdraw an application, would it make sense to "lock" the application state from any future updates and instead require creating a new application?
In this case, messages could be simplified to:
- MsgCreateApplication
- MsgUpdateApplicationMetadata
- MsgWithdrawApplication
There was a problem hiding this comment.
The main rationale is that there's a fair amount of ceremony for every new method - separate messages, validation, tests, etc. So I can split it up if the single method is overly complex. It's just a little more annoying for implementation.
Withdrawing would delete the application and you'd need to re-apply.
There was a problem hiding this comment.
Yeah, makes sense re: implementation. Lately I have been working on a couple projects that interact with ecocredit projects + credit class, and must say the I like the separate messages. It's somewhat self-documenting and seems consistent within the ecocredit module.
I think it works well because most of the time we are only updating project/credit class metadata. If we were frequently updating multiple fields then it would make sense to include in a single message. But in this case, too, it seems like metadata would be the most frequent update. A dedicated message for withdrawing wouldn't be bad imo.
There was a problem hiding this comment.
Okay I can make separate messages. Not that much harder
There was a problem hiding this comment.
Is one message okay for update enrollment?
There was a problem hiding this comment.
Hmm yeah one message for update enrollment makes more sense. It would be common to update both the metadata and status at the same time. Although this would be the only ecocredit entity without its own dedicated update metadata msg 🤷
There was a problem hiding this comment.
In looking into separating create and update, it honestly feels like too much code duplication to have a separate messages here since it will be the exact same fields in each message, the exact same validation and even the exact same keeper logic. The only different logic would be for withdraw, although I think that would also share all the same validation and most of the logic. So I'm inclined to either keep it as or just separate withdraw into its own message. I understand other operations in x/ecocredit are separated, but in those cases the arguments really are different enough that each message would have different validaton and substantially different logic. But that isn't the case here - it's very simple and almost identical. Maybe when you do your review @clevinson, you can opine on the direction.
There was a problem hiding this comment.
I think of the two options, separating out withdraw into it's own message makes the most sense. Like @paul121 said, the only two things we would really update are the metadata and withdraw. Having a separate withdraw message makes sense to me. Having a separate update metadata message I'm not sure about, mainly because I I don't know how often we would actually update application metadata or what that process would look like. If an issuer requested changes made to the application, would the project admin do we think it's important to have the issuer update the metadata to reflect the revised / new evidence?
There was a problem hiding this comment.
Having a separate update metadata message I'm not sure about, mainly because I I don't know how often we would actually update application metadata or what that process would look like. If an issuer requested changes made to the application, would the project admin do we think it's important to have the issuer update the metadata to reflect the revised / new evidence?
My understanding: When an issuer requests changes to an application I would expect the issuer to update the Enrollement status & metadata. I would then expect the project admin to update the Application metadata to address the requested changes. How revisions are represented is outside of the scope here, but could be included within the metadata content.
| // MsgCreateUnregisteredProjectResponse is the Msg/CreateUnregisteredProject response type. | ||
| message MsgCreateUnregisteredProjectResponse { | ||
| // project_id is the unique identifier of the project. | ||
| string project_id = 1; | ||
| } |
There was a problem hiding this comment.
How will project IDs be generated? Will they be auto-incrementing IDs unique across all credit classes?
There was a problem hiding this comment.
Auto-increment across all projects independent of credit class.
There was a problem hiding this comment.
Once a project registers under a credit class, does the project-id change to adopt the credit class project_id? What happens to the projct_id when the project de-registers from a credit class, or if it is enrolled in multiple credit classes?
There was a problem hiding this comment.
No, all project IDs would be stable, unique and uncorrelated with credit classes
@S4mmyb @clevinson any thoughts here?
If it's a new project ID, ex. For existing project IDs, say
Actually, we'd have a migration for existing credit classes to create the enrollment record. |
|
I've clarified that whenever an application is withdrawn or status is set to rejected or terminated, that the data is deleted from state allowing the project to apply again to the same credit class in the future. Let me know if this is undesirable for any reason. I know we had discussed keeping the on-chain state for terminated projects @S4mmyb, but in this case, we would need to allow issuers to set the state backed to accepted in order for the project to re-enroll. It seems simpler to just delete and then allow reapplication in the case where projects are terminated which would make the re-enrollment behavior uniform across withdrawn, rejected and terminated states. |
There was a problem hiding this comment.
Overall I think this looks good to me. I agree with @paul121 's comments about how updating project-ids will happen as projects enter/exit credit classes, or are enrolled in multiple credit classes. For instance, if I have an independent project with id P002 then enroll in Credit Class C08 as the 1st project of that credit class, would the id change to C08-001? What happens if I unenroll at a later point in time? Does it go back to being P002 or is it reassigned a new id based on autoincrementing? Also do we reserve the C08-001 id and just say that that project has been terminated? I have similar questions around id assignment for multiple enrollment.
Another question I had was who sets the governance-gated project-creation fee. To me this makes sense that it happens at the credit class level as the application process / overhead might differ depending on the nature of the credit class. Is that the intent? Would that be the same thing, or different from a burn fee?
One thing that I don't see mentioned above - would it be desirable for credit classes to have a status of whether they are accepting applications for new projects? Perhaps credit classes should not be able to deny creation of applications to their credit class (promote open participation). But there may be scenarios when a credit class is no longer "active" or has been replaced by a new credit class that should be used instead.
Great question @paul121. In general I think it makes sense to allow credit classes to have open vs closed enrollment, but I feel like at this point in time the real-world exploration of what it means to govern a credit class is too premature to really know how we should implment that. Would the admin set it? Could the registry set it? What happens if someone wants to take a deactivated credit class out of retirement? That said, it would be pretty simple to just add an open/closed field set by the credit class admin that would allow applications, and if it is maybe we just go for it.
|
|
||
| // enrollment_metadata is any arbitrary metadata set by the credit class | ||
| // admin evaluating the project's application to the credit class. | ||
| string enrollment_metadata = 6; |
There was a problem hiding this comment.
maybe this would be more aptly named evaluation_metadata?
| // MsgCreateUnregisteredProjectResponse is the Msg/CreateUnregisteredProject response type. | ||
| message MsgCreateUnregisteredProjectResponse { | ||
| // project_id is the unique identifier of the project. | ||
| string project_id = 1; | ||
| } |
There was a problem hiding this comment.
Once a project registers under a credit class, does the project-id change to adopt the credit class project_id? What happens to the projct_id when the project de-registers from a credit class, or if it is enrolled in multiple credit classes?
| // MsgCreateOrUpdateApplication is the Msg/CreateOrUpdateApplication request type. | ||
| message MsgCreateOrUpdateApplication { |
There was a problem hiding this comment.
I think of the two options, separating out withdraw into it's own message makes the most sense. Like @paul121 said, the only two things we would really update are the metadata and withdraw. Having a separate withdraw message makes sense to me. Having a separate update metadata message I'm not sure about, mainly because I I don't know how often we would actually update application metadata or what that process would look like. If an issuer requested changes made to the application, would the project admin do we think it's important to have the issuer update the metadata to reflect the revised / new evidence?
No, it would always be
I think there's some misunderstanding here. Project creation in this model happens independently from applying to credit classes. If we want an application fee for credit classes, that's a different consideration and I'm not sure we need to put that on chain right now. I'm just suggesting some small anti-spam fee (maybe ~$10USD) so people don't create tons of garbage projects. This fee would be set by chain governance just like the credit class creation fee.
We could have a boolean |
There was a problem hiding this comment.
Lgtm! Approving with a few minor suggestions, but hoping @aaronc can just make the final call with this and others' input in mind.
The one thing I'll add about the separating out of MsgCreateOrUpdateApplication is that it's not totally clear from documentation how the user should set the metadata field when they are using this msg to withdraw an application. I guess they should leave metadata blank?
With this confusion, I do have a preference for us to have a separate explicit msg for withdraw. As for the other actions (create & update), I think both strategies are fine, and don't have a strong preference as to whether they are consolidated or have separate Msg's.
|
I was assuming that withdrawal would allow metadata and that this would get passed onto the event. In that case do separate withdrawal and create/update messages still make sense? Like I said the arguments and validation would otherwise be 100% the same including metadata although we could remove the awkward withdraw bool. |
Co-authored-by: Cory <cjlevinson@gmail.com>
|
I'd like to merge this as is without adding a separate withdraw method. Each new method results in > 200 LOC when we include tests and all and with limited resources, maintainability and correctness are big concerns. Reducing implementation complexity can help with that even though I acknowledge it makes the resulting API a bit more complex. |
Description
Closes: #1323 and replaces #1422.
Based on the current way in which project pages are evaluating in app.regen.network, there is a need for an on-chain representation of projects before they have been admitted into credit classes. This PR introduces an application system for a project to enter into a credit class.
The key assumptions this design makes are:
This is intended to be done in a non API breaking way.
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers 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...
!in the type prefix if API or client breaking change