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 ORM proto definitions #700

Merged
merged 34 commits into from
Jan 26, 2022
Merged

feat: ecocredit ORM proto definitions #700

merged 34 commits into from
Jan 26, 2022

Conversation

technicallyty
Copy link
Contributor

Description

adds ORM proto definitions for x/ecocredit

Closes: n/a


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)

@technicallyty technicallyty requested a review from aaronc January 24, 2022 21:27
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.

Great start @technicallyty ! Left a few suggestions

proto/regen/ecocredit/v1beta1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/v1beta1/state.proto Show resolved Hide resolved
proto/regen/ecocredit/v1beta1/state.proto Show resolved Hide resolved
proto/regen/ecocredit/v1beta1/state.proto Outdated Show resolved Hide resolved
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.

Looking good. Don't we have sequences to track for generating class, project and batch IDs?

@technicallyty
Copy link
Contributor Author

Looking good. Don't we have sequences to track for generating class, project and batch IDs?

right, so these would be separate messages/tables right? i think batches are covered now with the addition of the auto_increment id

@aaronc
Copy link
Member

aaronc commented Jan 25, 2022

Yes separate tables, but I think not quite for batch ID because these batch IDs are unique per project and project IDs unique per class right?

@aaronc
Copy link
Member

aaronc commented Jan 25, 2022

We also need balances and supply

@technicallyty
Copy link
Contributor Author

technicallyty commented Jan 25, 2022

Yes separate tables, but I think not quite for batch ID because these batch IDs are unique per project and project IDs unique per class right?

actually looks like we were tracking batch sequence in the class via the field num_batches. should we continue with this method?

i've added a project seq table, and credit/basket balance. looks like the supply was already tracked in BatchInfo via total_amount.

@aaronc
Copy link
Member

aaronc commented Jan 25, 2022

Yes separate tables, but I think not quite for batch ID because these batch IDs are unique per project and project IDs unique per class right?

actually looks like we were tracking batch sequence in the class via the field num_batches. should we continue with this method?

i've added a project seq table, and credit/basket balance. looks like the supply was already tracked in BatchInfo via total_amount.

I sort of have the preference that things that get updated somewhat frequently - like sequences and supply - should live on separate tables from things which rarely or never change. It's not a huge deal, but I want to avoid the pattern that is done some places in the SDK like x/auth where every acct seq update rewrites the whole account with vesting, pub key and all

@technicallyty
Copy link
Contributor Author

Yes separate tables, but I think not quite for batch ID because these batch IDs are unique per project and project IDs unique per class right?

actually looks like we were tracking batch sequence in the class via the field num_batches. should we continue with this method?
i've added a project seq table, and credit/basket balance. looks like the supply was already tracked in BatchInfo via total_amount.

I sort of have the preference that things that get updated somewhat frequently - like sequences and supply - should live on separate tables from things which rarely or never change. It's not a huge deal, but I want to avoid the pattern that is done some places in the SDK like x/auth where every acct seq update rewrites the whole account with vesting, pub key and all

ok, added batch sequence and separate batch supply table

Co-authored-by: Aaron Craelius <aaron@regen.network>
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.

LGTM. Thanks @technicallyty 🎉 !

@aaronc aaronc enabled auto-merge (squash) January 25, 2022 20:13
@aaronc
Copy link
Member

aaronc commented Jan 25, 2022

Just need to fix conflicts

@codecov
Copy link

codecov bot commented Jan 25, 2022

Codecov Report

Merging #700 (f5c3d3d) into master (26cb432) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #700   +/-   ##
=======================================
  Coverage   74.80%   74.80%           
=======================================
  Files         118      118           
  Lines       19542    19542           
=======================================
  Hits        14618    14618           
  Misses       3947     3947           
  Partials      977      977           
Flag Coverage Δ
experimental-codecov 74.84% <ø> (-0.07%) ⬇️
stable-codecov 67.76% <ø> (+0.06%) ⬆️

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

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>
proto/regen/ecocredit/v1beta1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/v1beta1/state.proto Outdated Show resolved Hide resolved
proto/regen/ecocredit/v1beta1/baskets/state.proto Outdated Show resolved Hide resolved
@ryanchristo ryanchristo enabled auto-merge (squash) January 26, 2022 04:37
@ryanchristo ryanchristo merged commit 6699a38 into master Jan 26, 2022
@ryanchristo ryanchristo deleted the ty/eco_orm branch January 26, 2022 04:38
// sell_order_id is the sell order ID against which the buyer is trying to buy.
// When sell_order_id is set, this is known as a direct buy order because it
// is placed directly against a specific sell order.
uint64 sell_order_id = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this be a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think that would be out of the scope of this PR. this structure was copied from existing proto defs. so i believe this concern would warrant its own issue

Copy link
Member

Choose a reason for hiding this comment

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

why a list?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@aaronc to select multiple orders to fill.

Copy link
Member

Choose a reason for hiding this comment

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

this model is just based on buying from a specific sell order. it's not a batch operation. it's like a shopping cart where you buy one product from one vendor


// next_id is the sequence number for the project
uint64 next_id = 2;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

why do we need it? This could be easily done with a simple counter (no need to store the project_id in value).

Copy link
Contributor Author

@technicallyty technicallyty Jan 26, 2022

Choose a reason for hiding this comment

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

so the format of the project id, when auto-generated, is classID + sequence number. looking at it again, i think actually the project_id field should be class_id. but in any case, the 2 fields are needed

Copy link
Member

Choose a reason for hiding this comment

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

it should be class_id

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, so ProjectSequence is a type for a key.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@technicallyty if you will be updating the project_id field, let's update the message documentation as well.

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