-
Notifications
You must be signed in to change notification settings - Fork 34
feat: new layer of FeePayer as a helper for transaction final completion step
#328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
ashuralyk
wants to merge
14
commits into
ckb-devrel:master
Choose a base branch
from
ashuralyk:feat/fee-payer
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
239835a
feat: fee payer initialization
ashuralyk 7656820
feat: create a template for fee payer layer
ashuralyk 69b5e50
chore: apply fee payer manager on transaction
ashuralyk 5d1b344
chore: downgrade address related methods from Signer to SignerFeePayer
ashuralyk ed5bfe8
feat: solve cocurrence issue and doing rename
ashuralyk e796fc1
chore: rename group file
ashuralyk efa1e9e
chore: rewise spore test
ashuralyk e36515a
chore: adapt transaction test cases for new implementation
ashuralyk a524235
chore: fix data length fixed point issue and prepare dedicated test c…
ashuralyk 4c7b4cd
chore: make use of the result of prepareTransaction
ashuralyk cca8107
chore: return Transaction for mock prepareTransaction method
ashuralyk 6f54a7e
chore: adapt to comments of review
ashuralyk 6084911
chore: make changes for PR reviews
ashuralyk ac844ef
fix(test): remove client parameter while it removed from the function…
ashuralyk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@ckb-ccc/core": minor | ||
| --- | ||
|
|
||
| Abstract transaction completion step by a new layer named FeePayer |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -58,6 +58,20 @@ describe("Transaction", () => { | |||||
| }, | ||||||
| ); | ||||||
|
|
||||||
| // Mock the findCells method to return our mock UDT cells | ||||||
| vi.spyOn(client, "findCells").mockImplementation( | ||||||
| async function* (searchKey) { | ||||||
| if ( | ||||||
| searchKey.filter?.script && | ||||||
| ccc.Script.from(searchKey.filter.script).eq(type) | ||||||
| ) { | ||||||
| for (const cell of mockUdtCells) { | ||||||
| yield cell; | ||||||
| } | ||||||
| } | ||||||
| }, | ||||||
| ); | ||||||
|
|
||||||
| // Mock client.getCell to return the cell data for inputs | ||||||
| vi.spyOn(client, "getCell").mockImplementation(async (outPoint) => { | ||||||
| const cell = mockUdtCells.find((c) => c.outPoint.eq(outPoint)); | ||||||
|
|
@@ -263,9 +277,12 @@ describe("Transaction", () => { | |||||
|
|
||||||
| it("should use only one cell when user has only one cell available", async () => { | ||||||
| // Mock signer to return only one cell | ||||||
| vi.spyOn(signer, "findCells").mockImplementation( | ||||||
| async function* (filter) { | ||||||
| if (filter.script && ccc.Script.from(filter.script).eq(type)) { | ||||||
| vi.spyOn(client, "findCells").mockImplementation( | ||||||
| async function* (searchKey) { | ||||||
| if ( | ||||||
| searchKey.filter?.script && | ||||||
| ccc.Script.from(searchKey.filter.script).eq(type) | ||||||
| ) { | ||||||
| yield mockUdtCells[0]; // Only yield the first cell | ||||||
| } | ||||||
| }, | ||||||
|
|
@@ -328,6 +345,18 @@ describe("Transaction", () => { | |||||
| }, | ||||||
| ); | ||||||
|
|
||||||
| // Mock the findCells method to return capacity cells | ||||||
| vi.spyOn(client, "findCells").mockImplementation( | ||||||
| async function* (searchKey) { | ||||||
| // Return capacity cells for general queries | ||||||
| if (!searchKey.filter?.script || searchKey.filter?.scriptLenRange) { | ||||||
| for (const cell of mockCapacityCells) { | ||||||
| yield cell; | ||||||
| } | ||||||
| } | ||||||
| }, | ||||||
| ); | ||||||
|
|
||||||
| // Mock client.getCell to return the cell data for inputs | ||||||
| vi.spyOn(client, "getCell").mockImplementation(async (outPoint) => { | ||||||
| const cell = mockCapacityCells.find((c) => c.outPoint.eq(outPoint)); | ||||||
|
|
@@ -551,7 +580,15 @@ describe("Transaction", () => { | |||||
| ); | ||||||
|
|
||||||
| // Verify that findCells was called with the custom filter | ||||||
| expect(signer.findCells).toHaveBeenCalledWith(customFilter, true); | ||||||
| for (const address of await signer.getAddressObjs()) { | ||||||
| expect(client.findCells).toHaveBeenCalledWith({ | ||||||
| script: address.script, | ||||||
| scriptType: "lock", | ||||||
| filter: customFilter, | ||||||
| scriptSearchMode: "exact", | ||||||
| withData: true, | ||||||
| }); | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| it("should throw error when change function doesn't use all capacity", async () => { | ||||||
|
|
@@ -1128,4 +1165,244 @@ describe("Transaction", () => { | |||||
| }); | ||||||
| }); | ||||||
| }); | ||||||
|
|
||||||
| describe("Fee Payer Layer", () => { | ||||||
| let mockFeePayer1: ccc.FeePayer; | ||||||
| let mockFeePayer2: ccc.FeePayer; | ||||||
|
|
||||||
| beforeEach(() => { | ||||||
| // Create mock fee payers | ||||||
| mockFeePayer1 = { | ||||||
| prepareTransaction: vi | ||||||
| .fn() | ||||||
| .mockImplementation(async (tx: ccc.TransactionLike) => | ||||||
| ccc.Transaction.from(tx), | ||||||
| ), | ||||||
| completeTxFee: vi.fn().mockResolvedValue(undefined), | ||||||
| } as unknown as ccc.FeePayer; | ||||||
|
|
||||||
| mockFeePayer2 = { | ||||||
| prepareTransaction: vi | ||||||
| .fn() | ||||||
| .mockImplementation(async (tx: ccc.TransactionLike) => | ||||||
| ccc.Transaction.from(tx), | ||||||
| ), | ||||||
| completeTxFee: vi.fn().mockResolvedValue(undefined), | ||||||
| } as unknown as ccc.FeePayer; | ||||||
| }); | ||||||
|
|
||||||
| it("should call prepareTransaction on all fee payers", async () => { | ||||||
| const tx = ccc.Transaction.from({ | ||||||
| outputs: [ | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(100), | ||||||
| lock, | ||||||
| }, | ||||||
| ], | ||||||
| }); | ||||||
|
|
||||||
| await tx.completeByFeePayer(mockFeePayer1, mockFeePayer2); | ||||||
|
|
||||||
| expect(mockFeePayer1.prepareTransaction).toHaveBeenCalledWith(tx); | ||||||
| expect(mockFeePayer2.prepareTransaction).toHaveBeenCalledWith(tx); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the previous comment,
Suggested change
|
||||||
| expect(mockFeePayer1.prepareTransaction).toHaveBeenCalledTimes(1); | ||||||
| expect(mockFeePayer2.prepareTransaction).toHaveBeenCalledTimes(1); | ||||||
| }); | ||||||
|
|
||||||
| it("should call completeTxFee on all fee payers after prepareTransaction", async () => { | ||||||
| const tx = ccc.Transaction.from({ | ||||||
| outputs: [ | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(100), | ||||||
| lock, | ||||||
| }, | ||||||
| ], | ||||||
| }); | ||||||
|
|
||||||
| await tx.completeByFeePayer(mockFeePayer1, mockFeePayer2); | ||||||
|
|
||||||
| // Verify both methods were called | ||||||
| expect(mockFeePayer1.prepareTransaction).toHaveBeenCalled(); | ||||||
| expect(mockFeePayer2.prepareTransaction).toHaveBeenCalled(); | ||||||
| expect(mockFeePayer1.completeTxFee).toHaveBeenCalled(); | ||||||
| expect(mockFeePayer2.completeTxFee).toHaveBeenCalled(); | ||||||
|
|
||||||
| // Verify completeTxFee was called with a Transaction and client | ||||||
| const completeTxFee1Call = ( | ||||||
| mockFeePayer1.completeTxFee as ReturnType<typeof vi.fn> | ||||||
| ).mock.calls[0]; | ||||||
| const completeTxFee2Call = ( | ||||||
| mockFeePayer2.completeTxFee as ReturnType<typeof vi.fn> | ||||||
| ).mock.calls[0]; | ||||||
| expect(completeTxFee1Call[0]).toBeInstanceOf(ccc.Transaction); | ||||||
| expect(completeTxFee2Call[0]).toBeInstanceOf(ccc.Transaction); | ||||||
|
|
||||||
| // Verify prepareTransaction was called before completeTxFee | ||||||
| // by checking the order of calls | ||||||
| const prepare1Order = ( | ||||||
| mockFeePayer1.prepareTransaction as ReturnType<typeof vi.fn> | ||||||
| ).mock.invocationCallOrder[0]; | ||||||
| const complete1Order = ( | ||||||
| mockFeePayer1.completeTxFee as ReturnType<typeof vi.fn> | ||||||
| ).mock.invocationCallOrder[0]; | ||||||
| expect(prepare1Order).toBeLessThan(complete1Order); | ||||||
| }); | ||||||
|
|
||||||
| it("should handle single fee payer", async () => { | ||||||
| const tx = ccc.Transaction.from({ | ||||||
| outputs: [ | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(100), | ||||||
| lock, | ||||||
| }, | ||||||
| ], | ||||||
| }); | ||||||
|
|
||||||
| await tx.completeByFeePayer(mockFeePayer1); | ||||||
|
|
||||||
| expect(mockFeePayer1.prepareTransaction).toHaveBeenCalledTimes(1); | ||||||
| expect(mockFeePayer1.completeTxFee).toHaveBeenCalledTimes(1); | ||||||
| expect(mockFeePayer2.prepareTransaction).not.toHaveBeenCalled(); | ||||||
| expect(mockFeePayer2.completeTxFee).not.toHaveBeenCalled(); | ||||||
| }); | ||||||
|
|
||||||
| it("should handle empty fee payer list", async () => { | ||||||
| const tx = ccc.Transaction.from({ | ||||||
| outputs: [ | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(100), | ||||||
| lock, | ||||||
| }, | ||||||
| ], | ||||||
| }); | ||||||
|
|
||||||
| // Should not throw with empty fee payer list | ||||||
| await expect(tx.completeByFeePayer()).resolves.not.toThrow(); | ||||||
| }); | ||||||
|
|
||||||
| it("should handle multiple fee payers in sequence", async () => { | ||||||
| const tx = ccc.Transaction.from({ | ||||||
| outputs: [ | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(100), | ||||||
| lock, | ||||||
| }, | ||||||
| ], | ||||||
| }); | ||||||
|
|
||||||
| const callOrder: string[] = []; | ||||||
| ( | ||||||
| mockFeePayer1.prepareTransaction as ReturnType<typeof vi.fn> | ||||||
| ).mockImplementation(async (tx: ccc.TransactionLike) => { | ||||||
| callOrder.push("prepare1"); | ||||||
| return ccc.Transaction.from(tx); | ||||||
| }); | ||||||
| ( | ||||||
| mockFeePayer2.prepareTransaction as ReturnType<typeof vi.fn> | ||||||
| ).mockImplementation(async (tx: ccc.TransactionLike) => { | ||||||
| callOrder.push("prepare2"); | ||||||
| return ccc.Transaction.from(tx); | ||||||
| }); | ||||||
| ( | ||||||
| mockFeePayer1.completeTxFee as ReturnType<typeof vi.fn> | ||||||
| ).mockImplementation(async () => { | ||||||
| callOrder.push("complete1"); | ||||||
| }); | ||||||
| ( | ||||||
| mockFeePayer2.completeTxFee as ReturnType<typeof vi.fn> | ||||||
| ).mockImplementation(async () => { | ||||||
| callOrder.push("complete2"); | ||||||
| }); | ||||||
|
|
||||||
| await tx.completeByFeePayer(mockFeePayer1, mockFeePayer2); | ||||||
|
|
||||||
| // Verify order: all prepareTransaction calls first, then all completeTxFee calls | ||||||
| expect(callOrder).toEqual([ | ||||||
| "prepare1", | ||||||
| "prepare2", | ||||||
| "complete1", | ||||||
| "complete2", | ||||||
| ]); | ||||||
| }); | ||||||
|
|
||||||
| it("should propagate errors from prepareTransaction", async () => { | ||||||
| const tx = ccc.Transaction.from({ | ||||||
| outputs: [ | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(100), | ||||||
| lock, | ||||||
| }, | ||||||
| ], | ||||||
| }); | ||||||
|
|
||||||
| const error = new Error("Prepare transaction failed"); | ||||||
| ( | ||||||
| mockFeePayer1.prepareTransaction as ReturnType<typeof vi.fn> | ||||||
| ).mockRejectedValue(error); | ||||||
|
|
||||||
| await expect(tx.completeByFeePayer(mockFeePayer1)).rejects.toThrow( | ||||||
| "Prepare transaction failed", | ||||||
| ); | ||||||
| }); | ||||||
|
|
||||||
| it("should propagate errors from completeTxFee", async () => { | ||||||
| const tx = ccc.Transaction.from({ | ||||||
| outputs: [ | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(100), | ||||||
| lock, | ||||||
| }, | ||||||
| ], | ||||||
| }); | ||||||
|
|
||||||
| const error = new Error("Complete fee failed"); | ||||||
| ( | ||||||
| mockFeePayer1.completeTxFee as ReturnType<typeof vi.fn> | ||||||
| ).mockRejectedValue(error); | ||||||
|
|
||||||
| await expect(tx.completeByFeePayer(mockFeePayer1)).rejects.toThrow( | ||||||
| "Complete fee failed", | ||||||
| ); | ||||||
| }); | ||||||
|
|
||||||
| it("should handle fee payer that modifies transaction in prepareTransaction", async () => { | ||||||
| const tx = ccc.Transaction.from({ | ||||||
| outputs: [ | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(100), | ||||||
| lock, | ||||||
| }, | ||||||
| ], | ||||||
| }); | ||||||
|
|
||||||
| const modifiedTx = ccc.Transaction.from({ | ||||||
| outputs: [ | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(100), | ||||||
| lock, | ||||||
| }, | ||||||
| { | ||||||
| capacity: ccc.fixedPointFrom(50), | ||||||
| lock, | ||||||
| }, | ||||||
| ], | ||||||
| }); | ||||||
|
|
||||||
| ( | ||||||
| mockFeePayer1.prepareTransaction as ReturnType<typeof vi.fn> | ||||||
| ).mockResolvedValue(modifiedTx); | ||||||
|
|
||||||
| await tx.completeByFeePayer(mockFeePayer1); | ||||||
|
|
||||||
| // prepareTransaction is called with a clone of the original transaction | ||||||
| expect(mockFeePayer1.prepareTransaction).toHaveBeenCalled(); | ||||||
| const prepareCallArg = ( | ||||||
| mockFeePayer1.prepareTransaction as ReturnType<typeof vi.fn> | ||||||
| ).mock.calls[0][0] as ccc.Transaction; | ||||||
| expect(prepareCallArg).toBeInstanceOf(ccc.Transaction); | ||||||
| expect(prepareCallArg.outputs.length).toBe(1); | ||||||
| // completeTxFee should be called with the modified transaction returned by prepareTransaction | ||||||
| expect(mockFeePayer1.completeTxFee).toHaveBeenCalledWith(modifiedTx); | ||||||
| }); | ||||||
| }); | ||||||
| }); | ||||||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
completeByFeePayermethod clones the transaction before passing it toprepareTransaction. Therefore,mockFeePayer1.prepareTransactionreceives a clone of the originaltx, not the originaltxobject itself. The current assertiontoHaveBeenCalledWith(tx)will likely fail becausetxis the original transaction instance, not the cloned one passed to the mock. Consider asserting withexpect.any(ccc.Transaction)orexpect.objectContainingif specific properties of the cloned transaction need to be verified.