Conversation
🦋 Changeset detectedLatest commit: e0f89ac 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 |
Hanssen0
force-pushed
the
master
branch
2 times, most recently
from
4f1083c to
1d5d04d
Compare
- 修改 Script 接口中 args 的类型从 string[] 改为 string - 修改 shutdownChannel 函数中的 close_script.args 参数格式 - 确保所有参数都符合 API 期望的十六进制字符串格式 - 更新相关依赖和工具函数 这个修改解决了通道关闭时出现的 'invalid type: sequence, expected a 0x-prefixed hex string' 错误。
ashuralyk
force-pushed
the
feat/fiber-sdk
branch
from
b0d1404 to
97ae3f0
Compare
✅ Deploy Preview for appccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for liveccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
/gemini review |
![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 new fiber-sdk package, which is a significant and well-structured feature. The inclusion of comprehensive integration tests and a detailed README is excellent. However, there are several issues to address, including a critical security vulnerability related to a hardcoded private key, bugs in the example HTML files due to incorrect API usage, and some areas for code improvement for better maintainability and security.
.env
Outdated
| process.env.PRIVATE_KEY = | ||
| "0x0000000000000000000000000000000000000000000000000000000000000000"; |
There was a problem hiding this comment.
Committing an environment file containing a private key, even a zero-value one, poses a significant security risk. This file should be removed from the repository to prevent accidental exposure of real credentials in the future. It's recommended to use a template file (e.g., .env.example) to guide developers on the required environment variables, and keep the actual .env file in .gitignore.
packages/fiber/.prettierignore
Outdated
| .prettierrc | ||
| tsconfig.json | ||
| eslint.config.mjs | ||
| .prettierrc |
There was a problem hiding this comment.
There is a duplicate entry for .prettierrc in this file. Removing the duplicate will improve the maintainability of the configuration.
| const result = await sdk.channel.openChannel({ | ||
| peer_id: peerId, | ||
| funding_amount: BigInt(fundingAmount) * BigInt(100000000), // Convert to shannon | ||
| public: isPublic, | ||
| }); |
There was a problem hiding this comment.
The openChannel method expects parameters with camelCase keys (e.g., peerId, fundingAmount), as defined in the SDK. The current implementation uses snake_case keys (peer_id, funding_amount), which will cause runtime errors. Please update the keys to use camelCase.
| const result = await sdk.channel.openChannel({ | |
| peer_id: peerId, | |
| funding_amount: BigInt(fundingAmount) * BigInt(100000000), // Convert to shannon | |
| public: isPublic, | |
| }); | |
| const result = await sdk.channel.openChannel({ | |
| peerId: peerId, | |
| fundingAmount: BigInt(fundingAmount) * BigInt(100000000), // Convert to shannon | |
| public: isPublic, | |
| }); |
| if (channelToClose.state.state_name === "NEGOTIATING_FUNDING") { | ||
| showStatus("通道正在协商资金阶段，无法关闭", "error"); | ||
| return; | ||
| } | ||
|
|
||
| await sdk.channel.abandonChannel(channelToClose.channel_id); |
There was a problem hiding this comment.
The properties of the channel object returned by listChannels are in camelCase. The code is trying to access state.state_name and channel_id, but it should be state.stateName and channelId. Additionally, the abandonChannel method expects an object with the channelId property, not just the ID string.
| if (channelToClose.state.state_name === "NEGOTIATING_FUNDING") { | |
| showStatus("通道正在协商资金阶段，无法关闭", "error"); | |
| return; | |
| } | |
| await sdk.channel.abandonChannel(channelToClose.channel_id); | |
| if (channelToClose.state.stateName === "NEGOTIATING_FUNDING") { | |
| showStatus("通道正在协商资金阶段，无法关闭", "error"); | |
| return; | |
| } | |
| await sdk.channel.abandonChannel({ channelId: channelToClose.channelId }); |
| } | ||
|
|
||
| try { | ||
| await sdk.peer.connectPeer(peerAddress); |
There was a problem hiding this comment.
The connectPeer method expects an object with an address property, not the address string directly. Please wrap the peerAddress in an object.
| await sdk.peer.connectPeer(peerAddress); | |
| await sdk.peer.connectPeer({ address: peerAddress }); |
| if (channelToClose.state.state_name === "NEGOTIATING_FUNDING") { | ||
| document.getElementById("channelInfo").textContent = | ||
| "通道正在协商资金阶段，无法关闭"; | ||
| return; | ||
| } | ||
|
|
||
| await sdk.channel.abandonChannel(channelToClose.channel_id); |
There was a problem hiding this comment.
The properties of the channel object are in camelCase, so state.state_name should be state.stateName and channel_id should be channelId. Also, abandonChannel expects an object with a channelId property, like { channelId: ... }.
| if (channelToClose.state.state_name === "NEGOTIATING_FUNDING") { | |
| document.getElementById("channelInfo").textContent = | |
| "通道正在协商资金阶段，无法关闭"; | |
| return; | |
| } | |
| await sdk.channel.abandonChannel(channelToClose.channel_id); | |
| if (channelToClose.state.stateName === "NEGOTIATING_FUNDING") { | |
| document.getElementById("channelInfo").textContent = | |
| "通道正在协商资金阶段，无法关闭"; | |
| return; | |
| } | |
| await sdk.channel.abandonChannel({ channelId: channelToClose.channelId }); |
| } | ||
| }); | ||
|
|
||
| eval(workerData.script); |
There was a problem hiding this comment.
Using eval() can introduce security vulnerabilities by executing arbitrary code. Although this is within a test environment, it's a best practice to avoid eval(). Consider using a safer alternative like new Function(workerData.script)() to execute the script in a more controlled scope.
| eval(workerData.script); | |
| new Function(workerData.script)(); |
| export class OpenChannelParams { | ||
| constructor( | ||
| public readonly peerId: string, | ||
| public readonly fundingAmount: ccc.Hex, | ||
| isPublic?: boolean, | ||
| public readonly fundingUdtTypeScript?: ccc.Script, | ||
| public readonly shutdownScript?: ccc.Script, | ||
| public readonly commitmentDelayEpoch?: ccc.Hex, | ||
| public readonly commitmentFeeRate?: ccc.Hex, | ||
| public readonly fundingFeeRate?: ccc.Hex, | ||
| public readonly tlcExpiryDelta?: ccc.Hex, | ||
| public readonly tlcMinValue?: ccc.Hex, | ||
| public readonly tlcFeeProportionalMillionths?: ccc.Hex, | ||
| public readonly maxTlcValueInFlight?: ccc.Hex, | ||
| public readonly maxTlcNumberInFlight?: ccc.Hex, | ||
| ) { | ||
| this._isPublic = isPublic; | ||
| } | ||
|
|
||
| private readonly _isPublic?: boolean; | ||
| get public(): boolean | undefined { | ||
| return this._isPublic; | ||
| } | ||
|
|
There was a problem hiding this comment.
The implementation of the public property in OpenChannelParams using a private _isPublic field and a public getter is a bit confusing and could be simplified. A standard public property would be more straightforward and easier to maintain, especially since public is not a reserved keyword for property names in this context.
export class OpenChannelParams {
constructor(
public readonly peerId: string,
public readonly fundingAmount: ccc.Hex,
public readonly public?: boolean,
public readonly fundingUdtTypeScript?: ccc.Script,
public readonly shutdownScript?: ccc.Script,
public readonly commitmentDelayEpoch?: ccc.Hex,
public readonly commitmentFeeRate?: ccc.Hex,
public readonly fundingFeeRate?: ccc.Hex,
public readonly tlcExpiryDelta?: ccc.Hex,
public readonly tlcMinValue?: ccc.Hex,
public readonly tlcFeeProportionalMillionths?: ccc.Hex,
public readonly maxTlcValueInFlight?: ccc.Hex,
public readonly maxTlcNumberInFlight?: ccc.Hex,
) {}There was a problem hiding this comment.
public is reserved words so it cannot be used as a variable name in TypeScript
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive Fiber SDK for CCC, complete with a well-structured API, detailed type definitions, and an impressive testing suite that utilizes an in-process Fiber node. The overall quality of the code is high. However, there are a few significant issues that need attention. The most critical is the inclusion of a private key in the repository, which poses a serious security risk. Additionally, I've identified some bugs in a test HTML file and a subtle issue in a type definition class that could lead to runtime errors. I've provided specific comments and suggestions to address these points.
| const { invoiceAddress, invoice } = await sdk.newInvoice({ | ||
| amount: "0x5f5e100", | ||
| currency: "Fibt", | ||
| description: "Coffee", | ||
| expiry: "0xe10", | ||
| finalExpiryDelta: "0x9283C0", | ||
| }); |
There was a problem hiding this comment.
The code examples use magic hex strings for values like amount, expiry, and finalExpiryDelta. This makes the code harder to understand. It would be more readable if you either used decimal numbers with comments or defined constants with descriptive names to represent these values.
| } | ||
|
|
||
| // ── Run the fiber-js worker IIFE ────────────────────────────────────────────── | ||
| eval(workerData.script); |
There was a problem hiding this comment.
Using eval() can be a security risk as it executes code with the privileges of the caller. While in this context the script comes from a trusted dependency (fiber-js), it's a practice that should generally be avoided. If there's any alternative way to load and execute the worker script without eval, it would be preferable.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new package, @ckb-ccc/fiber, which provides a comprehensive SDK for interacting with the Fiber payment channel network. The implementation is robust, featuring a well-designed API, thorough type definitions, and an impressive testing setup that includes a Node.js runtime for the browser-based fiber-js library. The code quality is high, and the documentation is clear. I've included a couple of suggestions to improve code clarity and maintainability.
| await sdk.cancelInvoice(invoice.data.paymentHash); | ||
| ``` | ||
|
|
||
| `parseInvoice` on `FiberSDK` takes `{ invoice: string }` and returns a `CkbInvoice` (the API layer returns `{ invoice }` as `ParseInvoiceResult`). |
There was a problem hiding this comment.
The explanation of how parseInvoice works on the API layer is an implementation detail that might be confusing for an SDK user. To improve clarity, I suggest removing the parenthetical part and focusing only on the behavior of the FiberSDK method.
| `parseInvoice` on `FiberSDK` takes `{ invoice: string }` and returns a `CkbInvoice` (the API layer returns `{ invoice }` as `ParseInvoiceResult`). | |
| `parseInvoice` on `FiberSDK` takes `{ invoice: string }` and returns a `CkbInvoice`. |
| export class OpenChannelParams { | ||
| constructor( | ||
| public readonly peerId: string, | ||
| public readonly fundingAmount: ccc.Hex, | ||
| private readonly _public?: boolean, | ||
| public readonly fundingUdtTypeScript?: ccc.Script, | ||
| public readonly shutdownScript?: ccc.Script, | ||
| public readonly commitmentDelayEpoch?: ccc.Hex, | ||
| public readonly commitmentFeeRate?: ccc.Hex, | ||
| public readonly fundingFeeRate?: ccc.Hex, | ||
| public readonly tlcExpiryDelta?: ccc.Hex, | ||
| public readonly tlcMinValue?: ccc.Hex, | ||
| public readonly tlcFeeProportionalMillionths?: ccc.Hex, | ||
| public readonly maxTlcValueInFlight?: ccc.Hex, | ||
| public readonly maxTlcNumberInFlight?: ccc.Hex, | ||
| ) {} | ||
|
|
||
| get public(): boolean | undefined { | ||
| return this._public; | ||
| } |
There was a problem hiding this comment.
The use of a private _public property with a getter to handle the public keyword is a bit complex. Since public can be used as an object property name without issue, you can simplify this class by using public directly as a constructor parameter. This will make the code more straightforward and easier to maintain.
| export class OpenChannelParams { | |
| constructor( | |
| public readonly peerId: string, | |
| public readonly fundingAmount: ccc.Hex, | |
| private readonly _public?: boolean, | |
| public readonly fundingUdtTypeScript?: ccc.Script, | |
| public readonly shutdownScript?: ccc.Script, | |
| public readonly commitmentDelayEpoch?: ccc.Hex, | |
| public readonly commitmentFeeRate?: ccc.Hex, | |
| public readonly fundingFeeRate?: ccc.Hex, | |
| public readonly tlcExpiryDelta?: ccc.Hex, | |
| public readonly tlcMinValue?: ccc.Hex, | |
| public readonly tlcFeeProportionalMillionths?: ccc.Hex, | |
| public readonly maxTlcValueInFlight?: ccc.Hex, | |
| public readonly maxTlcNumberInFlight?: ccc.Hex, | |
| ) {} | |
| get public(): boolean | undefined { | |
| return this._public; | |
| } | |
| export class OpenChannelParams { | |
| constructor( | |
| public readonly peerId: string, | |
| public readonly fundingAmount: ccc.Hex, | |
| public readonly public?: boolean, | |
| public readonly fundingUdtTypeScript?: ccc.Script, | |
| public readonly shutdownScript?: ccc.Script, | |
| public readonly commitmentDelayEpoch?: ccc.Hex, | |
| public readonly commitmentFeeRate?: ccc.Hex, | |
| public readonly fundingFeeRate?: ccc.Hex, | |
| public readonly tlcExpiryDelta?: ccc.Hex, | |
| public readonly tlcMinValue?: ccc.Hex, | |
| public readonly tlcFeeProportionalMillionths?: ccc.Hex, | |
| public readonly maxTlcValueInFlight?: ccc.Hex, | |
| public readonly maxTlcNumberInFlight?: ccc.Hex, | |
| ) {} |
There was a problem hiding this comment.
use public as member in a class is prohibited in TypeScript
|
/canary |
|
❌ Canary version deployment failed. View workflow run |
3 participants
Background
Taking advantage of fiber-js to implement a basic usable
fiber-sdkin ccc, which contains only features of channel, invoice and payment.Inside test cases, there're two native fiber nodes started by
fiber-jswith interconnection, all the cases would interact with them to validate channel, invoice and payment functionalities, without needs of any outside fiber nodes.