feat(core): FeePayer abstraction#362
feat(core): FeePayer abstraction#362Hanssen0 wants to merge 2 commits intockb-devrel:releases/nextfrom
FeePayer abstraction#362Conversation
🦋 Changeset detectedLatest commit: 0d38e4c The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
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 |
✅ Deploy Preview for appccc 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 apiccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello, 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 refactors the transaction completion and fee payment mechanisms by introducing a dedicated Highlights
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. Footnotes
|
✅ Deploy Preview for docsccc ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 FeePayer abstraction, which is a significant and well-executed refactoring. Moving the transaction completion and fee payment logic out of the Transaction class into a dedicated FeePayer hierarchy greatly improves separation of concerns and makes the Transaction class cleaner. The new FeePayer, FeePayerFromAddress, and FeePayerGroup classes provide a flexible and extensible way to handle fee payments. Making Signer a subclass of FeePayerFromAddress is also a logical design choice. The changes are well-tested. I have one critical comment regarding the use of an incorrect error type, which has been updated in the code suggestion.
| @@ -0,0 +1,243 @@ | |||
| import { Address } from "../address/index.js"; | |||
| import { Cell, Transaction, TransactionLike } from "../ckb/transaction.js"; | |||
| import { ErrorTransactionInsufficientCapacity } from "../ckb/transactionErrors.js"; | |||
There was a problem hiding this comment.
You're importing ErrorTransactionInsufficientCapacity, which appears to be an old error type that was replaced as part of this refactoring. The new FeePayer module introduces its own ErrorFeePayerInsufficientCapacity in errors.ts.
To maintain consistency and correctness within the new abstraction, please use ErrorFeePayerInsufficientCapacity throughout this file. This includes updating the instanceof checks and throw statements.
| import { ErrorTransactionInsufficientCapacity } from "../ckb/transactionErrors.js"; | |
| import { ErrorFeePayerInsufficientCapacity } from "./errors.js"; |
Hanssen0
force-pushed
the
feat/fee-payer
branch
4 times, most recently
from
131e0eb to
310b86d
Compare
Hanssen0
force-pushed
the
feat/fee-payer
branch
2 times, most recently
from
f91857a to
48d2d73
Compare
Hanssen0
force-pushed
the
feat/fee-payer
branch
from
48d2d73 to
0d38e4c
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a FeePayer abstraction, a significant refactoring that separates transaction fee completion logic from the Transaction class. The change is well-structured, moving logic to the new FeePayer and FeePayerFromAddress classes and making Signer a subclass of FeePayerFromAddress. Deprecating old methods on Transaction while maintaining backward compatibility is a good strategy. My review includes suggestions to improve documentation for error handling in the new FeePayer class, address a minor type safety issue, and fix an incomplete mock object in a test file to improve test robustness. All original comments were kept as they did not contradict any provided rules.
| /** | ||
| * Completes the transaction fee by applying a custom change function. | ||
| * | ||
| * @param txLike - The transaction to complete the fee for. | ||
| * @param change - A function that modifies the transaction to handle the change. | ||
| * @param optionsLike - Optional configuration for completing the fee. | ||
| * @returns A promise that resolves to the transaction with the fee paid, whether it was modified, and the operation context. | ||
| */ |
There was a problem hiding this comment.
The JSDoc for completeFeeChangeTo is missing documentation for potential exceptions that can be thrown. This can make it harder for developers to use this method correctly and handle errors gracefully. Please add @throws tags to document the exceptions that can be thrown, such as ErrorFeePayerInsufficientCapacity and when the change function is misused.
For example:
* @throws {ErrorFeePayerInsufficientCapacity} If there is not enough capacity to cover the fee and change.
* @throws {Error} If the change function does not use all available capacity or removes existing transaction data.|
|
||
| let leastFee = Zero; | ||
| let leastExtraCapacity = Zero; | ||
| let collectCapacityContext = undefined as CollectCapacityContext; // It's fine because it's assigned below |
There was a problem hiding this comment.
The type cast undefined as CollectCapacityContext is used here to initialize collectCapacityContext. While this works due to the while(true) loop ensuring assignment before use, it's a code smell as it bypasses type safety. This can be risky if the logic is changed in the future. A safer approach would be to avoid the cast, for instance by declaring the variable with a union type CollectCapacityContext | undefined and using a non-null assertion ! where it's guaranteed to be defined.
| index: 0, | ||
| }, | ||
| }); | ||
| return { tx, addedCount: 1 }; |
There was a problem hiding this comment.
The mock implementation for completeInputsAtLeastOne returns an object that is missing the collectedCapacity property, which is required by the FeePayerCompleteInputsResult type. While the current test may pass because the return value is not fully utilized, this can lead to brittle tests that fail unexpectedly if the code under test changes to use this property. It's best practice for mocks to conform to the types they are mocking.
| return { tx, addedCount: 1 }; | |
| return { tx, addedCount: 1, collectedCapacity: ccc.Zero }; |
1 participant
Refactors #328