refactor(ecocredit)!: migrate core ecocredit msg_server to use ORM #723

technicallyty · 2022-02-03T19:03:23Z

Description

beginning of ecocredit ORM refactor

Closes: #696

Closes: #707

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...

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 · 2022-02-05T20:15:48Z

x/ecocredit/server/testsuite/suite.go

@@ -50,14 +54,13 @@ func NewIntegrationTestSuite(fixtureFactory testutil.FixtureFactory, paramSpace

 func (s *IntegrationTestSuite) SetupSuite() {


I think we should consider refactoring all these separate integration test into smaller, bite sized unit tests, and then just have one big integration test. imo it makes more sense at these tests look more like unit tests than integration anyways

it's a bit confusing to figure my way around this code. the batch info test is broken and the case thats failing has a hard coded batch denom to search for. i'm not really even sure what the best approach to fixing that test would be other than obliterating the existing test structure to fit my needs..

should we create an issue to move these smaller tests into unit tests? @aaronc @ryanchristo @robert-zaremba thoughts?

I agree. A lot of these tests are written like unit tests already. I think it makes sense to separate them out and to simply the integration tests in a separate issue. You should be able to trace the batch denom issue without obliterating the existing structure. Would be happy to take a look if you get stuck.

proto/regen/ecocredit/v1beta1/state.proto

proto/regen/ecocredit/v1alpha2/query.proto

technicallyty · 2022-02-07T21:40:34Z

proto/regen/ecocredit/basket/v1beta1/tx.proto

@@ -4,7 +4,7 @@ package regen.ecocredit.basket.v1beta1;

 import "regen/ecocredit/basket/v1beta1/types.proto";

-option go_package = "github.com/regen-network/regen-ledger/x/ecocredit/v1beta1";
+option go_package = "github.com/regen-network/regen-ledger/x/ecocredit/basket/v1beta1";


code wasn't being generated for these packages, i assume they were being overwritten by core which gets written last? all code from these sub packages get generated now

x/ecocredit/server/core/query_server.go

ryanchristo

Looking great. Nice work! Still working through the review. A few comments to start.

proto/regen/ecocredit/v1beta1/events.proto

proto/regen/ecocredit/v1beta1/tx.proto

ryanchristo · 2022-02-08T01:38:26Z

types/math/dec.go

+ if s == "" {
+ s = "0"
+ }


Should we throw an error if the string is empty? I think it might be safer to require a value to be explicitly set rather than assuming an empty string should be 0.

i found a few places where were we would explicitly check for the empty string case and then set it to "0" anyway, so it felt like a good way to reduce that.

are there any specific cases where we'd prefer to error here instead of defaulting to 0?

If we want to handle "", no need to parse anything just return a zero value.

Seems like we'd check for "" elsewhere if we actually want a nullable case so I think this default is okay

x/ecocredit/server/core/msg_server.go

ryanchristo · 2022-02-08T02:49:17Z

x/ecocredit/server/core/msg_server.go

+ // delete the old issuers
+ if err = s.stateStore.ClassIssuerStore().DeleteBy(ctx, ecocreditv1beta1.ClassIssuerClassIdIssuerIndexKey{}.WithClassId(class.Name)); err != nil {
+ return nil, err
+ }


Can we use Update rather than deleting the old and inserting the new one by one?

the primary key is made up of the classID+issuer_address, so it wouldn't work in this case. if it had a simple row ID primary key with class+issuer indexes, i could do that - i'm not sure on the benchmarks of either case though, could look into some metrics for ORM ops if we value it

x/ecocredit/server/core/msg_server.go

x/ecocredit/server/core/operations.go

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

…k/regen-ledger into ty/696-eco_orm_core

proto/regen/ecocredit/v1beta1/events.proto

x/ecocredit/denom.go

x/ecocredit/server/core/invariants_test.go

x/ecocredit/server/core/msg_server.go

aaronc

Implementation generally looks good. A few things to clean-up. Some of them can be in follow-ups as they're related to existing design choices

x/ecocredit/server/core/msg_server.go

aaronc · 2022-02-08T15:05:20Z

x/ecocredit/server/core/msg_server.go

+ projectID := req.ProjectId
+ if projectID == "" {
+ found := false
+ for !found {


why do we need a loop here? something seems not quite right here

it was actually your suggestion in the original projects PR 😄 - #647 (comment)

we need to loop to avoid a clash but the way this loop is structured is just confusing

do you have any suggestions to make it clearer? i had thought it was pretty straightforward but open to suggestions

x/ecocredit/server/core/msg_server.go

technicallyty · 2022-02-24T00:11:42Z

this PR is now more in line with the architecture used for #732. There's quite a lot of code in here, so I can opt to split this into chunks and submit multiple, smaller PR's.

I've implemented the msg server and query servers for v1beta1 proto files with unit tests. It is not wired up to the existing msg_server yet, as that require moving around more things and making an already large diff even larger.

Would appreciate any feedback/ideas on how to proceed from here! @aaronc @ryanchristo @clevinson :-)

ryanchristo · 2022-03-03T16:15:01Z

This pull request is being broken up into smaller pull requests.

blushi · 2022-03-04T08:33:42Z

This pull request is being broken up into smaller pull requests.

should we just close this then?

technicallyty commented Feb 5, 2022

View reviewed changes

aaronc reviewed Feb 7, 2022

View reviewed changes

proto/regen/ecocredit/v1beta1/state.proto Outdated Show resolved Hide resolved

aaronc reviewed Feb 7, 2022

View reviewed changes

proto/regen/ecocredit/v1alpha2/query.proto Outdated Show resolved Hide resolved

fix: generate all v1beta1 codes in their own packages, fix core servers

8676847

technicallyty force-pushed the ty/696-eco_orm_core branch from 2178d73 to 8676847 Compare February 7, 2022 21:36

technicallyty added 3 commits February 7, 2022 13:43

chore: reduce FormatClassID function

772e109

chore: reduce to single state store instance

534f46b

chore: fix invariants

3e11a19

technicallyty commented Feb 7, 2022

View reviewed changes

technicallyty added 2 commits February 7, 2022 14:09

feat: add row id to events/responses

015eaff

fix: formatclassid compatability

8b83dd1

technicallyty marked this pull request as ready for review February 7, 2022 22:18

technicallyty requested review from aaronc and ryanchristo February 7, 2022 22:22

ryanchristo reviewed Feb 8, 2022

View reviewed changes

technicallyty and others added 4 commits February 7, 2022 22:19

Update x/ecocredit/server/core/msg_server.go

1e0a2b9

Co-authored-by: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com>

chore: address review + switch remaining string addr to bytes

20695f8

Merge branch 'ty/696-eco_orm_core' of https://github.com/regen-networ…

5258920

…k/regen-ledger into ty/696-eco_orm_core

fix: query byte addrs

d1f1b0d

technicallyty mentioned this pull request Feb 8, 2022

ecocredit refactor follow-ups #728

Closed

8 tasks

chore: issue link

df492ef

technicallyty mentioned this pull request Feb 8, 2022

Implement ecocredit ORM params #729

Closed

4 tasks

amaury1093 assigned blushi Feb 8, 2022

aaronc reviewed Feb 8, 2022

View reviewed changes

proto/regen/ecocredit/v1beta1/events.proto Outdated Show resolved Hide resolved

proto/regen/ecocredit/v1beta1/events.proto Outdated Show resolved Hide resolved

aaronc reviewed Feb 8, 2022

View reviewed changes

x/ecocredit/denom.go Show resolved Hide resolved

x/ecocredit/server/core/invariants_test.go Outdated Show resolved Hide resolved

x/ecocredit/server/core/msg_server.go Outdated Show resolved Hide resolved

aaronc reviewed Feb 8, 2022

View reviewed changes

technicallyty added 4 commits February 8, 2022 08:26

chore: invariants test use test memory backend

4dd9cf6

test: add GetClassIdFromBatchDenom test

a99bff3

fix: change assertClassIssuer to use has

cdb7edc

chore: remove row_id from response/events

dd29c44

technicallyty added 5 commits February 8, 2022 08:52

refactor: use class row id for class issuer table

7186b2e

feat: add AddIssuers and RemoveIssuers from update class issuers method

90e5898

fix: credit types use ORM and remove marketplace code

d03d4ea

chore: add comment to remove params

8b6960d

chore: remove non-core queries

9bdcd8a

technicallyty force-pushed the ty/696-eco_orm_core branch from 2a7aac3 to 9bdcd8a Compare February 8, 2022 20:13

aaronc mentioned this pull request Feb 11, 2022

feat(x/ecocredit): Put basket server method #758

Merged

19 tasks

technicallyty added 13 commits February 16, 2022 00:39

wip: first few

6639754

feat: core impl keeper

c3d3afa

feat: impl, test

701dda0

feat(wip): unit tests

9675e73

wip: tests

f27d9b7

feat: update class unit tests

c6a1f5d

chore: add query utils

3f786c2

chore: cleanup queries

df6d5b3

chore: go mod tidy

f01d678

Merge branch 'master' into ty/696-eco_orm_core

a051202

fix: dec_test should allow empty string

f6d8e4b

wip: more tests

5e06a11

feat: query unit tests

f60c8bc

Merge branch 'master' into ty/696-eco_orm_core

55690f5

technicallyty marked this pull request as draft February 28, 2022 22:51

blushi removed their assignment Mar 4, 2022

technicallyty closed this Mar 4, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(ecocredit)!: migrate core ecocredit msg_server to use ORM #723

refactor(ecocredit)!: migrate core ecocredit msg_server to use ORM #723

technicallyty commented Feb 3, 2022 •

edited by zube bot

Loading

technicallyty Feb 5, 2022 •

edited

Loading

ryanchristo Feb 7, 2022

technicallyty Feb 7, 2022

ryanchristo left a comment

ryanchristo Feb 8, 2022

technicallyty Feb 8, 2022 •

edited

Loading

aaronc Feb 8, 2022 •

edited

Loading

ryanchristo Feb 8, 2022

technicallyty Feb 8, 2022

aaronc left a comment

aaronc Feb 8, 2022

technicallyty Feb 8, 2022

aaronc Feb 8, 2022

technicallyty Feb 8, 2022

technicallyty commented Feb 24, 2022

ryanchristo commented Mar 3, 2022

blushi commented Mar 4, 2022

		@@ -50,14 +54,13 @@ func NewIntegrationTestSuite(fixtureFactory testutil.FixtureFactory, paramSpace

		func (s *IntegrationTestSuite) SetupSuite() {

refactor(ecocredit)!: migrate core ecocredit msg_server to use ORM #723

refactor(ecocredit)!: migrate core ecocredit msg_server to use ORM #723

Conversation

technicallyty commented Feb 3, 2022 • edited by zube bot Loading

Description

Author Checklist

Reviewers Checklist

technicallyty Feb 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanchristo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technicallyty Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

aaronc Feb 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technicallyty commented Feb 24, 2022

ryanchristo commented Mar 3, 2022

blushi commented Mar 4, 2022

technicallyty commented Feb 3, 2022 •

edited by zube bot

Loading

technicallyty Feb 5, 2022 •

edited

Loading

technicallyty Feb 8, 2022 •

edited

Loading

aaronc Feb 8, 2022 •

edited

Loading