feat: canton cct sdk for registry pools - #360
Open
stackman27 wants to merge 19 commits into
Open
stackman27 wants to merge 19 commits into
stackman27 wants to merge 19 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…arg types + dead-code cleanup
# Conflicts: # ccip-sdk/src/canton/index.ts # ccip-sdk/src/canton/types.ts
CI Test Report❌ 7 of 3059 tests did not pass (pass 3046, fail 7, cancelled 0, skipped 6) in 4m 53s Failed suites: Failed testsSummaryCoverage report |
stackman27
marked this pull request as ready for review
September 16, 2026 01:04
stackman27
changed the title
RodrigoAD
requested changes
Sep 16, 2026
RodrigoAD
left a comment
RodrigoAD
left a comment
Member
There was a problem hiding this comment.
Left some comments. The cct/canton is quite big, I couldn't go through all.
As a general comment, let's please reduce the verbosity of the comments.
| @@ -0,0 +1,191 @@ | |||
| /** | |||
Member
There was a problem hiding this comment.
We should move these scripts somewhere else. If it's only for local testing, let's remove them. If they are useful for testing, we can convert them in integration tests. If useful for users, we can turn them into documentation
Author
There was a problem hiding this comment.
Keeping these as user-facing examples/docs rather than removing or converting to integration tests, I think this is genuinely useful reference material for integrating via the Wallet Gateway, not just local scratch scripts
| @@ -0,0 +1,51 @@ | |||
| # CCT Canton SDK — integration guide | |||
Member
There was a problem hiding this comment.
Considering we can't keep the previous scripts, I don't think these docs make too much sense
| * method. Returns a function with the standard `fetch` signature so it can be | ||
| * passed straight to `CantonChain.fromUrl({ fetch })` / `createCantonClient`. | ||
| */ | ||
| export function createGatewayLedgerFetch( |
Member
There was a problem hiding this comment.
This is only used by the new scripts. do we really need it?
If we do, let's please refactor it, it has many weird implementation decisions (jsonRpcId counter, the hardcoded /livez response...)
Author
There was a problem hiding this comment.
Kept it (examples still need it), refactored both issues: jsonRpcId is now per-instance instead of module-global, and /livez genuinely round-trips through the gateway instead of faking a 200
| * | ||
| * LocalNet/DevNet have no stable deployments — pass explicit overrides there. | ||
| */ | ||
| export const CANTON_NETWORKS: Readonly<Record<string, CantonNetworkConfig>> = { |
Member
There was a problem hiding this comment.
We should avoid hardcoded addresses or URLs. I see it's only used in a test, but why would we test against testnet or mainnet? I think we should remove this
Author
There was a problem hiding this comment.
Not test-only, and not a live-network test either: getCantonNetworkConfig('canton:TestNet') is just an in-memory lookup on the CANTON_NETWORKS table, and resolvePoolFactoryDeps uses it in
real production code to default deployTokenPool's deps on known networks.
Removing it would force every caller to pass tokenAdminRegistry/feeQuoter/rmnRemote explicitly, even on TestNet/MainNet. I'd rather keep it
| * to return every active contract of the template. | ||
| * @returns All matching active contracts (may be empty). | ||
| */ | ||
| async findActiveContractsByTemplate( |
Member
There was a problem hiding this comment.
This is almost a duplicate of findActiveContractByTemplate. Let's reuse it here
| * @param caller - Acting party (`actAs`). | ||
| * @returns The raw `exerciseResult` value, or `null` if no matching exercised event. | ||
| */ | ||
| async submitReadChoice( |
Member
There was a problem hiding this comment.
No callers of this one. We can remove it?
| * @param match - Predicate over the decoded `createArgument` record. | ||
| * @returns The first matching active contract, or `null` if none match. | ||
| */ | ||
| async findActiveContractByTemplate( |
| * 3. Execute the signed transaction (`/v2/interactive-submission/executeAndWaitForTransaction`). | ||
| */ | ||
| private async submitCommands( | ||
| async submitCommands( |
Member
There was a problem hiding this comment.
Making this public allows users to send arbitrary txs, allowing them to bypass validations for the execute/send commands, and making the SDK surface way bigger and confusing. Let's make it private again. We can extract the logic somewhere else if needs to be reused
Author
There was a problem hiding this comment.
extracted it into a standalone submitCantonCommands() function so CantonChain no longer exposes it at all, while operation.ts (which needed cross-module access) can still call the same logic
|
|
||
| /** Atomically deploys and initializes a `BurnMintTokenPool`/`LockReleaseTokenPool` (registry-pools family). */ | ||
| async deployTokenPool(opts: ExecuteDeployTokenPoolParams): Promise<ExecuteDeployTokenPoolResult> { | ||
| return this.#deployTokenPool.execute(this.chain, opts) as Promise<ExecuteDeployTokenPoolResult> |
Member
There was a problem hiding this comment.
We can't do these types of casting. It's false that the response has a poolCid field declared. It happens more time in this class
Author
There was a problem hiding this comment.
fixed, DeployTokenPool/ApplyChainUpdates now override execute() to actually extract poolCid (and rateLimiterCids/tokenConfigCid/poolInstanceAddress) from the transaction response, throwing if the ledger didn't produce what's expected
RodrigoAD
reviewed
Sep 22, 2026
| if (!contract?.contractId || !contract.createdEventBlob) return null | ||
| return edsContractToSdk(contract) | ||
| } catch { | ||
| return null |
Member
There was a problem hiding this comment.
Let's return the error here instead
| */ | ||
| export function normalizeRemoteAddress(s: string): string { | ||
| const bare = s.startsWith('0x') ? s.slice(2) : s | ||
| return bare.length === 40 ? bare.padStart(64, '0') : bare |
Member
There was a problem hiding this comment.
This doesn't guarantee 32 B if we pass any different than 20 B (40 char)
Author
There was a problem hiding this comment.
fixed, now always pads to 32 bytes and throws on non-hex or >32B input instead of passing through
| }, | ||
| }, | ||
| ], | ||
| commandId: `cct-deploy-${p.poolType}-pool-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, |
Member
There was a problem hiding this comment.
Suggested change
| commandId: `cct-deploy-${p.poolType}-pool-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, | |
| commandId: `cct-deploy-${p.poolType}-pool-${randomUUID()}`, |
I see many places with this logic. If there's some concurrency, this could cause some collision. I'd recommend using randomUUID for it. You do it already in tests
| ): Promise<CantonActiveContract | null> { | ||
| return contract && instanceAddress === RL_INSTANCE_ADDRESS ? contract : null | ||
| }, | ||
| } as unknown as CantonChain |
Member
There was a problem hiding this comment.
We shouldn't do this. Why doesn't as CantonChain work? Let's fix that instead
Author
There was a problem hiding this comment.
fixed, mocks now build a real instance via Object.assign(Object.create(CantonChain.prototype), {...}), no cast anywhere (as CantonChain can't work since the class has private fields)
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.