feat(x/ecocredit): update batch proto to support dynamic minting #937
Conversation
robert-zaremba
added
the
Scope: x/ecocredit
|
The name of the branch and the title of the pull request are |
|
Can we reduce this PR to just updated proto files (no codegen files, and no stubs implementing interfaces, etc.)? I think if its only a PR with proto files for now then it will be a great companion to the #924 issue so we can talk concretely about what an implementation hypothetically would look like. |
clevinson
changed the title
robert-zaremba
changed the title
robert-zaremba
force-pushed
the
robert/basket-mint
branch
from
3655be8
to
f846260
Compare
robert-zaremba
changed the title
|
Reducing this PR only to proto updates. |
robert-zaremba
force-pushed
the
robert/basket-mint
branch
from
f846260
to
19e9506
Compare
Codecov Report
@@ Coverage Diff @@
## master #937 +/- ##
==========================================
+ Coverage 72.97% 73.00% +0.02%
==========================================
Files 206 210 +4
Lines 23350 23363 +13
==========================================
+ Hits 17039 17055 +16
+ Misses 4968 4951 -17
- Partials 1343 1357 +14
Flags with carried forward coverage won't be shown. Click here to find out more. |
| // enable_future_minting tells if it's possible to mint new credits in | ||
| // the future. Once the enable_future_minting is set to false, it can't be | ||
| // toggled any more. | ||
| bool enable_future_minting = 8; | ||
| } |
There was a problem hiding this comment.
if we have enable_future_minting, max_supply is not needed any more. so I removed max_supply.
There was a problem hiding this comment.
I actually think the default should be sealed. So if we want a different name let's use open
There was a problem hiding this comment.
It's in the state, so we control the defaults. Good naming is a challenge.
If we want to rename this, then let's use open everywhere.
| // the id of a transaction based on a type (tx id, serial number) | ||
| string id = 2; | ||
| } | ||
| } |
There was a problem hiding this comment.
Both, the note and origin_tx will service the accounting purpose.
Co-authored-by: Aaron Craelius <aaron@regen.network>
|
| string note = 3; | ||
|
|
||
| // batch_denom is the unique ID of the credit batch. | ||
| string batch_denom = 4; |
There was a problem hiding this comment.
Normally I would prefer to not storing everything. We have events to keep that information. But events are not guaranteed to be permanent.
So ideally we don't store everything here and we sort out the events in SDK.
What do you think?
There was a problem hiding this comment.
in fact, since I'm adding this fields here, I've slimmed down the EventMintBatchCredits event
|
When writing the proto API for new features, we should either include dummy implementation methods or comment out any new rpc methods that will cause tests to fail after generating the proto files. Ideally changes to the existing API would not be made in the same pull request, which would prevent the need to update any code in the existing implementation. In cases like this where A potential process for designing new features starting with proto API updates:
|
ryanchristo
changed the title
ryanchristo
removed
the
Scope: x/ecocredit
proto/regen/ecocredit/v1/tx.proto
Outdated
| // open=true keeps the batch unlocked for future minting. | ||
| // Othrwise we will seal the batch after the mint. | ||
| // Throws an error if the batch is already sealed. | ||
| bool open = 4; |
There was a problem hiding this comment.
Could we update this to actually be a separate message? I think in most use cases (thinking of bridges), the issuer won't actually ever want to seal the batch. Adding this here requires the user to explicitly set this value every time they call MsgMintBatchCredits, and if they forget, then it would lock the batch and prevent the issuer from being able to issue more credits to that vintage (assuming we have the appropriate restrictions on project <> credit type duplicate issuances).
I think I'd rather see something like:
message MsgSealBatch {
string issuer = 1;
}There was a problem hiding this comment.
@aaronc I know you had initially proposed this bool open = 4; what do you think of this upgrade?
There was a problem hiding this comment.
I don't think it makes that big of a difference. This should all be done automatically anyway. But I don't object
There was a problem hiding this comment.
Yesterday we had a session with Cory about the process and we found that it would make sense to separate the toggle functionality to a separate message. So if nobody objects I will make the update.
|
I think the proto gen stubs suggested from @ryanchristo makes sense. Im happy to give a concept ACK on the general design now- the only api change i'd still like to see is a MsgSealBatch message separated out. @robert-zaremba can you add go stubs and address the related go changes to prevent tests from breaking? |
|
Sorry but we are going back and forth without being constructive. 2 weeks ago I started with basic implementation for Msg methods and wanted to add dummy implementation for the RPC implementation while waiting for reviews (which would prevent troubles when someone generates the code and introduces additional changes). It got a criticism (#937 (comment) and and discord) to ONLY do proto code (so NO code generation and NO other methods). The tests are not breaking because this PR doesn't include generated code. Now, I prefer to merge this PR and do other PR with implementation to not hold on this PR any more. |
|
I've added |
I just want to start out by saying I really appreciate all the work you are doing with this new feature. I do think this has been constructive despite some back and forth. I could be wrong, but if I recall correctly, there was more than a simple dummy implementation in the original changes (I am unable to verify due to force push). Either way, we want to start with only the proto definitions, and then once the design is approved, we would generate proto files and add the dummy implementation, and if necessary, minimal changes to make build and tests pass. If we don't generate and make minimal changes, we will block other pull requests that are making changes to the proto files, which will need to generate proto and implement the missing messages from this pull request. I can add the dummy implementation to help move this along given the design has been approved. |
|
Sorry about the back & forth @robert-zaremba, but I also remembered a bit more than dummy implementation in what you originally was putting forward (I think I remember some implementation work as well). Happy to have this merged now, and let's discuss maybe on one of our next dev calls to make sure we're not going back and forth too much with process around proto PRs as design. |
|
Great work Robert. Although I was originally pretty restrained about the dynamic batch approach, I am quite pleased how this has turned out. |
|
@ryanchristo , @clevinson there was:
|
|
thanks for merging! 🍻 |
Description
Closes: #924
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