From 0f026aa01ade071b4ba0d8caa5d5a19db2476f70 Mon Sep 17 00:00:00 2001 From: Ryan Christoffersen <12519942+ryanchristo@users.noreply.github.com> Date: Tue, 5 Jul 2022 14:08:19 -0700 Subject: [PATCH] feat(x/ecocredit): unique reference id at message server level (#1228) * feat(x/ecocredit): unique reference id at message server level * add and update project tests * add and update project tests * clean up * update bridge receive comments * update bridge receive comments * update bridge receive comments * fix tests * Update x/ecocredit/server/core/msg_create_project_test.go * Apply suggestions from code review * Update x/ecocredit/server/core/create_project.go Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> Co-authored-by: Tyler <48813565+technicallyty@users.noreply.github.com> --- x/ecocredit/client/testsuite/tx.go | 6 +- x/ecocredit/core/utils.go | 13 ++ x/ecocredit/core/utils_test.go | 13 ++ x/ecocredit/server/core/bridge_receive.go | 13 +- x/ecocredit/server/core/create_project.go | 31 +++++ .../core/features/msg_create_project.feature | 58 +++++--- .../server/core/msg_create_project_test.go | 124 ++++++++++++------ 7 files changed, 196 insertions(+), 62 deletions(-) diff --git a/x/ecocredit/client/testsuite/tx.go b/x/ecocredit/client/testsuite/tx.go index 818f78330d..865f1efde3 100644 --- a/x/ecocredit/client/testsuite/tx.go +++ b/x/ecocredit/client/testsuite/tx.go @@ -991,7 +991,7 @@ func (s *IntegrationTestSuite) TestUpdateProjectAdmin() { ClassId: s.classId, Metadata: "metadata", Jurisdiction: "US-WA", - ReferenceId: s.projectReferenceId, + ReferenceId: "VCS-002", }) // create new project in order to not interfere with other tests @@ -1000,7 +1000,7 @@ func (s *IntegrationTestSuite) TestUpdateProjectAdmin() { ClassId: s.classId, Metadata: "metadata", Jurisdiction: "US-WA", - ReferenceId: s.projectReferenceId, + ReferenceId: "VCS-003", }) // create new project in order to not interfere with other tests @@ -1009,7 +1009,7 @@ func (s *IntegrationTestSuite) TestUpdateProjectAdmin() { ClassId: s.classId, Metadata: "metadata", Jurisdiction: "US-WA", - ReferenceId: s.projectReferenceId, + ReferenceId: "VCS-004", }) testCases := []struct { diff --git a/x/ecocredit/core/utils.go b/x/ecocredit/core/utils.go index 68cff2b203..f286f90095 100644 --- a/x/ecocredit/core/utils.go +++ b/x/ecocredit/core/utils.go @@ -153,6 +153,19 @@ func ValidateJurisdiction(jurisdiction string) error { return nil } +// GetClassIdFromProjectId returns the credit class ID in a project ID. +func GetClassIdFromProjectId(projectId string) string { + var s strings.Builder + for _, r := range projectId { + if r != '-' { + s.WriteRune(r) + continue + } + break + } + return s.String() +} + // GetClassIdFromBatchDenom returns the credit class ID in a batch denom. func GetClassIdFromBatchDenom(denom string) string { var s strings.Builder diff --git a/x/ecocredit/core/utils_test.go b/x/ecocredit/core/utils_test.go index 37d7511672..8e0f4a4786 100644 --- a/x/ecocredit/core/utils_test.go +++ b/x/ecocredit/core/utils_test.go @@ -15,6 +15,7 @@ func TestUtils(t *testing.T) { t.Run("TestInvalidProjectId", rapid.MakeCheck(testInvalidProjectId)) t.Run("TestFormatBatchDenom", rapid.MakeCheck(testFormatBatchDenom)) t.Run("TestInvalidBatchDenom", rapid.MakeCheck(testInvalidBatchDenom)) + t.Run("TestGetClassIdFromProjectId", rapid.MakeCheck(testGetClassIdFromProjectId)) t.Run("TestGetClassIdFromBatchDenom", rapid.MakeCheck(testGetClassIdFromBatchDenom)) t.Run("GetCreditTypeAbbrevFromClassId", rapid.MakeCheck(testGetCreditTypeAbbrevFromClassId)) } @@ -79,6 +80,18 @@ func testInvalidBatchDenom(t *rapid.T) { require.Error(t, ValidateBatchDenom(batchDenom)) } +func testGetClassIdFromProjectId(t *rapid.T) { + creditType := genCreditType.Draw(t, "creditType").(*CreditType) + classSeq := rapid.Uint64().Draw(t, "classSeq").(uint64) + projectSeq := rapid.Uint64().Draw(t, "projectSeq").(uint64) + + classId := FormatClassId(creditType.Abbreviation, classSeq) + projectId := FormatProjectId(classId, projectSeq) + + result := GetClassIdFromProjectId(projectId) + require.Equal(t, classId, result) +} + func testGetClassIdFromBatchDenom(t *rapid.T) { creditType := genCreditType.Draw(t, "creditType").(*CreditType) classSeq := rapid.Uint64().Draw(t, "classSeq").(uint64) diff --git a/x/ecocredit/server/core/bridge_receive.go b/x/ecocredit/server/core/bridge_receive.go index 5364b00d17..145fa662b4 100644 --- a/x/ecocredit/server/core/bridge_receive.go +++ b/x/ecocredit/server/core/bridge_receive.go @@ -173,9 +173,11 @@ func (k Keeper) getBatchFromBridgeReq(ctx context.Context, req *core.MsgBridgeRe return nil, nil } -// getProjectFromBridgeReq attempts to get a project from state given the request. -// The first project seen in the iterator is returned. -// If no projects are found, nil is returned for both values. +// getProjectFromBridgeReq attempts to find a project with a matching reference id within the scope +// of the credit class. No more than one project will be returned when we list the projects based on +// class id and reference id because we enforce uniqueness on non-empty reference ids within the scope +// of a credit class (and we do this at the message server level and not the ORM level because reference +// id is optional when using Msg/CreateProject). If no project is found, nil is returned for both values. func (k Keeper) getProjectFromBridgeReq(ctx context.Context, req *core.MsgBridgeReceive_Project, classId string) (*api.Project, error) { class, err := k.stateStore.ClassTable().GetById(ctx, classId) if err != nil { @@ -189,7 +191,9 @@ func (k Keeper) getProjectFromBridgeReq(ctx context.Context, req *core.MsgBridge return nil, err } - // we only want the first project that matches the reference ID, so we do not loop here. + // we only want the first project that matches the reference ID, so we do not loop here. We enforce + // uniqueness for a non-empty reference id at the message service level so as long as the reference + // id is not empty (verified in validate basic), no more than one project will ever be returned. var project *api.Project if it.Next() { var err error @@ -199,5 +203,6 @@ func (k Keeper) getProjectFromBridgeReq(ctx context.Context, req *core.MsgBridge } } it.Close() + return project, nil } diff --git a/x/ecocredit/server/core/create_project.go b/x/ecocredit/server/core/create_project.go index 5738c27f57..8be0fcec69 100644 --- a/x/ecocredit/server/core/create_project.go +++ b/x/ecocredit/server/core/create_project.go @@ -35,6 +35,12 @@ func (k Keeper) CreateProject(ctx context.Context, req *core.MsgCreateProject) ( return nil, err } + // check if non-empty reference id is unique within the scope of the credit class + err = k.verifyReferenceId(ctx, classInfo.Key, req.ReferenceId) + if err != nil { + return nil, err + } + if err = k.stateStore.ProjectTable().Insert(ctx, &api.Project{ Id: projectID, Admin: adminAddress, @@ -80,3 +86,28 @@ func (k Keeper) genProjectID(ctx context.Context, classKey uint64, classID strin return core.FormatProjectId(classID, nextSeq), nil } + +// verifyReferenceId prevents multiple projects from having the same reference id within the +// scope of a credit class. We verify this here at the message server level rather than at the +// ORM level because reference id is optional and therefore multiple projects within the scope +// of a credit class can have an empty reference id (see BridgeReceive for more information) +func (k Keeper) verifyReferenceId(ctx context.Context, classKey uint64, referenceId string) error { + if referenceId == "" { + // reference id is optional so an empty reference id is valid + return nil + } + + key := api.ProjectClassKeyReferenceIdIndexKey{}.WithClassKeyReferenceId(classKey, referenceId) + it, err := k.stateStore.ProjectTable().List(ctx, key) + if err != nil { + return err + } + defer it.Close() + if it.Next() { + return sdkerrors.ErrInvalidRequest.Wrapf( + "a project with reference id %s already exists within this credit class", referenceId, + ) + } + + return nil +} diff --git a/x/ecocredit/server/core/features/msg_create_project.feature b/x/ecocredit/server/core/features/msg_create_project.feature index d9f5ae8eb0..56efe5d8ee 100644 --- a/x/ecocredit/server/core/features/msg_create_project.feature +++ b/x/ecocredit/server/core/features/msg_create_project.feature @@ -2,33 +2,34 @@ Feature: CreateProject Projects can be created: - when the creator is an approved credit class issuer + - when the non-empty reference id is unique within the scope of the credit class - ... Rule: A project id is always unique Scenario: multiple projects from the same credit class - Given a credit type exists with abbreviation "A" - And alice has created a credit class with credit type "A" - And alice has created a project with credit class id "A01" - When alice creates a project with credit class id "A01" - Then the project exists with project id "A01-002" + Given a credit type with abbreviation "A" + And a credit class with class id "A01" and issuer alice + And a project with project id "A01-001" + When alice attempts to create a project with class id "A01" + Then expect project with project id "A01-002" Scenario: multiple projects from different credit classes - Given a credit type exists with abbreviation "A" - And a credit type exists with abbreviation "B" - And alice has created a credit class with credit type "A" - And alice has created a credit class with credit type "B" - And alice has created a project with credit class id "A01" - And alice has created a project with credit class id "B01" - When alice creates a project with credit class id "B01" - Then the project exists with project id "B01-001" + Given a credit type with abbreviation "A" + And a credit type with abbreviation "B" + And a credit class with class id "A01" and issuer alice + And a credit class with class id "B01" and issuer alice + And a project with project id "A01-001" + And a project with project id "B01-001" + When alice attempts to create a project with class id "B01" + Then expect project with project id "B01-002" Scenario Outline: project id is formatted correctly - Given a credit type exists with abbreviation "A" - And alice has created a credit class with credit type "A" - And the project sequence for credit class "A01" is "" - When alice creates a project with credit class id "A01" - Then the project exists with project id "" + Given a credit type with abbreviation "A" + And a credit class with class id "A01" and issuer alice + And a project sequence "" for credit class "A01" + When alice attempts to create a project with class id "A01" + Then expect project with project id "" Examples: | next_sequence | project_id | @@ -38,3 +39,24 @@ Feature: CreateProject | 100 | A01-100 | | 1000 | A01-1000 | | 10000 | A01-10000 | + + Rule: A non-empty reference id must be unique within the scope of a credit class + + Background: + Given a credit type with abbreviation "C" + And a credit class with class id "C01" and issuer alice + + Scenario: non-empty reference id is unique within credit class + Given a project with project id "C01-001" and reference id "VCS-001" + When alice attempts to create a project with class id "C01" and reference id "VCS-002" + Then expect no error + + Scenario: empty reference id is allowed for multiple projects + Given a project with project id "C01-001" and reference id "" + When alice attempts to create a project with class id "C01" and reference id "" + Then expect no error + + Scenario: non-empty reference id is not unique within credit class + Given a project with project id "C01-001" and reference id "VCS-001" + When alice attempts to create a project with class id "C01" and reference id "VCS-001" + Then expect the error "a project with reference id VCS-001 already exists within this credit class: invalid request" diff --git a/x/ecocredit/server/core/msg_create_project_test.go b/x/ecocredit/server/core/msg_create_project_test.go index 6ad129a2ad..cfa5699ec7 100644 --- a/x/ecocredit/server/core/msg_create_project_test.go +++ b/x/ecocredit/server/core/msg_create_project_test.go @@ -2,9 +2,9 @@ package core import ( "strconv" + "strings" "testing" - "github.com/golang/mock/gomock" "github.com/regen-network/gocuke" "github.com/stretchr/testify/require" @@ -12,12 +12,12 @@ import ( api "github.com/regen-network/regen-ledger/api/regen/ecocredit/v1" "github.com/regen-network/regen-ledger/x/ecocredit/core" - "github.com/regen-network/regen-ledger/x/ecocredit/server/utils" ) type createProjectSuite struct { *baseSuite alice sdk.AccAddress + res *core.MsgCreateProjectResponse err error } @@ -30,72 +30,122 @@ func (s *createProjectSuite) Before(t gocuke.TestingT) { s.alice = s.addr } -func (s *createProjectSuite) ACreditTypeExistsWithAbbreviation(a string) { - err := s.k.stateStore.CreditTypeTable().Insert(s.ctx, &api.CreditType{ +func (s *createProjectSuite) ACreditTypeWithAbbreviation(a string) { + // TODO: Save for now but credit type should not exist prior to unit test #893 + err := s.k.stateStore.CreditTypeTable().Save(s.ctx, &api.CreditType{ Abbreviation: a, Name: a, }) require.NoError(s.t, err) } -func (s *createProjectSuite) AliceHasCreatedACreditClassWithCreditType(a string) { - gmAny := gomock.Any() +func (s *createProjectSuite) ACreditClassWithClassIdAndIssuerAlice(a string) { + creditTypeAbbrev := core.GetCreditTypeAbbrevFromClassId(a) - fee := sdk.Coin{ - Denom: "regen", - Amount: sdk.NewInt(20), - } + cKey, err := s.k.stateStore.ClassTable().InsertReturningID(s.ctx, &api.Class{ + Id: a, + CreditTypeAbbrev: creditTypeAbbrev, + }) + require.NoError(s.t, err) - allowListEnabled := false - utils.ExpectParamGet(&allowListEnabled, s.paramsKeeper, core.KeyAllowlistEnabled, 1) - coinFee := sdk.Coins{fee} - utils.ExpectParamGet(&coinFee, s.paramsKeeper, core.KeyCreditClassFee, 1) + err = s.k.stateStore.ClassIssuerTable().Insert(s.ctx, &api.ClassIssuer{ + ClassKey: cKey, + Issuer: s.alice, + }) + require.NoError(s.t, err) +} - s.bankKeeper.EXPECT().SendCoinsFromAccountToModule(gmAny, gmAny, gmAny, gmAny).Return(nil).AnyTimes() +func (s *createProjectSuite) AProjectSequenceForCreditClass(a, b string) { + class, err := s.k.stateStore.ClassTable().GetById(s.ctx, b) + require.NoError(s.t, err) - s.bankKeeper.EXPECT().BurnCoins(gmAny, gmAny, gmAny).Return(nil).AnyTimes() + nextSequence, err := strconv.ParseUint(a, 10, 32) + require.NoError(s.t, err) - _, err := s.k.CreateClass(s.ctx, &core.MsgCreateClass{ - Admin: s.alice.String(), - Issuers: []string{s.alice.String()}, - CreditTypeAbbrev: a, - Fee: &fee, + err = s.k.stateStore.ProjectSequenceTable().Insert(s.ctx, &api.ProjectSequence{ + ClassKey: class.Key, + NextSequence: nextSequence, }) require.NoError(s.t, err) } -func (s *createProjectSuite) AliceHasCreatedAProjectWithCreditClassId(a string) { - _, s.err = s.k.CreateProject(s.ctx, &core.MsgCreateProject{ - Admin: s.alice.String(), - ClassId: a, - Jurisdiction: "US", +func (s *createProjectSuite) AProjectWithProjectId(a string) { + classId := core.GetClassIdFromProjectId(a) + + class, err := s.k.stateStore.ClassTable().GetById(s.ctx, classId) + require.NoError(s.t, err) + + err = s.k.stateStore.ProjectTable().Insert(s.ctx, &api.Project{ + Id: a, + ClassKey: class.Key, + }) + require.NoError(s.t, err) + + seq := s.getProjectSequence(a) + + // Save because project sequence may already exist + err = s.k.stateStore.ProjectSequenceTable().Save(s.ctx, &api.ProjectSequence{ + ClassKey: class.Key, + NextSequence: seq + 1, }) + require.NoError(s.t, err) } -func (s *createProjectSuite) TheProjectSequenceForCreditClassIs(a, b string) { - class, err := s.k.stateStore.ClassTable().GetById(s.ctx, a) +func (s *createProjectSuite) AProjectWithProjectIdAndReferenceId(a, b string) { + classId := core.GetClassIdFromProjectId(a) + + class, err := s.k.stateStore.ClassTable().GetById(s.ctx, classId) require.NoError(s.t, err) - nextSequence, err := strconv.ParseUint(b, 10, 32) + err = s.k.stateStore.ProjectTable().Insert(s.ctx, &api.Project{ + Id: a, + ClassKey: class.Key, + ReferenceId: b, + }) require.NoError(s.t, err) - err = s.k.stateStore.ProjectSequenceTable().Insert(s.ctx, &api.ProjectSequence{ + seq := s.getProjectSequence(a) + + // Save because project sequence may already exist + err = s.k.stateStore.ProjectSequenceTable().Save(s.ctx, &api.ProjectSequence{ ClassKey: class.Key, - NextSequence: nextSequence, + NextSequence: seq + 1, }) require.NoError(s.t, err) } -func (s *createProjectSuite) AliceCreatesAProjectWithCreditClassId(a string) { - _, s.err = s.k.CreateProject(s.ctx, &core.MsgCreateProject{ - Admin: s.alice.String(), - ClassId: a, - Jurisdiction: "US", +func (s *createProjectSuite) AliceAttemptsToCreateAProjectWithClassId(a string) { + s.res, s.err = s.k.CreateProject(s.ctx, &core.MsgCreateProject{ + Admin: s.alice.String(), + ClassId: a, }) } -func (s *createProjectSuite) TheProjectExistsWithProjectId(a string) { +func (s *createProjectSuite) AliceAttemptsToCreateAProjectWithClassIdAndReferenceId(a, b string) { + s.res, s.err = s.k.CreateProject(s.ctx, &core.MsgCreateProject{ + Admin: s.alice.String(), + ClassId: a, + ReferenceId: b, + }) +} + +func (s *createProjectSuite) ExpectNoError() { + require.NoError(s.t, s.err) +} + +func (s *createProjectSuite) ExpectTheError(a string) { + require.EqualError(s.t, s.err, a) +} + +func (s *createProjectSuite) ExpectProjectWithProjectId(a string) { project, err := s.k.stateStore.ProjectTable().GetById(s.ctx, a) require.NoError(s.t, err) require.Equal(s.t, a, project.Id) } + +func (s *createProjectSuite) getProjectSequence(projectId string) uint64 { + str := strings.Split(projectId, "-") + seq, err := strconv.ParseUint(str[1], 10, 32) + require.NoError(s.t, err) + return seq +}