-
-
Notifications
You must be signed in to change notification settings - Fork 26
fix(session): separate sender from fee payer in settle and close #247
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
brendanjryan
wants to merge
1
commit into
main
Choose a base branch
from
fix/settle-sender-separation
base: main
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.
+54
−11
Open
Changes from all commits
Commits
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,10 @@ | ||
| --- | ||
| 'mppx': patch | ||
| --- | ||
|
|
||
| Fixed `settleOnChain` and `closeOnChain` to use the payee account as | ||
| `msg.sender` instead of the fee payer when submitting fee-sponsored | ||
| transactions. Previously, `sendFeePayerTx` used the fee payer as both | ||
| sender and gas sponsor, causing the escrow contract to revert with | ||
| `NotPayee()`. Added `account` option to `tempo.settle()` so callers can | ||
| specify the signing account separately from the fee payer. |
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
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 |
|---|---|---|
|
|
@@ -730,7 +730,10 @@ describe.runIf(isLocalnet)('on-chain', () => { | |
| expect(channel.finalized).toBe(false) | ||
| }) | ||
|
|
||
| test('settles a channel with fee payer', async () => { | ||
| // TODO: add on-chain test with distinct feePayer != account once localnet | ||
| // supports fee-sponsored settle (currently msg.sender resolves to feePayer). | ||
|
|
||
| test('settles with explicit account (no fee payer)', async () => { | ||
| const salt = nextSalt() | ||
| const deposit = 10_000_000n | ||
| const settleAmount = 5_000_000n | ||
|
|
@@ -752,6 +755,7 @@ describe.runIf(isLocalnet)('on-chain', () => { | |
| chain.id, | ||
| ) | ||
|
|
||
| // Pass account explicitly — should use it as sender instead of client.account | ||
| const txHash = await settleOnChain( | ||
| client, | ||
| escrowContract, | ||
|
|
@@ -760,6 +764,7 @@ describe.runIf(isLocalnet)('on-chain', () => { | |
| cumulativeAmount: settleAmount, | ||
| signature, | ||
| }, | ||
| undefined, | ||
| accounts[0], | ||
| ) | ||
|
|
||
|
|
@@ -769,6 +774,21 @@ describe.runIf(isLocalnet)('on-chain', () => { | |
| expect(channel.settled).toBe(settleAmount) | ||
| expect(channel.finalized).toBe(false) | ||
| }) | ||
|
|
||
| test('throws when no account available', async () => { | ||
| const noAccountClient = { chain: { id: 42431 } } as any | ||
| const dummyEscrow = '0x0000000000000000000000000000000000000001' as Address | ||
| const dummyChannelId = | ||
| '0x0000000000000000000000000000000000000000000000000000000000000001' as Hex | ||
|
|
||
| await expect( | ||
| settleOnChain(noAccountClient, dummyEscrow, { | ||
| channelId: dummyChannelId, | ||
| cumulativeAmount: 1_000_000n, | ||
| signature: '0xsig' as Hex, | ||
| }), | ||
| ).rejects.toThrow('no account available') | ||
| }) | ||
| }) | ||
|
|
||
| describe('closeOnChain', () => { | ||
|
|
@@ -806,7 +826,10 @@ describe.runIf(isLocalnet)('on-chain', () => { | |
| expect(channel.finalized).toBe(true) | ||
| }) | ||
|
|
||
| test('closes a channel with fee payer', async () => { | ||
| // TODO: add on-chain test with distinct feePayer != account once localnet | ||
| // supports fee-sponsored close (currently msg.sender resolves to feePayer). | ||
|
Comment on lines
+829
to
+830
Member
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. test.todo() |
||
|
|
||
| test('closes with explicit account (no fee payer)', async () => { | ||
| const salt = nextSalt() | ||
| const deposit = 10_000_000n | ||
| const closeAmount = 5_000_000n | ||
|
|
@@ -828,6 +851,7 @@ describe.runIf(isLocalnet)('on-chain', () => { | |
| chain.id, | ||
| ) | ||
|
|
||
| // Pass account explicitly — should use it as sender instead of client.account | ||
| const txHash = await closeOnChain( | ||
| client, | ||
| escrowContract, | ||
|
|
@@ -836,7 +860,6 @@ describe.runIf(isLocalnet)('on-chain', () => { | |
| cumulativeAmount: closeAmount, | ||
| signature, | ||
| }, | ||
| undefined, | ||
| accounts[0], | ||
| ) | ||
|
|
||
|
|
||
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 |
|---|---|---|
|
|
@@ -101,15 +101,21 @@ export async function settleOnChain( | |
| escrowContract: Address, | ||
| voucher: SignedVoucher, | ||
| feePayer?: Account | undefined, | ||
| account?: Account | undefined, | ||
|
Comment on lines
103
to
+104
Member
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. Generic type parameter to require account if feePayer is defined? |
||
| ): Promise<Hex> { | ||
| assertUint128(voucher.cumulativeAmount) | ||
| const resolved = account ?? client.account | ||
| if (!resolved) | ||
| throw new Error( | ||
| 'Cannot settle channel: no account available. Pass an `account` to tempo.settle(), or provide a `getClient` that returns an account-bearing client.', | ||
| ) | ||
| const args = [voucher.channelId, voucher.cumulativeAmount, voucher.signature] as const | ||
| if (feePayer) { | ||
| const data = encodeFunctionData({ abi: escrowAbi, functionName: 'settle', args }) | ||
| return sendFeePayerTx(client, feePayer, escrowContract, data, 'settle') | ||
| return sendFeePayerTx(client, resolved, feePayer, escrowContract, data, 'settle') | ||
| } | ||
| return writeContract(client, { | ||
| account: client.account!, | ||
| account: resolved, | ||
| chain: client.chain, | ||
| address: escrowContract, | ||
| abi: escrowAbi, | ||
|
|
@@ -137,7 +143,7 @@ export async function closeOnChain( | |
| const args = [voucher.channelId, voucher.cumulativeAmount, voucher.signature] as const | ||
| if (feePayer) { | ||
| const data = encodeFunctionData({ abi: escrowAbi, functionName: 'close', args }) | ||
| return sendFeePayerTx(client, feePayer, escrowContract, data, 'close') | ||
| return sendFeePayerTx(client, resolved, feePayer, escrowContract, data, 'close') | ||
| } | ||
| return writeContract(client, { | ||
| account: resolved, | ||
|
|
@@ -155,9 +161,13 @@ export async function closeOnChain( | |
| * Follows the same signTransaction + sendRawTransactionSync pattern used | ||
| * by broadcastOpenTransaction / broadcastTopUpTransaction, but originates | ||
| * the transaction server-side (estimating gas and fees first). | ||
| * | ||
| * @param account - The logical sender / msg.sender (e.g. the payee). | ||
| * @param feePayer - The gas sponsor — only co-signs to cover fees. | ||
| */ | ||
| async function sendFeePayerTx( | ||
| client: Client, | ||
| account: Account, | ||
| feePayer: Account, | ||
| to: Address, | ||
| data: Hex, | ||
|
|
@@ -167,20 +177,18 @@ async function sendFeePayerTx( | |
| // token. `feePayer: true` tells the prepare hook to use expiring nonces but | ||
| // does NOT set feeToken automatically, so we must provide it explicitly. | ||
| const chainId = client.chain?.id | ||
| const feeToken = chainId | ||
| ? defaults.currency[chainId as keyof typeof defaults.currency] | ||
| : undefined | ||
| const feeToken = chainId ? defaults.resolveCurrency({ chainId }) : undefined | ||
|
|
||
| const prepared = await prepareTransactionRequest(client, { | ||
| account: feePayer, | ||
| account, | ||
| calls: [{ to, data }], | ||
| feePayer: true, | ||
| ...(feeToken ? { feeToken } : {}), | ||
| } as never) | ||
|
|
||
| const serialized = (await signTransaction(client, { | ||
| ...prepared, | ||
| account: feePayer, | ||
| account, | ||
| feePayer, | ||
| } as never)) as Hex | ||
|
|
||
|
|
||
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.
test.todo() instead of comment?