feat: new layer of FeePayer as a helper for transaction final completion step
#328
Conversation
🦋 Changeset detectedLatest commit: cca8107 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 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 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. |
✅ Deploy Preview for appccc 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. |
ashuralyk
changed the title
FeePayer as a helper for transaction final completion step
|
further implementation on margin fee payer for Spore module, here it is: ashuralyk#8 |
|
/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 FeePayer abstraction layer to handle the transaction completion step, which is a significant and positive architectural change. It successfully decouples fee payment logic from the Transaction class, making it more modular and extensible. The refactoring of the Signer class to leverage this new abstraction is clean. However, I've identified a critical issue in the new completeByFeePayer method where the return value of prepareTransaction is ignored, potentially leading to incorrect transaction preparation and fee calculation.
| async completeByFeePayer( | ||
| client: Client, | ||
| ...feePayers: FeePayer[] | ||
| ): Promise<void> { | ||
| for (const feePayer of feePayers) { | ||
| await feePayer.prepareTransaction(this); | ||
| } | ||
| for (const feePayer of feePayers) { | ||
| await feePayer.completeTxFee(this, client); | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of completeByFeePayer has a critical bug. The prepareTransaction method on a FeePayer can return a new Transaction instance, but its return value is ignored in the first loop. This means any preparations (like adding cell dependencies) are lost, and the subsequent completeTxFee calls operate on the original, unprepared transaction, which can lead to incorrect fee calculation and transaction failure.
To fix this, you should chain the results of prepareTransaction, perform fee completion on the fully prepared transaction, and then copy the state of the modified transaction back to the current instance.
async completeByFeePayer(
client: Client,
...feePayers: FeePayer[]
): Promise<void> {
let tx: Transaction = this;
for (const feePayer of feePayers) {
tx = await feePayer.prepareTransaction(tx);
}
for (const feePayer of feePayers) {
await feePayer.completeTxFee(tx, client);
}
this.copy(tx);
}
packages/core/src/ckb/transaction.ts
Outdated
| ...feePayers: FeePayer[] | ||
| ): Promise<void> { | ||
| for (const feePayer of feePayers) { | ||
| await feePayer.prepareTransaction(this); |
There was a problem hiding this comment.
I agree with Gemini here: we should use the return value of prepareTransaction rather than assuming it modifies the tx in place. The previous implementation should be considered a bug.
Possible situation: A wallet (like JoyID, a web wallet) decides to handle the prepareTransaction stage itself to support more lock scripts in the future, and we serialize the tx for the API. This prevents the operation from happening in place.
To fix this problem without changing APIs, I assume we can use Transaction.copy to modify this at the end of completeBy* methods.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new FeePayer abstraction layer, significantly improving the modularity and extensibility of transaction completion logic. By moving the fee calculation and input collection responsibilities from the Transaction class to dedicated FeePayer and FeePayerFromAddress classes, the codebase becomes cleaner and more maintainable. The Signer class now correctly extends FeePayerFromAddress, inheriting these capabilities. The new test cases for margin calculation and the FeePayer layer are comprehensive and well-structured, ensuring the new functionality works as expected. This is a well-executed architectural improvement.
|
|
||
| await tx.completeByFeePayer(client, mockFeePayer1, mockFeePayer2); | ||
|
|
||
| expect(mockFeePayer1.prepareTransaction).toHaveBeenCalledWith(tx); |
There was a problem hiding this comment.
The completeByFeePayer method clones the transaction before passing it to prepareTransaction. Therefore, mockFeePayer1.prepareTransaction receives a clone of the original tx, not the original tx object itself. The current assertion toHaveBeenCalledWith(tx) will likely fail because tx is the original transaction instance, not the cloned one passed to the mock. Consider asserting with expect.any(ccc.Transaction) or expect.objectContaining if specific properties of the cloned transaction need to be verified.
| expect(mockFeePayer1.prepareTransaction).toHaveBeenCalledWith(tx); | |
| expect(mockFeePayer1.prepareTransaction).toHaveBeenCalledWith(expect.any(ccc.Transaction)); |
| await tx.completeByFeePayer(client, mockFeePayer1, mockFeePayer2); | ||
|
|
||
| expect(mockFeePayer1.prepareTransaction).toHaveBeenCalledWith(tx); | ||
| expect(mockFeePayer2.prepareTransaction).toHaveBeenCalledWith(tx); |
There was a problem hiding this comment.
Similar to the previous comment, mockFeePayer2.prepareTransaction also receives a cloned transaction. The assertion toHaveBeenCalledWith(tx) should be updated to reflect this.
| expect(mockFeePayer2.prepareTransaction).toHaveBeenCalledWith(tx); | |
| expect(mockFeePayer2.prepareTransaction).toHaveBeenCalledWith(expect.any(ccc.Transaction)); |
2 participants
Description
Add a new layer named
FeePayer(could be changed further), to abstract our transaction complete step, like balancing process, paying with capacity margin or with DeFi swap ratio, etc.This change can bring much clearer layer hierarchy on CCC, it makes transaction completion step programmable.