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

Sinner: resolvers and updating db read functions #1514

Merged
merged 8 commits into from
Oct 30, 2023

Conversation

nautsimon
Copy link
Contributor

@nautsimon nautsimon commented Oct 26, 2023

Summary by CodeRabbit

New Features:

  • Introduced GetOriginInfos and GetDestinationInfos functions to retrieve multiple origin and destination information respectively, enhancing data retrieval capabilities.

Refactor:

  • Updated the parameters for several functions and queries, replacing txHash and chainID with messageHash for single record retrieval and vice versa for multiple records, improving the specificity of data queries.
  • Modified the OriginSent and Executed database models, changing the primary key to MessageHash and adding an index to ChainID, optimizing database operations.

New Feature:

  • Added dbToGraphqlModelOrigin and dbToGraphqlModelDestination functions to convert database models to GraphQL models, improving data handling between different systems.

Refactor:

  • Updated the DestinationInfo and OriginInfo GraphQL models, adding new fields and changing the types of several existing fields, enhancing the granularity of data representation.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2023

Walkthrough

The changes primarily revolve around the shift from using txHash and chainID to messageHash as the primary key in various functions and database schemas. New functions have been added to retrieve multiple records based on txHash and chainID. The GraphQL queries and resolvers have been updated accordingly, and new types have been introduced to handle the changes.

Changes

File(s) Summary
services/sinner/api/resolver_test.go
services/sinner/graphql/client/client.go
services/sinner/graphql/server/graph/queries.resolvers.go
Updated test cases and GraphQL client and server to use messageHash instead of txHash and chainID. Added new functions to retrieve multiple records.
services/sinner/contracts/origin/parser.go Updated ParseSent function to handle iFace.Message as a byte array instead of a slice.
services/sinner/db/event.go
services/sinner/db/mocks/event_db.go
services/sinner/db/model/model.go
services/sinner/db/read_test.go
services/sinner/db/sql/base/read.go
Updated database functions and models to use messageHash as the primary key. Added new functions to retrieve multiple records based on txHash and chainID.
services/sinner/graphql/client/queries/queries.graphql
services/sinner/graphql/server/graph/schema/queries.graphql
services/sinner/graphql/server/graph/schema/types.graphql
Updated GraphQL queries to use messageHash and added new queries to retrieve multiple records. Introduced new types to handle the changes.
services/sinner/graphql/server/graph/helper.go
services/sinner/graphql/server/graph/model/models_gen.go
services/sinner/graphql/server/graph/types.resolvers.go
Added new functions to convert database models to GraphQL models. Updated existing models and added new resolvers to handle the changes.
services/sinner/service/indexer.go Removed an empty line.

"In the land of code, where logic is king, 🤴

Changes were made, oh what joy they bring! 🎉

From txHash and chainID, we did part, 👋

messageHash now plays the starring part. 🌟

New functions added, with a graceful twirl, 🌀

Unveiling a world, where multiple records unfurl. 🌐"

  • CodeRabbit 🐇

Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai help to get help.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

@nautsimon nautsimon changed the base branch from master to feat/sinner October 26, 2023 15:05
@github-actions github-actions bot added go Pull requests that update Go code size/l labels Oct 26, 2023
@nautsimon nautsimon marked this pull request as ready for review October 26, 2023 15:05
@nautsimon nautsimon requested a review from trajan0x as a code owner October 26, 2023 15:05
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (839902c) 50.83841% compared to head (f1fc2e9) 49.62568%.
Report is 2 commits behind head on feat/sinner.

Additional details and impacted files
@@                  Coverage Diff                  @@
##           feat/sinner       #1514         +/-   ##
=====================================================
- Coverage     50.83841%   49.62568%   -1.21274%     
=====================================================
  Files              369         366          -3     
  Lines            25465       24578        -887     
  Branches           268         268                 
=====================================================
- Hits             12946       12197        -749     
- Misses           11257       11311         +54     
+ Partials          1262        1070        -192     
Flag Coverage Δ
cctp-relayer 63.33333% <ø> (-1.05692%) ⬇️
core 58.76250% <ø> (+0.81511%) ⬆️
git-changes-action 53.94265% <ø> (ø)
omnirpc 53.19635% <ø> (ø)
promexporter 41.02564% <ø> (ø)
release-copier-action 19.33333% <ø> (ø)
scribe 52.02703% <ø> (ø)
sinner 14.42623% <0.00000%> (+3.31512%) ⬆️
tools 21.72452% <ø> (ø)

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

Files Coverage Δ
services/sinner/service/indexer.go 0.00000% <ø> (ø)
services/sinner/api/server.go 51.76471% <0.00000%> (+4.00352%) ⬆️

... and 16 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 20e23da and b0b3047.
Files selected for processing (17)
  • services/sinner/api/resolver_test.go (2} hunks)
  • services/sinner/contracts/origin/parser.go (1} hunks)
  • services/sinner/db/event.go (1} hunks)
  • services/sinner/db/mocks/event_db.go (2} hunks)
  • services/sinner/db/model/model.go (3} hunks)
  • services/sinner/db/read_test.go (2} hunks)
  • services/sinner/db/sql/base/read.go (3} hunks)
  • services/sinner/graphql/client/client.go (8} hunks)
  • services/sinner/graphql/client/queries/queries.graphql (2} hunks)
  • services/sinner/graphql/server/graph/helper.go (2} hunks)
  • services/sinner/graphql/server/graph/model/models_gen.go (2} hunks)
  • services/sinner/graphql/server/graph/queries.resolvers.go (1} hunks)
  • services/sinner/graphql/server/graph/resolver/server.go (41} hunks)
  • services/sinner/graphql/server/graph/schema/queries.graphql (1} hunks)
  • services/sinner/graphql/server/graph/schema/types.graphql (3} hunks)
  • services/sinner/graphql/server/graph/types.resolvers.go (1} hunks)
  • services/sinner/service/indexer.go (1} hunks)
Files not summarized due to errors (1)
  • services/sinner/graphql/server/graph/resolver/server.go: Error: Message exceeds token limit
Files skipped from review due to trivial changes (4)
  • services/sinner/contracts/origin/parser.go
  • services/sinner/db/sql/base/read.go
  • services/sinner/graphql/server/graph/helper.go
  • services/sinner/service/indexer.go
Additional comments: 50
services/sinner/graphql/server/graph/schema/queries.graphql (4)
  • 7-8: The getOriginInfo function signature has been updated to take messageHash as a parameter instead of txHash and chainID. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the messageHash is being correctly generated and passed to this function.

  • 10-12: The getDestinationInfo function signature has been updated in a similar manner to getOriginInfo. The same considerations apply here.

  • 14-17: The new getOriginInfos function retrieves multiple OriginInfo records based on txHash and chainID. Ensure that the function implementation correctly handles the retrieval of multiple records and that the returned data is correctly processed by the caller.

  • 19-22: The new getDestinationInfos function retrieves multiple DestinationInfo records based on txHash and chainID. The same considerations as for getOriginInfos apply here.

services/sinner/db/model/model.go (3)
  • 67-67: The TxHash field is no longer a primary key in the OriginSent struct. This change is consistent with the PR summary, which mentions that MessageHash is now the primary key. Ensure that this change is reflected in all related database operations and queries.

  • 79-79: The MessageHash field is now a primary key in the OriginSent struct. This change is consistent with the PR summary. Ensure that this change is reflected in all related database operations and queries.

  • 117-121: The TxHash field is no longer a primary key in the Executed struct, and MessageHash is now the primary key. This change is consistent with the PR summary. Ensure that this change is reflected in all related database operations and queries.

services/sinner/graphql/client/queries/queries.graphql (4)
  • 14-37: The GetOriginInfo query has been updated to take messageHash as a parameter instead of txHash and chainID. This change aligns with the PR's goal of refactoring the handling of transaction and message hashes. Ensure that all calls to this query throughout the codebase have been updated to match the new signature.

  • 39-40: The GetOriginInfos query has been introduced to retrieve multiple records based on txHash and chainID. This is a new addition and does not replace any existing functionality. Ensure that this new query is used appropriately in the codebase.

  • 66-77: The GetDestinationInfo query has been updated to take messageHash as a parameter instead of txHash and chainID. This change aligns with the PR's goal of refactoring the handling of transaction and message hashes. Ensure that all calls to this query throughout the codebase have been updated to match the new signature.

  • 80-81: The GetDestinationInfos query has been introduced to retrieve multiple records based on txHash and chainID. This is a new addition and does not replace any existing functionality. Ensure that this new query is used appropriately in the codebase.

services/sinner/graphql/server/graph/schema/types.graphql (4)
  • 11-13: The comment for the OriginInfo type has been updated to specify that it provides information on an origin sent event. This change is more descriptive and provides better context for the type.

  • 22-34: The types of several fields in the OriginInfo type have been changed from String to Int. This includes originChainID, destinationChainID, optimisticSeconds, and messageFlag. This change is likely due to the underlying data for these fields being numerical. Ensure that these changes are reflected in the corresponding database schema and any code that interacts with these fields.

  • 35-36: Two new fields, messageStatus and destinationInfo, have been added to the OriginInfo type. These fields are linked to the MessageStatus and DestinationInfo types respectively using the @goField directive. This change allows for more detailed information to be retrieved when querying for OriginInfo.

  • 51-52: Two new fields, messageStatus and originInfo, have been added to the DestinationInfo type. These fields are linked to the MessageStatus and OriginInfo types respectively using the @goField directive. This change allows for more detailed information to be retrieved when querying for DestinationInfo.

services/sinner/graphql/server/graph/queries.resolvers.go (4)
  • 26-33: The function signature for GetOriginInfo has been updated to take a messageHash instead of txHash and chainID. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the RetrieveOriginSent function has been updated to handle messageHash as an argument.

  • 36-43: The function signature for GetDestinationInfo has been updated to take a messageHash instead of txHash and chainID. Ensure that all calls to this function throughout the codebase have been updated to match the new signature. Also, verify that the RetrieveExecuted function has been updated to handle messageHash as an argument.

  • 46-56: The new function GetOriginInfos retrieves multiple records based on txHash and chainID. Ensure that the RetrieveOriginSents function is implemented correctly to handle these parameters and return the expected results. Also, verify that the dbToGraphqlModelOrigin function can handle multiple records.

  • 59-70: The new function GetDestinationInfos retrieves multiple records based on txHash and chainID. Ensure that the RetrieveExecuteds function is implemented correctly to handle these parameters and return the expected results. Also, verify that the dbToGraphqlModelDestination function can handle multiple records.

services/sinner/db/mocks/event_db.go (4)
  • 26-45: The RetrieveExecuted function has been updated to take a messageHash instead of chainID and txHash. This change aligns with the updated database schema where MessageHash is now the primary key. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

  • 47-59: The RetrieveExecuteds function is a new addition that retrieves multiple executed events based on the provided chainID and txHash. This function could be useful in scenarios where multiple events are associated with the same transaction. Ensure that the function is used appropriately in the codebase.

  • 112-130: The RetrieveOriginSent function has been updated to take a messageHash instead of chainID and txHash. This change aligns with the updated database schema where MessageHash is now the primary key. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

  • 133-145: The RetrieveOriginSents function is a new addition that retrieves multiple origin sent events based on the provided chainID and txHash. This function could be useful in scenarios where multiple events are associated with the same transaction. Ensure that the function is used appropriately in the codebase.

services/sinner/graphql/server/graph/model/models_gen.go (2)
  • 13-22: The DestinationInfo struct has been updated with two new fields: MessageStatus and OriginInfo. These fields provide additional context about the status of a message and its origin, respectively. Ensure that these fields are populated correctly wherever this struct is used. Also, verify that the GraphQL schema has been updated to reflect these changes.

  • 35-55: The OriginInfo struct has been significantly updated. The types of OriginChainID and DestinationChainID have been changed from *string to *int. This change should be verified across the codebase to ensure compatibility. The OptimisticSeconds and MessageFlag fields have also been changed from *string to *int, which should be more appropriate for these fields.

New fields MessageStatus and DestinationInfo have been added. These fields provide additional context about the status of a message and its destination, respectively. Ensure that these fields are populated correctly wherever this struct is used. Also, verify that the GraphQL schema has been updated to reflect these changes.

Lastly, the fields SummitTip, AttestationTip, ExecutionTip, and DeliveryTip have been added. Ensure that these fields are populated correctly wherever this struct is used.

services/sinner/graphql/server/graph/types.resolvers.go (6)
  • 16-23: The MessageStatus resolver function retrieves the message status from the database using the MessageHash field of the DestinationInfo model. The error handling is done correctly, wrapping the original error with context. This is a good practice as it helps in debugging if an error occurs.

  • 26-32: The OriginInfo resolver function retrieves the origin sent data from the database using the MessageHash field of the DestinationInfo model. The error handling is done correctly, wrapping the original error with context. The function dbToGraphqlModelOrigin is used to convert the database model to the GraphQL model. Ensure that this function handles all necessary conversions and edge cases.

  • 34-42: The MessageStatus resolver function for the OriginInfo model retrieves the message status from the database using the MessageHash field of the OriginInfo model. The error handling is done correctly, wrapping the original error with context.

  • 44-51: The DestinationInfo resolver function retrieves the executed data from the database using the MessageHash field of the OriginInfo model. The error handling is done correctly, wrapping the original error with context. The function dbToGraphqlModelDestination is used to convert the database model to the GraphQL model. Ensure that this function handles all necessary conversions and edge cases.

  • 54-59: The DestinationInfo and OriginInfo functions return the corresponding resolver implementations. This is a standard practice in GraphQL to provide a way to resolve specific fields on types.

  • 61-62: The destinationInfoResolver and originInfoResolver types are defined, embedding the Resolver type. This is a common pattern in Go and GraphQL to provide methods on these types that can resolve the corresponding fields.

services/sinner/db/event.go (1)
  • 34-41: The RetrieveOriginSent and RetrieveExecuted methods have been updated to retrieve records based on messageHash instead of chainID and txHash. This change aligns with the updated database schema where MessageHash is now the primary key.

New methods RetrieveOriginSents and RetrieveExecuteds have been added to retrieve multiple records based on chainID and txHash. This could be useful in scenarios where multiple events are tied to a single transaction.

Ensure that all calls to these methods throughout the codebase have been updated to match the new signatures and that the new methods are used appropriately.

---new hunk for review---

//
//nolint:interfacebloat
type EventDBWriter interface {
47:
	// StoreOriginSent stores an OriginSent event.
48:
	StoreOriginSent(ctx context.Context, event model.OriginSent) error
49:
	// StoreExecuted stores an Executed event.
50:
	StoreExecuted(ctx context.Context, event model.Executed) error
51:
	// UpdateMessageStatus updates the status of a message.
52:
	UpdateMessageStatus(ctx context.Context, messageHash string, status graphqlModel.MessageStatus) error
53:
	// StoreLastStoredBlock stores the last block stored in sinner.
54:
	StoreLastStoredBlock(ctx context.Context, chainID uint32, address common.Address, blockNumber uint64) error
}

// EventDB combines EventDBReader and EventDBWriter.

---old hunk for context---

//
//nolint:interfacebloat
type EventDBWriter interface {
	// StoreOriginSent stores an OriginSent event.
	StoreOriginSent(ctx context.Context, event model.OriginSent) error
	// StoreExecuted stores an Executed event.
	StoreExecuted(ctx context.Context, event model.Executed) error
	// UpdateMessageStatus updates the status of a message.
	UpdateMessageStatus(ctx context.Context, messageHash string, status graphqlModel.MessageStatus) error
	// StoreLastStoredBlock stores the last block stored in sinner.
	StoreLastStoredBlock(ctx context.Context, chainID uint32, address common.Address, blockNumber uint64) error
}

// EventDB combines EventDBReader and EventDBWriter.

---end hunk---

services/sinner/graphql/server/graph/resolver/server.go (9)
  • 39-40: The ResolverRoot interface has been extended with two new methods: DestinationInfo() and OriginInfo(). Ensure that all implementations of this interface have been updated to include these new methods.

  • 74-75: New functions MessageStatus and OriginInfo have been added to the DestinationInfo and OriginInfo interfaces respectively. Make sure these functions are implemented wherever these interfaces are used.

  • 93-98: The Query struct has been extended with new functions GetDestinationInfo, GetDestinationInfos, GetMessageStatus, GetOriginInfo, and GetOriginInfos. Ensure that these functions are implemented and used correctly.

  • 101-108: New interfaces DestinationInfoResolver and OriginInfoResolver have been introduced. Make sure these interfaces are implemented correctly wherever they are used.

  • 314-320: New cases have been added to handle DestinationInfo.messageStatus, DestinationInfo.originInfo, OriginInfo.destinationInfo, and OriginInfo.messageStatus. Ensure that these cases are handled correctly in the switch statement.

  • 423-435: New cases have been added to handle Query.getDestinationInfo, Query.getDestinationInfos, Query.getOriginInfo, and Query.getOriginInfos. Ensure that these cases are handled correctly in the switch statement.

  • 542-557: The GraphQL schema has been updated with new queries getOriginInfo, getDestinationInfo, getOriginInfos, and getDestinationInfos. Make sure these queries are implemented and used correctly.

  • 580-605: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [572-598]

The OriginInfo and DestinationInfo types in the GraphQL schema have been updated with new fields messageStatus and destinationInfo/originInfo respectively. Ensure that these fields are handled correctly in the GraphQL resolvers.

  • 1103-1237: New resolvers have been added for DestinationInfo.messageStatus, DestinationInfo.originInfo, OriginInfo.messageStatus, and OriginInfo.destinationInfo. Make sure these resolvers are implemented correctly and return the expected data.

1672, 1713:
The return type of the OriginInfo.originChainID and OriginInfo.destinationChainID fields has been changed from string to int. Ensure that this change is reflected in all places where these fields are used.

services/sinner/api/resolver_test.go (2)
  • 30-38: The test TestGetOrigin() has been updated to test the new GetOriginInfo() function that takes a messageHash instead of txHash and chainID. It also tests the new GetOriginInfos() function. Ensure that the messageHash is correctly generated and used in these tests. Also, consider adding a check to verify the length of the results slice to ensure that the GetOriginInfos() function is returning the correct number of results.
+	Equal(t.T(), txHash, *result.Response.OriginTxHash)
+	results, err := t.sinnerAPI.GetOriginInfos(t.GetTestContext(), txHash, int(chainID))
+	Nil(t.T(), err)
+	NotNil(t.T(), results)
+	Equal(t.T(), txHash, *results.Response[0].OriginTxHash)
Committable suggestion (Beta)
	result, err := t.sinnerAPI.GetOriginInfo(t.GetTestContext(), messageHash)
	Nil(t.T(), err)
	NotNil(t.T(), result)
	Equal(t.T(), txHash, *result.Response.OriginTxHash)

	results, err := t.sinnerAPI.GetOriginInfos(t.GetTestContext(), txHash, int(chainID))
	Nil(t.T(), err)
	NotNil(t.T(), results)
	Equal(t.T(), 1, len(results.Response))
	Equal(t.T(), txHash, *results.Response[0].OriginTxHash)
  • 59-67: The test TestGetExecuted() has been updated to test the new GetDestinationInfo() function that takes a messageHash instead of txHash and chainID. It also tests the new GetDestinationInfos() function. Ensure that the messageHash is correctly generated and used in these tests. Also, consider adding a check to verify the length of the results slice to ensure that the GetDestinationInfos() function is returning the correct number of results.
+	Equal(t.T(), txHash, *result.Response.TxHash)
+	results, err := t.sinnerAPI.GetDestinationInfos(t.GetTestContext(), txHash, int(chainID))
+	Nil(t.T(), err)
+	NotNil(t.T(), results)
+	Equal(t.T(), txHash, *results.Response[0].TxHash)
Committable suggestion (Beta)
	result, err := t.sinnerAPI.GetDestinationInfo(t.GetTestContext(), messageHash)
	Nil(t.T(), err)
	NotNil(t.T(), result)
	Equal(t.T(), txHash, *result.Response.TxHash)

	results, err := t.sinnerAPI.GetDestinationInfos(t.GetTestContext(), txHash, int(chainID))
	Nil(t.T(), err)
	NotNil(t.T(), results)
	Equal(t.T(), 1, len(results.Response))
	Equal(t.T(), txHash, *results.Response[0].TxHash)
services/sinner/graphql/client/client.go (7)
  • 22-26: The Query struct has been updated to include two new fields: GetOriginInfos and GetDestinationInfos. These fields correspond to the new GraphQL queries for retrieving multiple records based on the chain ID and transaction hash. The types of these fields are slices of pointers to model.OriginInfo and model.DestinationInfo, respectively, indicating that the queries return multiple records.

  • 61-72: The GetOriginInfo and GetOriginInfos structs have been updated with new fields and type changes. The OriginChainID, DestinationChainID, OptimisticSeconds, MessageFlag, Version, and GasLimit fields have been changed from *string to *int. This change aligns the types with the expected data types in the GraphQL schema. The MessageHash field has been added to both structs, reflecting the change in the primary key from TxHash to MessageHash in the OriginSent table.

  • 94-103: A new struct GetDestinationInfos has been added to handle the response from the getDestinationInfos GraphQL query. This struct is similar to GetDestinationInfo but is designed to handle multiple records in the response.

  • 155-157: The GetOriginInfo function and the corresponding GraphQL query document have been updated to replace the txHash and chainID parameters with messageHash. This change reflects the update in the primary key from TxHash to MessageHash in the OriginSent table.

  • 193-196: New GraphQL query documents and functions have been added for the getOriginInfos and getDestinationInfos queries. These queries retrieve multiple records from the OriginSent and Executed tables based on the chain ID and transaction hash. The functions take a context, transaction hash, chain ID, and optional HTTP request options, and return the corresponding response struct.

  • 221-223: The GetDestinationInfo function and the corresponding GraphQL query document have been updated to replace the txHash and chainID parameters with messageHash. This change reflects the update in the primary key from TxHash to MessageHash in the Executed table.

  • 248-251: New GraphQL query documents and functions have been added for the getDestinationInfos queries. These queries retrieve multiple records from the Executed table based on the chain ID and transaction hash. The functions take a context, transaction hash, chain ID, and optional HTTP request options, and return the corresponding response struct.

services/sinner/db/read_test.go Show resolved Hide resolved
services/sinner/db/read_test.go Show resolved Hide resolved
// TxIndex is the index of the tx in a block.
TxIndex uint `gorm:"column:tx_index"`
// MessageHash is the message hash.
MessageHash string `gorm:"column:message_hash"`
MessageHash string `gorm:"column:message_hash;primaryKey"`
Copy link
Contributor

Choose a reason for hiding this comment

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

should be a foreign key constraint to message table

Copy link
Contributor Author

@nautsimon nautsimon Oct 27, 2023

Choose a reason for hiding this comment

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

Is this for data integrity if deleting from the db? currently the messagestatus table is updated concurrently w/origin/destination table insert while indexing.

An idea could be to do a has many relationship, but the idea of the message status table was to be a lite as possible on query side so it could be pinged a lot for quick updates.

type MessageStatus struct {
	MessageHash        string `gorm:"column:message_hash;uniqueIndex:idx_message_hash_status"`
	OriginTxHash       string `gorm:"column:origin_txhash"`
	DestinationTxHash  string `gorm:"column:destination_txhash"`
	OriginSentMessages []OriginSent `gorm:"foreignKey:MessageHash;references:MessageHash;constraint:OnUpdate:CASCADE,OnDelete:SET NULL;"`
	ExecutedMessages   []Executed   `gorm:"foreignKey:MessageHash;references:MessageHash;constraint:OnUpdate:CASCADE,OnDelete:SET NULL;"`
}

Copy link
Contributor

Choose a reason for hiding this comment

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

has many would be the idea yeah, this will also speed up joins, etc

Copy link
Contributor

Choose a reason for hiding this comment

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

destination tx hash is many to may

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between b0b3047 and 02dd707.
Files selected for processing (1)
  • services/sinner/db/sql/base/read.go (3} hunks)
Additional comments: 3
services/sinner/db/sql/base/read.go (3)
  • 59-106: The RetrieveOriginSent(), RetrieveOriginSents(), RetrieveExecuted(), and RetrieveExecuteds() functions have been updated to use messageHash instead of chainID and txHash. This change aligns with the updated database schema where MessageHash is now the primary key. Ensure that all calls to these functions throughout the codebase have been updated to match the new signatures. Also, verify that the messageHash is being correctly generated and used in the upstream code.

  • 72-83: The new function RetrieveOriginSents() has been added to retrieve multiple OriginSent records based on chainID and txHash. This function can be useful when there are multiple OriginSent events associated with a single transaction. However, it's important to note that this function may return a large number of records for transactions with many events, which could impact performance. Consider adding pagination or limiting the number of records returned if this becomes an issue.

  • 98-106: The new function RetrieveExecuteds() has been added to retrieve multiple Executed records based on chainID and txHash. Similar to the RetrieveOriginSents() function, this function may return a large number of records for transactions with many executed events. Consider adding pagination or limiting the number of records returned if this becomes an issue.

services/sinner/db/sql/base/read.go Outdated Show resolved Hide resolved
services/sinner/db/sql/base/read.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 02dd707 and 0bc0194.
Files selected for processing (1)
  • services/sinner/db/sql/base/read.go (3} hunks)
Files skipped from review due to trivial changes (1)
  • services/sinner/db/sql/base/read.go

@trajan0x trajan0x mentioned this pull request Oct 27, 2023
services/sinner/db/event.go Show resolved Hide resolved
// TxIndex is the index of the tx in a block.
TxIndex uint `gorm:"column:tx_index"`
// MessageHash is the message hash.
MessageHash string `gorm:"column:message_hash"`
MessageHash string `gorm:"column:message_hash;primaryKey"`
Copy link
Contributor

Choose a reason for hiding this comment

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

destination tx hash is many to may

services/sinner/db/mocks/event_db.go Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 0bc0194 and 3ee3f7c.
Files ignored due to filter (1)
  • services/sinner/go.mod
Files selected for processing (1)
  • services/sinner/db/event.go (1 hunks)
Additional comments: 1
services/sinner/db/event.go (1)
  • 33-46: The changes to the EventDBReader interface look good. The new methods RetrieveOriginSents and RetrieveExecuteds have been added correctly and the existing methods RetrieveOriginSent and RetrieveExecuted have been updated to use messageHash instead of chainID and txHash. Ensure that all implementations of this interface have been updated to reflect these changes.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 3ee3f7c and f1fc2e9.
Files selected for processing (1)
  • services/sinner/api/server.go (2 hunks)
Additional comments: 8
services/sinner/api/server.go (8)
  • 24-25: The introduction of errNoPort is a good practice for error handling. It provides a clear and specific error message when the port is not specified.

  • 33-34: The use of ginhelper.New(logger) to create a new router is good for maintainability and modularity. It abstracts the creation of a new router into a helper function, which can be reused in other parts of the code.

  • 2-8: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [36-41]

The error handling here is well done. If there is an error initializing the database, the function returns immediately with a wrapped error message. This is a good practice as it provides more context about the error.

  • 2-8: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [43-44]

The use of gqlServer.EnableGraphql(router, eventDB, cfg, handler) to enable GraphQL on the router is good for modularity. It abstracts the process of enabling GraphQL into a separate function, which can be reused in other parts of the code.

  • 2-8: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [46-47]

The creation of a new listener is well handled. If there is an error creating the listener, the function returns immediately with a wrapped error message. This is a good practice as it provides more context about the error.

  • 2-8: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [49-58]

The use of an error group to handle the serving of HTTP requests is a good practice. It allows for the handling of multiple goroutines that return an error. If any of the goroutines return an error, the error group will return that error.

  • 2-8: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [60-65]

The error handling here is well done. If there is an error with the server, the function returns immediately with a wrapped error message. This is a good practice as it provides more context about the error.

  • 2-8: > Note: This review was outside of the patch, but no patch was found that overlapped with it. Original lines [67-130]

The InitDB function is well structured and handles errors appropriately. It checks the database type and initializes the appropriate database accordingly. If there is an error at any point, it returns immediately with a wrapped error message. This is a good practice as it provides more context about the error.

Comment on lines +29 to +31
if cfg.HTTPPort == 0 {
return errNoPort
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The check for cfg.HTTPPort == 0 is a good addition. It ensures that the server does not start without a valid port. However, it might be beneficial to also check if the port is within the valid range (1-65535). If the port is outside this range, it should also return an error.

if cfg.HTTPPort <= 0 || cfg.HTTPPort > 65535 {
	return errNoPort
}

@trajan0x trajan0x merged commit 2a59e49 into feat/sinner Oct 30, 2023
37 of 38 checks passed
@trajan0x trajan0x deleted the feat/sinner-resolvers branch October 30, 2023 14:42
nautsimon added a commit that referenced this pull request Nov 2, 2023
* init

* stashing at prev checkpoint to resolve build issues

* updating graphql

* live filling updates

* stash prev changes before tests

* update queries w/tests

* update api and cli

* stash before deploy

* api tests

* lint

* lint

* lint + gen

* code cov

* gen

* adding code cov

* lint + test + coderabbit suggestions

* symlink

* edits

* hold

* Update go.mod

* sinner docker + main.go

* sinner test flake, bytes -> hex string update, update assert use, lint

* update logging

* deprecate port flag

* updating fetch ceil

* adding concurrency during parse store

* add comments

* gen (stringer)

* add build to .goreleaser.yml

* build

* Revert "build" [goreleaser]

This reverts commit 8c61076.

* fix goreleaser [goreleaser]

* fix replacement issue

* [sinner] add start readme (#1499)

* add start readme

* fix https://github.com/synapsecns/sanguine/pull/1380/files#r1371810612

---------

Co-authored-by: Trajan0x <[email protected]>

* sinner strings for contract_type (#1498)

Co-authored-by: Trajan0x <[email protected]>

* trajan edits

* nits

* explorer update

* Sinner: resolvers and updating db read functions (#1514)

* init staging

* add more db read functions

* update model to where

* update filters

* hash

* Revert "hash"

This reverts commit 9b159df.

* remove unused db mocks

* handle invalid port

---------

Co-authored-by: Trajan0x <[email protected]>

* type fix

* simplify w generics

* add go error group to unified command + updated read.go

* code rabbit nit

* logger fix

* Parser, API, and config updates

* tests, sinner/indexer updates, db updates

* lint + refactor graphql + db

* Add log range asc to API

* [goreleaser]

* Update README.md

* refactor indexer

* exit 1 on fail

* update scribe fetcher and pagination behavior

* mysql context

* mysql + refresh rate

* update config and readme + [goreleaser]

* update tests

* typo + [goreleaser]

* prom exporter gen

* revert abi - [goreleaser]

* [goreleaser]

* specify varchar length for indexing [goreleaser]

* update 64 to 66 to account for 0x [goreleaser]

* update api

* [goreleaser]

* gen

* update comment

* [goreleaser]

* add asc flag

* [goroutine]

* [goreleaser]

---------

Co-authored-by: Trajan0x <[email protected]>
Co-authored-by: trajan0x <[email protected]>
nautsimon added a commit that referenced this pull request Nov 4, 2023
* init

* stashing at prev checkpoint to resolve build issues

* updating graphql

* live filling updates

* stash prev changes before tests

* update queries w/tests

* update api and cli

* stash before deploy

* api tests

* lint

* lint

* lint + gen

* code cov

* gen

* adding code cov

* lint + test + coderabbit suggestions

* symlink

* edits

* hold

* Update go.mod

* sinner docker + main.go

* sinner test flake, bytes -> hex string update, update assert use, lint

* update logging

* deprecate port flag

* updating fetch ceil

* adding concurrency during parse store

* add comments

* gen (stringer)

* add build to .goreleaser.yml

* build

* Revert "build" [goreleaser]

This reverts commit 8c61076.

* fix goreleaser [goreleaser]

* fix replacement issue

* [sinner] add start readme (#1499)

* add start readme

* fix https://github.com/synapsecns/sanguine/pull/1380/files#r1371810612

---------

Co-authored-by: Trajan0x <[email protected]>

* sinner strings for contract_type (#1498)

Co-authored-by: Trajan0x <[email protected]>

* trajan edits

* nits

* explorer update

* Sinner: resolvers and updating db read functions (#1514)

* init staging

* add more db read functions

* update model to where

* update filters

* hash

* Revert "hash"

This reverts commit 9b159df.

* remove unused db mocks

* handle invalid port

---------

Co-authored-by: Trajan0x <[email protected]>

* type fix

* simplify w generics

* add go error group to unified command + updated read.go

* code rabbit nit

* logger fix

* Parser, API, and config updates

* tests, sinner/indexer updates, db updates

* lint + refactor graphql + db

* Add log range asc to API

* [goreleaser]

* Update README.md

* refactor indexer

* exit 1 on fail

* update scribe fetcher and pagination behavior

* mysql context

* mysql + refresh rate

* update config and readme + [goreleaser]

* init

* mod lint

* omnirpc version bump

* chart bump

* Update values.yaml

* lint

* rm omnirpc dependacy

* update op rpc (ratelimited)

* rev

---------

Co-authored-by: Trajan0x <[email protected]>
Co-authored-by: trajan0x <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Pull requests that update Go code size/l
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants