Conversation
Summary of ChangesHello @sadiq1971, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly overhauls the Canton client integration by introducing a new, modular Canton SDK. This refactoring aims to improve the maintainability and extensibility of the codebase by breaking down the previous monolithic client into specialized sub-clients for ledger, identity, token, and bridge operations. Additionally, it integrates Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring by creating a new canton-sdk package to encapsulate all interactions with the Canton ledger. This is a great improvement for code structure and maintainability. The old pkg/canton is removed, and its functionality is now provided by the new SDK. The changes in cmd/api-server and cmd/relayer correctly adopt this new SDK.
My review focuses on a few critical issues in the configuration and some areas for improvement in the new SDK implementation, including a critical misconfiguration in .golangci.yml that disables linting, a bug in the Makefile that prevents downloading the linter, some brittle error handling, and code duplication.
.golangci.yml
Outdated
| paths: | ||
| - pkg/apidb | ||
| - pkg/auth | ||
| - pkg/canton | ||
| - pkg/config | ||
| - pkg/db | ||
| - pkg/ethereum | ||
| - pkg/ethrpc | ||
| - pkg/keys | ||
| - pkg/registration | ||
| - pkg/relayer | ||
| - pkg/service | ||
| - cmd | ||
| - internal |
There was a problem hiding this comment.
The paths in linters.exclusions.paths are not anchored. This will cause golangci-lint to exclude any file path that contains these substrings. For example, a path like my/pkg/internal/stuff.go would be excluded because it contains internal. This effectively disables linting for almost the entire codebase. The comment on line 142 correctly states that these should be anchored. You should anchor all paths to match only the intended directories at the root of the project, for example by prefixing them with ^ (e.g., ^pkg/apidb/).
.golangci.yml
Outdated
| paths: | ||
| - pkg/apidb | ||
| - pkg/auth | ||
| - pkg/canton | ||
| - pkg/config | ||
| - pkg/db | ||
| - pkg/ethereum | ||
| - pkg/ethrpc | ||
| - pkg/keys | ||
| - pkg/registration | ||
| - pkg/relayer | ||
| - pkg/service | ||
| - cmd | ||
| - internal |
There was a problem hiding this comment.
Similar to the linters exclusions, the paths in formatters.exclusions.paths are not anchored. This will disable formatting for a large portion of the codebase unintentionally. Please anchor all paths with ^ to ensure they only match directories at the project root.
| if [ ! -f ./bin/golangci-lint ]; then \ | ||
| curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s v2.9.0; \ | ||
| fi; |
There was a problem hiding this comment.
The version v2.9.0 for golangci-lint is invalid and will cause the get_lint target to fail. The latest version of golangci-lint is v1.59.1. Please use a valid version. Also, the install script is invoked in a way that might not place the binary in ./bin. It's better to explicitly use the -b flag to specify the output directory.
if [ ! -f ./bin/golangci-lint ]; then \
curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b ./bin v1.59.1; \
fi;
| if err != nil { | ||
| // TODO: Match grpc error code without hard-wiring exact text. | ||
| if strings.Contains(strings.ToLower(err.Error()), "already") { | ||
| return nil | ||
| } | ||
| return fmt.Errorf("complete withdrawal: %w", err) | ||
| } |
There was a problem hiding this comment.
The error handling for an already completed withdrawal is brittle as it relies on string matching the error message. This can break if the error message from the upstream service changes. It's better to check for the specific gRPC status code, similar to how isAuthError is implemented. You should check for codes.AlreadyExists. The TODO on line 383 also points this out.
| if err != nil { | |
| // TODO: Match grpc error code without hard-wiring exact text. | |
| if strings.Contains(strings.ToLower(err.Error()), "already") { | |
| return nil | |
| } | |
| return fmt.Errorf("complete withdrawal: %w", err) | |
| } | |
| if err != nil { | |
| s, ok := status.FromError(err) | |
| if ok && s.Code() == codes.AlreadyExists { | |
| return nil | |
| } | |
| return fmt.Errorf("complete withdrawal: %w", err) | |
| } |
| CorePackageID: cfg.CorePackageID, | ||
| BridgeModule: cfg.BridgeModule, | ||
| CIP56PackageID: cfg.CIP56PackageID, | ||
| // TODO: why bridge config needs the common package id |
There was a problem hiding this comment.
This TODO indicates a point of confusion in the configuration. It would be beneficial to investigate this and either add a comment explaining the reason or refactor the configuration to make it clearer. Please create a follow-up ticket to track this.
| // Todo: split this into multiple files (source & destination) that will implement source and destination respectively. | ||
| // Current name 'handlers' doesn't represent clear context |
There was a problem hiding this comment.
This is a good observation. To improve code organization, it would be beneficial to split this file into source.go and destination.go to better separate the concerns of different bridge endpoints. Please create a follow-up ticket to track this refactoring.
| SourceChain: "canton", // todo: use constant | ||
| DestinationChain: "ethereum", // todo: use constant |
There was a problem hiding this comment.
Using constants for chain names like canton and ethereum would improve readability and prevent typos. Please consider defining these as constants in a relevant package.
| @@ -1,10 +1,12 @@ | |||
| package relayer | |||
|
|
|||
| // TODO: remove the mock impl and use mockery to generate mock | |||
There was a problem hiding this comment.
Using a tool like mockery to auto-generate mocks would be more maintainable than manually creating them. Please consider creating a ticket to switch to auto-generated mocks.
| // todo: log error if update fails on db. | ||
| p.store.UpdateTransferStatus(event.ID, db.TransferStatusFailed, nil) |
There was a problem hiding this comment.
This todo is important. It's crucial to handle and log errors that might occur when updating the transfer status in the database to avoid silent failures and inconsistent state.
| } | ||
|
|
||
| // Update status to completed | ||
| // todo: handle error |
There was a problem hiding this comment.
Similar to the failure case, it's important to handle and log any potential errors when updating the transfer status to 'completed' to ensure the database state is consistent.
1 participant
Closes #96