feat: implemented token and identity on canton sdk#114
feat: implemented token and identity on canton sdk#114sadiq1971 wants to merge 11 commits intofeat/canton-sdk-initfrom
Conversation
…contract filtering Replace client-side template filtering with server-side TemplateFilter in Canton Ledger API v2 GetActiveContracts calls. This reduces network traffic and improves scalability by having the ledger server filter contracts before streaming them to the middleware. Changes: - Add templateFilter(), cip56HoldingFilter(), tokenConfigFilter() helpers - Add wildcardFilter() helper for the generic getActiveContractsByTemplate - Replace WildcardFilter with CIP56Holding TemplateFilter in 6 methods: GetUserBalance, GetTotalSupply, findHoldingForTransfer, findRecipientHolding, GetAllCIP56Holdings - Replace WildcardFilter with TokenConfig TemplateFilter in GetTokenConfig - Replace WildcardFilter with WayfinderBridgeConfig TemplateFilter in getTokenConfigCidFromBridge - Replace WildcardFilter with dual PendingDeposit+DepositReceipt TemplateFilter in IsDepositProcessed - Remove wildcard fallback in GetFingerprintMapping (common_package_id is always configured) - Remove redundant client-side templateId checks after server-side filtering - Remove stale comment block about wildcard filter limitations - Keep WildcardFilter only in getActiveContractsByTemplate (generic helper) Closes #81
Add defensive fallback to WildcardFilter when package ID config values are empty, preventing silent failures where TemplateFilter with an empty PackageId would match nothing. This aligns with the existing fallback pattern previously used in GetFingerprintMapping. Affected methods: GetFingerprintMapping, IsDepositProcessed, getTokenConfigCidFromBridge, cip56HoldingFilter, tokenConfigFilter.
…ge IDs Validate that cip56_package_id, common_package_id, and bridge_package_id are set when creating the Canton client. This fails fast on misconfiguration rather than silently degrading to WildcardFilter at runtime.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
refactor: replace WildcardFilter with TemplateFilter for server-side contract filtering
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 enhances the Canton SDK by integrating robust identity and token management capabilities. It introduces dedicated client modules for handling party allocation, fingerprint-based identity resolution, and a full suite of CIP-56 token operations like minting, burning, and transfers. The changes also include crucial optimizations to ledger data retrieval by implementing server-side template filtering, which streamlines contract queries and improves overall performance. 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
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 new SDK packages for identity and token management, which is a great step towards better code structure and modularity. It also includes a valuable refactoring in pkg/canton/client.go to leverage server-side template filtering instead of client-side wildcard filtering, which should significantly improve performance. The new code is well-structured, and I have a few minor suggestions to enhance it further.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a substantial and valuable refactoring, extracting token and identity management logic into a new, well-structured canton-sdk package. This greatly improves modularity and code organization. The move to use server-side template filtering is also a significant cleanup. I've identified a critical issue with the new value decoding functions, which could lead to silent errors, and have also provided several medium-severity suggestions to enhance maintainability by using constants for hardcoded strings and sentinel errors for better error handling. Overall, this is a strong contribution that moves the codebase in a very positive direction.
| // Numeric extracts a numeric value as string. | ||
| func Numeric(v *lapiv2.Value) string { | ||
| if v == nil { | ||
| return "0" | ||
| } | ||
| if n, ok := v.Sum.(*lapiv2.Value_Numeric); ok { | ||
| return n.Numeric | ||
| } | ||
| return "0" | ||
| } |
There was a problem hiding this comment.
The Numeric function (and others in this file like Text, Party) suppresses errors during value extraction by returning a default value ("0" or ""). This can lead to silent failures and incorrect data processing. For example, if a contract argument is unexpectedly not a numeric type, this function will return "0" instead of signaling an error, which could lead to incorrect balance calculations without any error being logged.
The previous implementation of similar helper functions in pkg/canton/encoding.go (e.g., extractNumericV2) returned an error, which is a safer pattern.
Please change the function signature to return an error, forcing callers to handle potential decoding failures explicitly.
| // Numeric extracts a numeric value as string. | |
| func Numeric(v *lapiv2.Value) string { | |
| if v == nil { | |
| return "0" | |
| } | |
| if n, ok := v.Sum.(*lapiv2.Value_Numeric); ok { | |
| return n.Numeric | |
| } | |
| return "0" | |
| } | |
| // Numeric extracts a numeric value as string. | |
| func Numeric(v *lapiv2.Value) (string, error) { | |
| if v == nil { | |
| return "", fmt.Errorf("cannot extract numeric from nil value") | |
| } | |
| if n, ok := v.Sum.(*lapiv2.Value_Numeric); ok { | |
| return n.Numeric, nil | |
| } | |
| return "", fmt.Errorf("value is not of type numeric: %T", v.GetSum()) | |
| } |
| return nil, fmt.Errorf("invalid request: %w", err) | ||
| } | ||
| if l == nil { | ||
| return nil, fmt.Errorf("nil ledger client") |
There was a problem hiding this comment.
The error for a nil ledger client is created with fmt.Errorf. It would be more idiomatic and useful for callers to define this as a sentinel error variable at the package level. This allows callers to programmatically check for this specific error using errors.Is. This pattern is already used for ErrInsufficientBalance and ErrBalanceFragmented in the token package.
This applies to other similar static error strings in this file, such as on line 66.
For example:
var ErrNilLedger = errors.New("nil ledger client")
// in New()
if l == nil {
return nil, ErrNilLedger
}| module := "Common.FingerprintAuth" | ||
| entity := "FingerprintMapping" |
There was a problem hiding this comment.
Hardcoding module and entity names as string literals can lead to typos and makes maintenance harder. These values are used in multiple places (e.g., here and in GetFingerprintMapping on lines 182-183).
Consider defining them as constants at the package level for consistency and safety.
For example:
const (
fingerprintAuthModule = "Common.FingerprintAuth"
fingerprintMappingEntity = "FingerprintMapping"
)Then you can use these constants throughout the client.
| Rights: []*adminv2.Right{right}, | ||
| }) | ||
| if err != nil { | ||
| if isAlreadyExistsError(err) { // TODO: need to verify this works |
There was a problem hiding this comment.
The TODO comment here indicates uncertainty about whether isAlreadyExistsError will work as expected. Leaving TODOs for core logic like error handling in production code is risky. Please verify this behavior and remove the TODO. If the error is not guaranteed to be of type codes.AlreadyExists, you might need a more robust way to check for this condition, for example by inspecting the error message string as a fallback, though that is brittle.
| ModuleName: "CIP56.Config", | ||
| EntityName: "TokenConfig", |
There was a problem hiding this comment.
Similar to the identity client, this file has many hardcoded strings for template modules, entities, and choice names (e.g., CIP56.Config, TokenConfig, IssuerMint, CIP56.Token, CIP56Holding). This is repeated across many functions (Mint, Burn, GetHoldings, etc.).
To improve maintainability and reduce the risk of typos, it's highly recommended to define these as constants at the package level.
Example:
const (
cip56ConfigModule = "CIP56.Config"
tokenConfigEntity = "TokenConfig"
cip56TokenModule = "CIP56.Token"
cip56HoldingEntity = "CIP56Holding"
choiceIssuerMint = "IssuerMint"
// ... and so on
)2 participants
Closes #94