Conversation
🦋 Changeset detectedLatest commit: a39edf0 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s Go dependencies (notably CTF framework + Sui-related modules) and bumps the default Sui CTF Docker image tag used for non-arm64 environments.
Changes:
- Bump
chainlink-testing-framework/frameworkfromv0.15.13tov0.15.16. - Update Sui-related dependencies (e.g.,
chainlink-sui,chainlink-common,chainlink-protos/cre) and related indirect deps. - Update the default Sui tools Docker image tag from
mysten/sui-tools:devnet-v1.61.0todevnet-v1.69.0in the Sui CTF provider.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
go.mod |
Updates required/indirect module versions to newer CTF framework + Sui ecosystem versions. |
go.sum |
Syncs module checksums with the updated dependency graph. |
chain/sui/provider/ctf_provider.go |
Pins the non-arm64 default Sui tools image to devnet-v1.69.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| key, err := parseSuiKeytoolGenerateJSON(keyOut) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("%w (raw output: %.300q)", err, keyOut) | ||
| } |
There was a problem hiding this comment.
On parse failure, the returned error wraps and includes raw output: ... from sui keytool generate --json. That raw output can contain mnemonics / key material, so including it in an error risks leaking secrets when errors are logged upstream. Prefer returning a redacted/structured error (e.g., include only a truncated, sanitized snippet or just the container command / exit status).
| } | ||
| } | ||
|
|
||
| // newSuiCTFBlockchainNetwork mirrors CTF blockchain.newSui but uses generateKeyDataCTF so |
There was a problem hiding this comment.
why can't we use/patch CTF instead? I don't think we should have this much container management inside CLDF unless we really must.
There was a problem hiding this comment.
will address this in CTF first
There was a problem hiding this comment.
agree, if we bake this is CTF, then more people can benefit it too?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // multiplexed stream format (first byte 1=stdout / 2=stderr). Must run before stripping 0x01, | ||
| // which appears in stream headers and would corrupt the stream if removed globally. |
There was a problem hiding this comment.
The comment mentions stripping 0x01, but this code doesn't strip 0x01 anywhere (it strips \x00 later in parseSuiKeytoolGenerateJSON). Please update the comment to reflect the actual behavior/requirement so future changes don't optimize around a nonexistent constraint.
| // multiplexed stream format (first byte 1=stdout / 2=stderr). Must run before stripping 0x01, | |
| // which appears in stream headers and would corrupt the stream if removed globally. | |
| // multiplexed stream format (first byte 1=stdout / 2=stderr). This must run before any later | |
| // output sanitization/parsing because those leading bytes are part of Docker's stream headers; | |
| // parseSuiKeytoolGenerateJSON later strips only "\x00" bytes after the stream has been demuxed. |
|
No description provided.