SI-2544 - Feature/identity api devicedefinition query tableland #88

rhvivancoeffio · 2024-04-05T13:48:13Z

No description provided.

linear · 2024-04-05T13:48:15Z

SI-2544 Create Device Definition Query from tableland

KevinJoiner

We might want to change the name from device definition to vehicle definitions since these queries only relate to vehicles and would not make sense for other devices in the future.

internal/config/settings.go

KevinJoiner · 2024-04-05T14:11:43Z

internal/repositories/devicedefinition/devicedefinition.go

+	"context"
+	"encoding/json"
+	"fmt"
+	gmodel "github.com/DIMO-Network/identity-api/graph/model"
+	"github.com/DIMO-Network/identity-api/internal/helpers"
+	"github.com/DIMO-Network/identity-api/internal/repositories/base"
+	"github.com/vektah/gqlparser/v2/gqlerror"
+	"golang.org/x/exp/slices"
+	"net/http"
+	"net/url"
+	"path"
+	"strings"


std lib vars should be separate from external imports if you are using vscode this can be done by changing the golang formater to goimports

The references was updated

internal/repositories/devicedefinition/devicedefinition.go

graph/schema/devicedefinition.graphqls

internal/repositories/devicedefinition/devicedefinition.go

graph/schema/devicedefinition.graphqls

KevinJoiner · 2024-04-05T14:43:24Z

internal/repositories/devicedefinition/devicedefinition.go

+func ToAPI(v *DeviceDefinitionTablelandModel) *gmodel.DeviceDefinition {
+	var result = gmodel.DeviceDefinition{}
+
+	result.ID = v.ID


Suggested change

result.ID = v.ID

globalID, err := base.EncodeGlobalTokenID(TokenPrefix, v.ID)

if err != nil {

return nil, fmt.Errorf("error encoding device definition id: %w", err)

}

result.ID = globalID

result.DefinitionID = v.ID

elffjs · 2024-04-24T16:01:24Z

.gitignore

@@ -23,3 +23,5 @@ settings.yaml

 .idea

+# Desktop Service Store (macOS)
+.DS_Store


Can we do a newline at the end of these ppc

I'll do it.

elffjs · 2024-04-24T16:01:40Z

cmd/identity-api/main.go

-	}
-
-	logger.Info().Msg("Contract events consumer started.")
+	//kc := kafka.Config{


Let's not commit this commenting out!

elffjs · 2024-04-24T16:02:13Z

graph/devicedefinition.resolvers.go

+)
+
+// Devicedefinition is the resolver for the devicedefinition field.
+func (r *queryResolver) Devicedefinition(ctx context.Context, by model.DevicedefinitionBy) (*model.DeviceDefinition, error) {


We seem split in the Go and the GraphQL on whether it's devicedefinition or deviceDefinition. I would vote for deviceDefinition.

The definition was updated

elffjs · 2024-04-24T16:02:37Z

graph/resolver.go

 }

 // NewResolver creates a new Resolver with allocated repositories.
 func NewResolver(baseRepo *base.Repository) *Resolver {
+


Suggested change

I will run around deleting these newlines.

elffjs · 2024-04-24T16:03:28Z

graph/schema/devicedefinition.graphqls

+  ksuid: String
+
+  """
+  Model for this device definition.
+  """
+  model: String
+
+  """
+  Year for this device definition.
+  """
+  year: Int
+
+  """
+  Device Type for this device definition.
+  """
+  deviceType: String
+
+  """
+  Image URI for this device definition.
+  """
+  imageURI: String


Are some of these required?

I would think we would follow JavaScript casing and do imageUri. What do we do elsewhere in this repo?

The property was updated

elffjs · 2024-04-24T16:19:56Z

internal/repositories/devicedefinition/devicedefinition.go

+	if errList != nil {
+		return res, errList
+	}
+	return res, nil


We'd get the same result by writing return res, errList yeah?

The func was updated

elffjs · 2024-04-24T16:21:48Z

internal/services/contracts_manufacturer_service.go

+	contractAddress := common.HexToAddress(m.settings.DIMORegistryAddr)
+	queryInstance, err := contracts.NewRegistry(contractAddress, m.client)


I think we could initialize these two things just once, in the New function.

The func was updated

elffjs · 2024-04-24T16:22:30Z

internal/services/tableland_api_service.go

+		return fmt.Errorf("failed to create request: %w", err)
+	}
+
+	resp, err := http.DefaultClient.Do(req)


Perhaps we should use a client with a timeout, or use a context with some deadline.

elffjs · 2024-04-24T16:22:40Z

internal/services/tableland_api_service.go

+	if err != nil {
+		return fmt.Errorf("failed to complete request: %w", err)
+	}
+
+	if err != nil {
+		return err
+	}


Why two of these?

The definition was updated

elffjs · 2024-04-24T16:23:08Z

internal/services/tableland_api_service.go

+}
+
+func (r *TablelandApiService) Query(ctx context.Context, queryParams map[string]string, result interface{}) error {
+	fullURL, err := url.Parse(r.settings.TablelandAPIGateway)


Maybe do this in the "constructor"?

The definition was updated

0xdev22 · 2024-04-24T16:24:38Z

cmd/identity-api/main.go

-	}
-
-	logger.Info().Msg("Contract events consumer started.")
+	//kc := kafka.Config{


Please uncomment this function

0xdev22 · 2024-04-24T16:35:49Z

graph/resolver.go

+
+	ethClient, err := ethclient.Dial(baseRepo.Settings.EthereumRPCURL)
+	if err != nil {
+		fmt.Print("Failed to create Ethereum client.")


This error should be handled more severely, maybe a log.Fatal or something

0xdev22 · 2024-04-24T17:18:59Z

graph/schema/devicedefinition.graphqls

+  """
+  Legacy ID for this device definition.
+  """
+  ksuid: String


Can we name this something else, maybe LegacyID. KSUID doesn't describe what the variable does.

The proeprty was updated

0xdev22 · 2024-04-24T17:28:10Z

internal/repositories/devicedefinition/devicedefinition.go

+
+	tableName, err := r.ManufacturerContractService.GetTableName(ctx, manufacturer.ID)
+	if err != nil {
+		return nil, err


Can we wrap this error fmt.Errorf("...", err.Error())

The func was updated

0xdev22 · 2024-04-24T17:29:47Z

internal/repositories/devicedefinition/devicedefinition.go

+	var errList gqlerror.List
+	var endCur, startCur *string
+	//if len(all) != 0 {
+	//	//ec := helpers.IDToCursor(all[len(all)-1].ID)


Please remove comment

0xdev22 · 2024-04-24T17:33:40Z

internal/services/cache_manufacturer_service.go

+
+	manufacturers, err := models.Manufacturers().All(ctx, m.PDB.DBS().Reader)
+	if err != nil {
+		if errors.Is(err, sql.ErrNoRows) {


I don't think .All() returns this error (I'm not sure). You should rather check if the returned manufacturers variable is empty

0xdev22 · 2024-04-24T17:40:46Z

internal/services/cache_manufacturer_service_test.go

+	suite.Run(t, new(ManufacturerCacheServiceTestSuite))
+}
+
+func (o *ManufacturerCacheServiceTestSuite) Test_Manufacturer_All_Success() {


Two tests that I see missing here

Since you want to return an error when manufacturers are not found, can you write a test for that scenario too.

Can we verify the cache data is actually being returned early as implemented here

elffjs · 2024-05-02T01:59:41Z

Will take a look tomorrow. Sorry about the delay.

…tableland

elffjs · 2024-05-06T03:10:12Z

.gitignore

@@ -23,3 +23,5 @@ settings.yaml

 .idea

+# Desktop Service Store (macOS)
+.DS_Store


I'll do it.

elffjs · 2024-05-06T03:14:57Z

graph/schema/devicedefinition.graphqls

+  """
+  Device Definition ID for this device definition.
+  """
+  deviceDefinitionID: String


I would do deviceDefinitionId. See "tokenId" elsewhere.

elffjs · 2024-05-06T03:15:04Z

graph/schema/devicedefinition.graphqls

+  """
+  Legacy ID for this device definition.
+  """
+  legacyID: String


Similarly, legacyId.

elffjs · 2024-05-06T03:16:18Z

internal/repositories/devicedefinition/devicedefinition_test.go

+//const xmigrationsDir = "../../../migrations"
+
+func TestGetDeviceDefinition(t *testing.T) {
+	//ctx := context.Background()


Still curious about this!

elffjs · 2024-05-06T03:17:20Z

internal/services/tableland_api_service.go

+	"github.com/rs/zerolog"
+)
+
+type TablelandApiService struct {


I would name this TablelandAPIService and so one. Go casing.

elffjs · 2024-05-06T03:21:34Z

graph/resolver.go

 }

 // NewResolver creates a new Resolver with allocated repositories.
 func NewResolver(baseRepo *base.Repository) *Resolver {
+


I will run around deleting these newlines.

elffjs · 2024-05-06T03:22:14Z

internal/helpers/helpers.go

@@ -44,3 +53,17 @@ func GetAftermarketDeviceImageUrl(baseURL string, tokenID int) string {
 func GetVehicleDataURI(baseURL string, tokenID int) string {
 	return fmt.Sprintf("%s%d", baseURL, tokenID)
 }
+
+func SlugString(term string) string {


Maybe some tests would be good too.

elffjs · 2024-05-06T03:23:02Z

internal/repositories/devicedefinition/devicedefinition.go

+
+	manufacturerSlug := splitID[0]
+
+	manufactures, err := r.ManufacturerCacheService.GetAllManufacturers(ctx)


I will fix these for you.

elffjs · 2024-05-06T03:26:40Z

internal/repositories/devicedefinition/devicedefinition.go

+
+	manufacturerSlug := splitID[0]
+
+	manufacturers, err := r.ManufacturerCacheService.GetAllManufacturers(ctx)


Why do we need to pull and cache these? We have a manufacturers table—can we store the information we need there, using chain events?

Okay, one call you're making is to getManufacturerIdByName, but we already have this information in the manufacturers table. Let me think about the other part.

elffjs · 2024-05-06T03:37:26Z

graph/schema/devicedefinition.graphqls

+  """
+  The manufacturer for the device definition.
+  """
+  manufacturer: String!


Still curious about this. I think this will prove difficult to use.

If we're requiring the manufacturer, maybe we make this a sub-query of the Manufacturer object?

elffjs · 2024-05-06T05:06:02Z

internal/repositories/devicedefinition/devicedefinition.go

+		return nil, err
+	}
+
+	fmt.Print(statement)


Let's turn this into a real log statement, or delete it, something.

elffjs · 2024-05-06T05:06:57Z

internal/repositories/devicedefinition/devicedefinition.go

+
+	fmt.Print(statement)
+
+	var cont = 1


Aren't we using this to construct the "global id" on line 49, and it's always 1? That seems odd.

elffjs · 2024-05-06T05:07:18Z

internal/repositories/devicedefinition/devicedefinition.go

+	fmt.Print(statement)
+
+	var cont = 1
+	for _, item := range modelTableland {


We expect this to always have one element in it? This is a bit hard to follow.

@deprecated

* Reinstate and populate Vehicle.image as a @deprecated field * Regular expressions are great * Fix one image

* Present this in the API if we have it * Remove old "Definition URI" that we never used

internal/repositories/vehicle/vehicles.go

…tableland

ricardovivanco added 4 commits April 5, 2024 08:30

Resolve Device Definition from tableland

97fd6f6

Resolve Device Definition from tableland

a7f19e2

Resolve Device Definition from tableland

6e8eb52

Resolve Device Definition from tableland

02fe995

KevinJoiner requested changes Apr 5, 2024

View reviewed changes

ricardovivanco added 4 commits April 9, 2024 11:48

Remove vendor folder

044ced4

Add Registry file

8cc7f90

Fix comment

e6f20ee

Fix lint

a6ad53d

rhvivancoeffio marked this pull request as ready for review April 24, 2024 12:42

rhvivancoeffio requested review from elffjs, Allyson-English and 0xdev22 as code owners April 24, 2024 12:42

elffjs requested changes Apr 24, 2024

View reviewed changes

0xdev22 suggested changes Apr 24, 2024

View reviewed changes

ricardovivanco added 3 commits April 29, 2024 21:47

Fix comment

63715ea

Update variables

3c4cb68

Add RowIndex

6adfc57

elffjs added 4 commits May 5, 2024 23:10

Merge branch 'main' into feature/identity-api-devicedefinition-query-…

569ffdd

…tableland

Newline ree

8a6a701

Newline, tidy

8355f08

Fix spelling of manufacturers

15fdf36

elffjs requested changes May 6, 2024

View reviewed changes

Newlines ree

6102375

elffjs reviewed May 6, 2024

View reviewed changes

github-actions bot and others added 18 commits May 15, 2024 23:46

Update Image Version to 6b20c2c

9724c15

Reinstate and populate Vehicle.image as a @deprecated field (#105)

336526c

* Reinstate and populate Vehicle.image as a @deprecated field * Regular expressions are great * Fix one image

Update Image Version to 0fb6617

62ddc3f

Handle vehicle minting with device definition (#106)

0eb0631

* Present this in the API if we have it * Remove old "Definition URI" that we never used

Update Image Version to 111d124

752d32c

Tidy

1c9c43e

Update Image Version to a510625

4e1bce7

Resolve Device Definition from tableland

2af9050

Resolve Device Definition from tableland

7c6152a

Resolve Device Definition from tableland

4a73495

Remove vendor folder

0e6e1f3

Fix comment

b4b65c3

Fix comment

72cea4d

Add RowIndex

525ae04

Newline, tidy

6fe79bf

Use shared slugger

8273f69

Rip rip rip

05a5c02

rebase

6932154

Allyson-English reviewed May 16, 2024

View reviewed changes

internal/repositories/vehicle/vehicles.go Outdated Show resolved Hide resolved

Allyson-English reviewed May 16, 2024

View reviewed changes

internal/repositories/vehicle/vehicles.go Outdated Show resolved Hide resolved

Allyson-English reviewed May 16, 2024

View reviewed changes

internal/repositories/vehicle/vehicles.go Outdated Show resolved Hide resolved

elffjs added 7 commits May 16, 2024 09:49

Merge branch 'main' into feature/identity-api-devicedefinition-query-…

ad0bdf8

…tableland

Merge conflict in sum

374ec0a

Another one

8f6a425

Remove package downgrades

c86f386

Regenerate, remove unneeded service

b722c79

Imports merge conflict

9297d69

Just get rid of this test for now

1b833f5

JamesReate merged commit 5f5caf8 into main May 23, 2024
2 checks passed

JamesReate deleted the feature/identity-api-devicedefinition-query-tableland branch May 23, 2024 15:34

-	result.ID = v.ID
+    globalID, err := base.EncodeGlobalTokenID(TokenPrefix, v.ID)
+	if err != nil {
+		return nil, fmt.Errorf("error encoding device definition id: %w", err)
+	}
+	result.ID = globalID
+	result.DefinitionID = v.ID

		contractAddress := common.HexToAddress(m.settings.DIMORegistryAddr)
		queryInstance, err := contracts.NewRegistry(contractAddress, m.client)


		manufacturerSlug := splitID[0]

		manufactures, err := r.ManufacturerCacheService.GetAllManufacturers(ctx)


		manufacturerSlug := splitID[0]

		manufacturers, err := r.ManufacturerCacheService.GetAllManufacturers(ctx)

SI-2544 - Feature/identity api devicedefinition query tableland #88

SI-2544 - Feature/identity api devicedefinition query tableland #88

Conversation

rhvivancoeffio commented Apr 5, 2024

linear bot commented Apr 5, 2024

KevinJoiner 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

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

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

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

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

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

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

elffjs commented May 2, 2024

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

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

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