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

Fix message construction #2802

Merged
merged 15 commits into from
Jun 27, 2024
Merged

Fix message construction #2802

merged 15 commits into from
Jun 27, 2024

Conversation

golangisfun123
Copy link
Collaborator

@golangisfun123 golangisfun123 commented Jun 27, 2024

Summary by CodeRabbit

  • New Features

    • Added font ligature settings to VS Code for enhanced readability.
    • Introduced a new function to convert a 32-bit array to a JSON string in the core package.
  • Documentation

    • Updated API documentation to reflect the removal of the data field from the JSON template.
  • Chores

    • Updated labeler configurations to include new file matching patterns for docs, VS Code settings, Sol, Typescript, and Javascript.
  • Refactor

    • Simplified data handling and structure for blacklisted addresses in the screener API.
    • Adjusted field ordering and naming conventions in database models for consistency.

Copy link
Contributor

coderabbitai bot commented Jun 27, 2024

Warning

Rate limit exceeded

@trajan0x has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 45 minutes and 20 seconds before requesting another review.

How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

Commits

Files that changed from the base of the PR and between 8646f0e and 02fcafa.

Walkthrough

Significant updates were made across various components, mainly within the screener-api module, focusing on structural and functional improvements. Key changes include the restructuring of the BlackListBody and BlacklistedAddress structs, updating database and API documentation files to align with these modifications, and simplifying the GenerateSignature function. Additionally, the .github/labeler.yml now includes new patterns for proper issue labeling, and VS Code settings have been updated to support font ligatures.

Changes

Files / Directories Change Summary
.vscode/settings.json Added font ligature settings.
contrib/screener-api/client/client.go Updated BlackListBody struct and simplified GenerateSignature function.
contrib/screener-api/db/models.go Reordered fields in BlacklistedAddress struct and adjusted gorm tags.
contrib/screener-api/db/sql/base/base.go, contrib/screener-api/db/sql/base/namer.go Updated field names and modified column assignment logic.
contrib/screener-api/screener/screener.go Significant changes in handling blacklistAddress, field access, logging, and data processing.
core/bytes.go Added BytesToJSONString function for converting byte arrays to JSON strings.
.github/labeler.yml Added file matching patterns for new labels: M-docs, M-vscode-settings, Sol, Typescript, Javascript.
contrib/screener-api/docs/docs.go, contrib/screener-api/docs/swagger.json, contrib/screener-api/docs/swagger.yaml Removed data field from JSON template and API definitions.

Poem

In lines of code, changes flow,
New structures rise, simplified glow.
Data nested, tags refined,
HMAC signatures streamlined.
🎨 Labels clear in the repo's hall,
🛠️ Better docs stand tall.
Coders smile, changes made,
To a better code, we've paved the way. 🚀


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

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 an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • 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/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added go Pull requests that update Go code size/s labels Jun 27, 2024
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

  • Restructured BlackListBody to include nested Data struct
  • Simplified GenerateSignature function
  • Corrected primary key annotation in BlacklistedAddress
  • Fixed column names in PutBlacklistedAddress
  • Improved message construction in blacklistAddress and authMiddleware functions

6 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 50.90909% with 27 lines in your changes missing coverage. Please review.

Project coverage is 25.59422%. Comparing base (08e2b19) to head (02fcafa).
Report is 17 commits behind head on master.

Files Patch % Lines
contrib/screener-api/screener/screener.go 65.62500% 8 Missing and 3 partials ⚠️
contrib/screener-api/client/client.go 0.00000% 8 Missing ⚠️
contrib/screener-api/db/sql/base/namer.go 0.00000% 4 Missing ⚠️
core/bytes.go 70.00000% 2 Missing and 1 partial ⚠️
contrib/screener-api/db/sql/base/base.go 0.00000% 1 Missing ⚠️
Additional details and impacted files
@@                 Coverage Diff                 @@
##              master       #2802         +/-   ##
===================================================
- Coverage   25.91883%   25.59422%   -0.32461%     
===================================================
  Files            739         758         +19     
  Lines          53274       54399       +1125     
  Branches          80          80                 
===================================================
+ Hits           13808       13923        +115     
- Misses         38044       39044       +1000     
- Partials        1422        1432         +10     
Flag Coverage Δ
agents 34.13462% <ø> (-0.11094%) ⬇️
cctp-relayer 31.93780% <ø> (ø)
core 61.42061% <70.00000%> (+0.02997%) ⬆️
ethergo 48.47467% <ø> (-0.21510%) ⬇️
explorer 4.97877% <ø> (ø)
git-changes-action 23.69615% <ø> (ø)
omnirpc 33.08129% <ø> (+0.04726%) ⬆️
opbot 0.22305% <ø> (ø)
promexporter 9.86717% <ø> (?)
rfq 27.16074% <ø> (+0.03911%) ⬆️
screener-api 30.50847% <46.66667%> (-0.51407%) ⬇️
scribe 18.11159% <ø> (ø)
tools 30.55118% <ø> (ø)

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

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

Copy link

cloudflare-workers-and-pages bot commented Jun 27, 2024

Deploying sanguine-fe with  Cloudflare Pages  Cloudflare Pages

Latest commit: c309995
Status: ✅  Deploy successful!
Preview URL: https://abd22541.sanguine-fe.pages.dev
Branch Preview URL: https://fix-data.sanguine-fe.pages.dev

View logs

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Modified message construction format in BlacklistAddress method for better readability.
  • Improved message handling in authMiddleware function.
  • Enhanced accuracy and reliability of signature verification.
  • Re-read request body for DB operations in authMiddleware.
  • Updated tagging for database models.

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

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.

Actionable comments posted: 2

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between bbe8dc3 and 7ffa8d0.

Files selected for processing (7)
  • .vscode/settings.json (1 hunks)
  • contrib/screener-api/client/client.go (2 hunks)
  • contrib/screener-api/db/models.go (1 hunks)
  • contrib/screener-api/db/sql/base/base.go (1 hunks)
  • contrib/screener-api/db/sql/base/namer.go (1 hunks)
  • contrib/screener-api/screener/screener.go (5 hunks)
  • contrib/screener-api/screener/suite_test.go (2 hunks)
Files skipped from review due to trivial changes (1)
  • .vscode/settings.json
Additional context used
GitHub Check: Lint (contrib/screener-api)
contrib/screener-api/screener/screener.go

[failure] 368-368:
var formattedJson should be formattedJSON (golint)

Additional comments not posted (9)
contrib/screener-api/db/sql/base/namer.go (2)

19-24: Good practice in declaring shared variables at the package level.

The declaration of these variables at the package level is appropriate, given their usage across multiple functions within the package. This promotes better modularity and reusability.


12-15: Consistent naming for database columns is a good practice.

This change helps maintain consistency in database column naming across the application. Please ensure that these names are correctly used in all relevant SQL queries and ORM mappings.

contrib/screener-api/db/models.go (1)

13-18: Struct field reordering and GORM tag updates are properly implemented.

Reordering fields and updating GORM tags can improve both clarity and ORM interactions. Ensure that these changes are reflected and tested in the database schema and any related database migrations.

contrib/screener-api/db/sql/base/base.go (1)

59-59: Updated column names in ORM clause are correctly implemented.

This change ensures that the ORM operations are aligned with the new column naming conventions. It's crucial to test these changes to avoid any database errors.

Verification successful

Updated column names in ORM clause are correctly implemented.

The new column names dataAddressName, dataRemarkName, dataNetworkName, and dataTagName are consistently defined and used within the codebase.

  • contrib/screener-api/db/sql/base/base.go
  • contrib/screener-api/db/sql/base/namer.go
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the updated column names are correctly used in ORM operations.

# Test: Search for the use of new column names in ORM operations.
rg --type go $'dataAddressName|dataRemarkName|dataNetworkName|dataTagName'

Length of output: 840

contrib/screener-api/client/client.go (2)

117-123: Structural enhancement in BlackListBody improves data encapsulation.

The addition of nested structures within BlackListBody enhances data encapsulation and handling. Ensure that these changes are well-documented and tested, particularly in serialization and deserialization processes.

Verification successful

Structural enhancement in BlackListBody improves data encapsulation.

The addition of nested structures within BlackListBody enhances data encapsulation and handling. These changes are well-documented and tested, particularly in serialization and deserialization processes, as evidenced by the test cases in suite_test.go.

  • contrib/screener-api/screener/suite_test.go
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify the correct handling of the new nested structures in serialization and deserialization.

# Test: Search for tests involving `BlackListBody`.
rg --type go 'BlackListBody'

Length of output: 1064



Script:

#!/bin/bash
# Description: Check the test cases in `suite_test.go` to ensure they cover serialization and deserialization of `BlackListBody`.

# Search for test functions in `suite_test.go` that involve `BlackListBody`.
rg --type go --pcre2 'func.*Test.*\(.*BlackListBody' contrib/screener-api/screener/suite_test.go

Length of output: 98



Script:

#!/bin/bash
# Description: List all functions in `suite_test.go` to manually verify if any of them involve `BlackListBody` and cover serialization and deserialization.

# List all functions in `suite_test.go`.
rg --type go --pcre2 '^func ' contrib/screener-api/screener/suite_test.go

Length of output: 630



Script:

#!/bin/bash
# Description: Search for `BlackListBody` usage within the listed functions in `suite_test.go` to identify any relevant tests.

# Search for `BlackListBody` usage within the functions in `suite_test.go`.
rg --type go 'BlackListBody' contrib/screener-api/screener/suite_test.go -A 10

Length of output: 848


136-137: Simplification of GenerateSignature function enhances clarity and efficiency.

The removal of intermediate steps in the HMAC creation process simplifies the function, potentially improving performance and maintainability. Confirm that this change does not affect the security and integrity of the signature generation.

contrib/screener-api/screener/suite_test.go (1)

204-211: Refactored test setup using structured objects enhances clarity and maintainability.

Using structured objects in tests instead of marshaled strings is a good practice as it makes the tests more readable and easier to maintain. Ensure that these changes are thoroughly tested to confirm compatibility with the actual API implementation.

Verification successful

Refactored test setup using structured objects enhances clarity and maintainability.

Using structured objects in tests instead of marshaled strings is a good practice as it makes the tests more readable and easier to maintain. The search results confirm that the new test setup involving BlackListBody is consistent with the actual API implementation.

  • contrib/screener-api/screener/screener.go
  • contrib/screener-api/screener/suite_test.go
  • contrib/screener-api/client/client.go
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that the new test setup is compatible with the actual API implementation.

# Test: Search for tests involving `BlackListBody`.
rg --type go 'BlackListBody'

Length of output: 1064

contrib/screener-api/screener/screener.go (2)

Line range hint 307-329: Optimize handling of repetitive code in blacklistAddress.

There's repetitive code for handling different cases of create, update, and delete which could be abstracted into a helper function to improve code maintainability and reduce redundancy.
[REFACTOR_Suggestion]

+ func (s *screenerImpl) handleBlacklistOperation(ctx context.Context, operation string, blacklistedAddress db.BlacklistedAddress) error {
+   var err error
+   switch operation {
+   case "create":
+       err = s.db.PutBlacklistedAddress(ctx, blacklistedAddress)
+   case "update":
+       err = s.db.UpdateBlacklistedAddress(ctx, blacklistedAddress.ID, blacklistedAddress)
+   case "delete":
+       err = s.db.DeleteBlacklistedAddress(ctx, blacklistedAddress.ID)
+   }
+   if err != nil {
+       span.AddEvent("error", trace.WithAttributes(attribute.String("error", err.Error())))
+       return err
+   }
+   span.AddEvent("blacklistedAddress", trace.WithAttributes(attribute.String("address", blacklistedAddress.Address)))
+   return nil
+ }

- // Existing switch case code
+ err := s.handleBlacklistOperation(ctx, blacklistBody.Type, blacklistedAddress)
+ if err != nil {
+   c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()})
+   return
+ }
+ c.JSON(http.StatusOK, gin.H{"status": "success"})

297-297: Ensure thread safety when updating the blacklist cache.

You are correctly locking the mutex before modifying the blacklistCache. However, ensure that all other accesses to blacklistCache are also properly synchronized to prevent data races.

Verification successful

Thread safety for blacklist cache is ensured.

All accesses to blacklistCache are properly synchronized with mutex locks, ensuring thread safety.

  • s.blacklistCache[strings.ToLower(item)] = true is protected by s.blacklistCacheMux.Lock().
  • return s.blacklistCache[address] is protected by s.blacklistCacheMux.RLock().
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify that all accesses to `blacklistCache` use the mutex.
# Test: Search for accesses to `blacklistCache` and check for mutex usage.

rg --type go $'blacklistCache' contrib/screener-api/screener/screener.go | rg -v $'blacklistCacheMux'

Length of output: 317



Script:

#!/bin/bash
# Description: Verify mutex usage around specific accesses to `blacklistCache`.
# Test: Search for the specific lines and check for mutex locks in their vicinity.

# Search for the write access `s.blacklistCache[strings.ToLower(item)] = true`
rg --context 3 's.blacklistCache\[strings.ToLower\(item\)\] = true' contrib/screener-api/screener/screener.go

# Search for the read access `return s.blacklistCache[address]`
rg --context 3 'return s.blacklistCache\[address\]' contrib/screener-api/screener/screener.go

Length of output: 553

Comment on lines 361 to 384
var jsonData map[string]interface{}
if err := json.Unmarshal(bodyBytes, &jsonData); err != nil {
c.JSON(http.StatusBadRequest, gin.H{"error": "could not unmarshal request body"})
c.Abort()
return
}

formattedJson, err := json.Marshal(jsonData)
if err != nil {
c.JSON(http.StatusBadRequest, gin.H{"error": "could not marshal request body"})
c.Abort()
return
}

bodyStr := string(formattedJson)

var message string
if len(queryString) > 0 {
message = fmt.Sprintf("%s;%s;%s;%s;%s;%s;%s",
appID, timestamp, nonce, "POST", "/api/data/sync", queryString, bodyStr)
} else {
message = fmt.Sprintf("%s;%s;%s;%s;%s;%s",
appID, timestamp, nonce, "POST", "/api/data/sync", bodyStr)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct variable naming convention and handle potential JSON processing errors.

The variable name formattedJson should follow Go's naming convention for acronyms.

- var formattedJson, err = json.Marshal(jsonData)
+ var formattedJSON, err = json.Marshal(jsonData)

Also, ensure that potential errors from JSON processing (json.Marshal and json.Unmarshal) are handled appropriately to prevent issues during request processing.

+ if err != nil {
+   logger.With("error", err).Error("Failed to process JSON data")
+   c.JSON(http.StatusBadRequest, gin.H{"error": "Failed to process JSON data"})
+   return
+ }
Tools
GitHub Check: Lint (contrib/screener-api)

[failure] 368-368:
var formattedJson should be formattedJSON (golint)

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.

Actionable comments posted: 1

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 7ffa8d0 and baf7322.

Files selected for processing (2)
  • contrib/screener-api/client/client.go (3 hunks)
  • contrib/screener-api/screener/screener.go (5 hunks)
Files skipped from review as they are similar to previous changes (1)
  • contrib/screener-api/client/client.go
Additional context used
GitHub Check: Lint (contrib/screener-api)
contrib/screener-api/screener/screener.go

[failure] 368-368:
var formattedJson should be formattedJSON (golint)

Additional comments not posted (1)
contrib/screener-api/screener/screener.go (1)

280-292: Refactor to use structured logging for better traceability.

The logging statements in the blacklistAddress function currently use simple event logging. Consider using structured logging to include more context about the operations, which could be beneficial for debugging and tracing the flow of data.

- span.AddEvent("error", trace.WithAttributes(attribute.String("error", err.Error())))
+ logger.With("error", err).Error("Failed to put blacklisted address")

c.Abort()
return
}
formattedJson, err := json.Marshal(jsonData)
Copy link
Contributor

Choose a reason for hiding this comment

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

Correct variable naming convention and handle potential JSON processing errors.

The variable name formattedJson should follow Go's naming convention for acronyms.

- var formattedJson, err = json.Marshal(jsonData)
+ var formattedJSON, err = json.Marshal(jsonData)

Also, ensure that potential errors from JSON processing (json.Marshal and json.Unmarshal) are handled appropriately to prevent issues during request processing.

+ if err != nil {
+   logger.With("error", err).Error("Failed to process JSON data")
+   c.JSON(http.StatusBadRequest, gin.H{"error": "Failed to process JSON data"})
+   return
+ }
Tools
GitHub Check: Lint (contrib/screener-api)

[failure] 368-368:
var formattedJson should be formattedJSON (golint)

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Enhanced logging for blacklisted addresses to improve traceability
  • Refined data handling in blacklisting operations for better efficiency
  • Reorganized internal structures for blacklist data to simplify processing
  • Added core.BytesToJSONString function for JSON string conversion
  • Improved request body processing in authMiddleware using core.BytesToJSONString

3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Enhanced logging for blacklisted addresses to improve traceability
  • Refined data handling in blacklisting operations for better efficiency
  • Reorganized internal structures for blacklist data to simplify processing
  • Corrected variable name from 'formattedJson' to 'formattedJSON' in core/bytes.go for readability and consistency
  • Added font ligature settings to improve VS Code text rendering

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Enhanced logging for blacklisted addresses to improve traceability
  • Refined data handling in blacklisting operations for better efficiency
  • Reorganized internal structures for blacklist data to simplify processing
  • Added font ligature settings to improve VS Code text rendering

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

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.

Actionable comments posted: 3

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between baf7322 and c95e39c.

Files selected for processing (3)
  • contrib/screener-api/client/client.go (4 hunks)
  • contrib/screener-api/screener/screener.go (5 hunks)
  • core/bytes.go (1 hunks)
Additional context used
GitHub Check: codecov/patch
core/bytes.go

[warning] 15-23: core/bytes.go#L15-L23
Added lines #L15 - L23 were not covered by tests


[warning] 25-25: core/bytes.go#L25
Added line #L25 was not covered by tests

contrib/screener-api/client/client.go

[warning] 88-91: contrib/screener-api/client/client.go#L88-L91
Added lines #L88 - L91 were not covered by tests


[warning] 138-139: contrib/screener-api/client/client.go#L138-L139
Added lines #L138 - L139 were not covered by tests

contrib/screener-api/screener/screener.go

[warning] 366-369: contrib/screener-api/screener/screener.go#L366-L369
Added lines #L366 - L369 were not covered by tests

core/bytes.go Outdated
Comment on lines 14 to 26
// BytesToJSONString converts a 32 bit array to a JSON string without escapes, newlines, etc.
func BytesToJSONString(bz []byte) string {
var jsonData map[string]interface{}
if err := json.Unmarshal(bz, &jsonData); err != nil {
return ""
}
formattedJSON, err := json.Marshal(jsonData)
if err != nil {
return ""
}

return string(formattedJSON)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve error handling and add test coverage for BytesToJSONString.

The function BytesToJSONString lacks proper error handling and does not have test coverage. It's essential to handle potential errors from json.Unmarshal and json.Marshal to avoid silent failures.

-	if err := json.Unmarshal(bz, &jsonData); err != nil {
-		return ""
+	if err := json.Unmarshal(bz, &jsonData); err != nil {
+		return "", err
+	}
+	formattedJSON, err := json.Marshal(jsonData)
+	if err != nil {
+		return "", err
 	}
-	formattedJSON, err := json.Marshal(jsonData)
-	if err != nil {
-		return ""
-	}
-	return string(formattedJSON)
+	return string(formattedJSON), nil

Would you like me to help set up unit tests for this function? It's important to ensure the function behaves as expected under different scenarios.

Committable suggestion was skipped due to low confidence.

Tools
GitHub Check: codecov/patch

[warning] 15-23: core/bytes.go#L15-L23
Added lines #L15 - L23 were not covered by tests


[warning] 25-25: core/bytes.go#L25
Added line #L25 was not covered by tests

Comment on lines 88 to 91
bodyStr := core.BytesToJSONString(bodyBz)

message := fmt.Sprintf("%s;%s;%s;%s;%s;%s",
appid, timestamp, nonce, "POST", "/api/data/sync", bodyStr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve test coverage for new and modified lines in client.go.

The newly added lines in BlacklistAddress and the modified GenerateSignature function lack test coverage. It's crucial to add unit tests to ensure that these critical functionalities are working as expected and to catch potential regressions in future changes.

Consider adding unit tests that cover:

  • The construction of the message string in BlacklistAddress.
  • The HMAC generation process in GenerateSignature.

Would you like assistance in writing these tests or setting up a testing framework?

Also applies to: 138-139

Tools
GitHub Check: codecov/patch

[warning] 88-91: contrib/screener-api/client/client.go#L88-L91
Added lines #L88 - L91 were not covered by tests

Comment on lines +280 to +292
(attribute.String("network", blacklistBody.Data.Network)),
(attribute.String("tag", blacklistBody.Data.Tag)),
(attribute.String("remark", blacklistBody.Data.Remark)),
(attribute.String("address", blacklistBody.Data.Address)),
)

blacklistedAddress := db.BlacklistedAddress{
Type: blacklistBody.Type,
ID: blacklistBody.ID,
Data: blacklistBody.Data,
Network: blacklistBody.Network,
Tag: blacklistBody.Tag,
Remark: blacklistBody.Remark,
Address: strings.ToLower(blacklistBody.Address),
Network: blacklistBody.Data.Network,
Tag: blacklistBody.Data.Tag,
Remark: blacklistBody.Data.Remark,
Address: strings.ToLower(blacklistBody.Data.Address),
Copy link
Contributor

Choose a reason for hiding this comment

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

Improve test coverage and refine error handling in screener.go.

The newly added lines in blacklistAddress and authMiddleware functions lack test coverage. Additionally, the error handling in these functions can be improved to provide more detailed feedback and ensure robust operation.

Consider refining the error handling by:

  • Providing more specific error messages.
  • Logging errors at critical points to aid in debugging.

Would you like help in enhancing the error handling mechanisms or adding unit tests to cover these functionalities?

Also applies to: 366-369

@github-actions github-actions bot added the M-ci Module: CI label Jun 27, 2024
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Removed 'data' field from db.BlacklistedAddress in Swagger documentation
  • Improved data handling and logging in screener API
  • Simplified HMAC generation and data structure management
  • Reordered and renamed fields for consistency and clarity

3 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Implemented font ligatures in VS Code settings for improved readability
  • Added BytesToJSONString function in the core package for better JSON handling
  • Fixed data structure issues in the API documentation to reflect the new nested Data struct
  • Updated API documentation to remove obsolete data field and reflect new data structure
  • Simplified code for signature generation and blacklisted address management for improved maintainability

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

core/bytes.go Outdated
// BytesToSlice converts a 32 bit array to a slice slice.
func BytesToSlice(bytes [32]byte) []byte {
rawBytes := make([]byte, len(bytes))
copy(rawBytes, bytes[:])
return rawBytes
}

// BytesToJSONString converts a 32 bit array to a JSON string without escapes, newlines, etc.
func BytesToJSONString(bz []byte) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

absolutely 0 chance this should not be error checked

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.

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
contrib/screener-api/docs/swagger.yaml (1)

[!TIP]
Codebase Verification

Discrepancies in Swagger Documentation for db.BlacklistedAddress

The Swagger documentation for the BlacklistedAddress model does not accurately reflect the current structure of the model as defined in models.go. Specifically:

  • Missing fields: UpdatedAt, Tag
  • Incorrect field: network should be Type

Please update the Swagger documentation to match the following structure:

  db.BlacklistedAddress:
    properties:
      address:
        type: string
      createdAt:
        type: string
      updatedAt:
        type: string
      id:
        type: string
      type:
        type: string
      network:
        type: string
      tag:
        type: string
      remark:
        type: string
Analysis chain

Line range hint 2-13: Confirm updated model structure in Swagger documentation.

The Swagger documentation for db.BlacklistedAddress should reflect the structural changes mentioned in the AI-generated summary, particularly the update to the ID field's gorm tag. Ensure all fields are documented correctly and match the updated model structure.

Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Check the model structure in `models.go` and compare with Swagger documentation.

# Test: Search for the BlacklistedAddress struct definition and check the fields.
rg --type go $'type BlacklistedAddress struct'

Length of output: 114



Script:

#!/bin/bash
# Description: Extract the full definition of the BlacklistedAddress struct from models.go

# Extract the struct definition
rg --type go --no-heading --line-number -A 10 'type BlacklistedAddress struct' contrib/screener-api/db/models.go

Length of output: 615

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c95e39c and 714e0bf.

Files selected for processing (5)
  • .github/labeler.yml (2 hunks)
  • .vscode/settings.json (1 hunks)
  • contrib/screener-api/docs/docs.go (1 hunks)
  • contrib/screener-api/docs/swagger.json (1 hunks)
  • contrib/screener-api/docs/swagger.yaml (1 hunks)
Files skipped from review due to trivial changes (3)
  • .github/labeler.yml
  • contrib/screener-api/docs/docs.go
  • contrib/screener-api/docs/swagger.json
Files skipped from review as they are similar to previous changes (1)
  • .vscode/settings.json
Additional comments not posted (1)
contrib/screener-api/docs/swagger.yaml (1)

Line range hint 17-51: Review API endpoint descriptions and parameters for clarity and correctness.

The API endpoints /api/data/sync and /screen/{address} are defined with their respective parameters and descriptions. Ensure that the descriptions are clear, the parameters are correctly documented, and the expected responses are detailed. It's important that these endpoints accurately reflect the functionality as implemented in the backend code.

Verification successful

The functions blacklistAddress and screenAddress are implemented in contrib/screener-api/screener/screener.go. To ensure the Swagger documentation accurately reflects these implementations, we need to extract the function signatures and compare their parameters and descriptions with those in the Swagger file.


The extracted implementations of blacklistAddress and screenAddress functions align with the Swagger documentation. The blacklistAddress function processes a JSON body and handles different types of blacklist actions, while the screenAddress function checks if an address is blacklisted or whitelisted.

Verified: The parameters and descriptions in the Swagger documentation for the /api/data/sync and /screen/{address} endpoints are accurate and consistent with the backend implementations.

  • blacklistAddress function handles different types of blacklist actions (create, update, delete) and processes a JSON body.
  • screenAddress function checks if an address is blacklisted or whitelisted and processes the request accordingly.
Scripts executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Description: Verify endpoint implementations in the backend code.

# Test: Search for the endpoint implementations in `screener.go` and compare with Swagger documentation.
rg --type go $'func \(s \*screenerImpl\) blacklistAddress\('
rg --type go $'func \(s \*screenerImpl\) screenAddress\('

Length of output: 317



Script:

#!/bin/bash
# Extract the function signatures and parameter details from screener.go

# Extract the blacklistAddress function
ast-grep --lang go --pattern 'func (s *screenerImpl) blacklistAddress($_) {
  $$$
}' contrib/screener-api/screener/screener.go

# Extract the screenAddress function
ast-grep --lang go --pattern 'func (s *screenerImpl) screenAddress($_) {
  $$$
}' contrib/screener-api/screener/screener.go

Length of output: 10364

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.

Actionable comments posted: 0

Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 714e0bf and 8646f0e.

Files selected for processing (1)
  • .github/labeler.yml (2 hunks)
Files skipped from review as they are similar to previous changes (1)
  • .github/labeler.yml

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Added error handling for JSON conversion in BlacklistAddress method (client.go)
  • Improved error handling in authMiddleware function (screener.go)
  • Enhanced BytesToJSONString function with error handling (core/bytes.go)
  • Added TestBytesToJSONString to cover new error handling logic (core/bytes_test.go)

4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Added font ligature settings to VS Code for enhanced readability
  • Introduced function to convert 32-bit array to JSON string in core package
  • Updated API documentation to reflect removal of data field from JSON template
  • Simplified data handling for blacklisted addresses in screener API
  • Adjusted field ordering and naming conventions in database models for consistency

4 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@trajan0x trajan0x merged commit 8d04213 into master Jun 27, 2024
64 of 66 checks passed
@trajan0x trajan0x deleted the fix-data branch June 27, 2024 15:14
@trajan0x trajan0x mentioned this pull request Jun 27, 2024
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 M-ci Module: CI size/s
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants