feat: add fiber-sdk#194
Conversation
🦋 Changeset detectedLatest commit: 4415268 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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. |
|
/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 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.
| 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 |
|
/canary |
|
❌ Canary version deployment failed. View workflow run |
|
/canary |
1 similar comment
|
/canary |
|
🚀 Canary version published successfully! View workflow run The following packages have been published to npm:
|
|
Hanssen0
left a comment
Hanssen0
left a comment
There was a problem hiding this comment.
Overall, LGTM, and I really like the idea of separating APIs into different classes to improve maintainability. However, the manual composition of API classes into an SDK class is annoying to me.
I suggest using the mixins pattern to eliminate design complexity. Such a design pattern is also utilised by CCC's molecule codec part to reduce coding tasks for devs.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the @ckb-ccc/fiber package, a comprehensive TypeScript SDK for the Fiber payment channel network. The implementation features a high-level FiberSDK built with mixins for channel, invoice, payment, and peer management, supported by a low-level JSON-RPC client that handles automatic camelCase to snake_case conversion and hex serialization. Additionally, the PR includes a specialized testing runtime that polyfills browser-only features like Web Workers and IndexedDB to enable integration testing in Node.js. The review feedback identifies critical issues in the serialization logic, specifically regarding the handling of class instances and Uint8Array types, as well as a bug where private fields with getters are lost during object spreading when preparing RPC payloads.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the @ckb-ccc/fiber package, providing a TypeScript SDK for the Fiber payment channel network. The implementation includes a modular FiberSDK using mixins for various domains, a JSON-RPC client with automatic case conversion, and a specialized testing runtime for Node.js. Review feedback identifies several technical issues, including serialization bugs caused by using private fields with getters in classes intended for spreading, incorrect nesting of custom record objects in RPC payloads, and missing file extensions required for ESM compatibility. Additionally, improvements were suggested for safer object property checks and preventing the mangling of user-defined keys during recursive case conversion.
| function convertKeys(value: unknown, keyFn: (key: string) => string): unknown { | ||
| if (value === null || value === undefined) { | ||
| return value; | ||
| } | ||
| if (Array.isArray(value)) { | ||
| return value.map((item) => convertKeys(item, keyFn)); | ||
| } | ||
| if (typeof value === "object" && !(value instanceof Uint8Array)) { | ||
| const obj = value as Record<string, unknown>; | ||
| const out: Record<string, unknown> = {}; | ||
| for (const key of Object.keys(obj)) { | ||
| const newKey = keyFn(key); | ||
| out[newKey] = convertKeys(obj[key], keyFn); | ||
| } | ||
| return out; | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
The convertKeys function recursively transforms all object keys between camelCase and snake_case. This can mangle user-defined keys in data maps like customRecords. Consider adding a mechanism to skip key conversion for specific fields or objects intended to be used as opaque maps.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the @ckb-ccc/fiber package, providing a TypeScript/JavaScript SDK for the Fiber payment channel network. The implementation includes a high-level FiberSDK with domain-specific mixins for channels, invoices, and payments, supported by a custom JSON-RPC client that handles automatic camelCase to snake_case conversion and hex serialization. Feedback identifies a bug in the toHex utility where zero values are incorrectly treated as falsy, missing .js extensions in ESM relative imports, and a missing null check in payment parameter validation. Additionally, it is recommended to update the Attribute type definitions to camelCase to align with the SDK's runtime transformation logic.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the @ckb-ccc/fiber package, providing a TypeScript SDK for interacting with Fiber nodes. It includes API mixins for channels, invoices, payments, node info, and peers, along with necessary RPC serialization and type definitions. I have suggested an improvement to the openChannelWithExternalFunding method to ensure the returned unsignedFundingTx is automatically converted into a ccc.Transaction instance, enhancing consistency and developer experience.
| async openChannelWithExternalFunding( | ||
| params: fiber.OpenChannelWithExternalFundingParamsLike, | ||
| ): Promise<fiber.OpenChannelWithExternalFundingResult> { | ||
| const normalized = | ||
| fiber.OpenChannelWithExternalFundingParams.from(params); | ||
| return this.rpc.call<fiber.OpenChannelWithExternalFundingResult>( | ||
| "open_channel_with_external_funding", | ||
| [{ ...normalized }], | ||
| ); | ||
| } |
There was a problem hiding this comment.
The unsignedFundingTx returned by the RPC is a plain object after snakeToCamel conversion. To provide a better developer experience and maintain consistency with the CCC ecosystem, this object should be converted into a ccc.Transaction instance. This allows callers to directly use methods like .hash() on the returned transaction. This implementation also follows the rule of accepting generic 'Like' types for parameters and performing internal normalization.
async openChannelWithExternalFunding(
params: fiber.OpenChannelWithExternalFundingParamsLike,
): Promise<fiber.OpenChannelWithExternalFundingResult> {
const normalized =
fiber.OpenChannelWithExternalFundingParams.from(params);
const res = await this.rpc.call<fiber.OpenChannelWithExternalFundingResult>(
"open_channel_with_external_funding",
[{ ...normalized }],
);
return {
...res,
unsignedFundingTx: ccc.Transaction.from(res.unsignedFundingTx),
};
}References
- Functions should accept more generic types like HexLike and perform necessary type conversions internally, rather than requiring the caller to do so.
4 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.