-
Notifications
You must be signed in to change notification settings - Fork 295
feat(sdk-coin-sui): fee payer signing support #6423
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
base: master
Are you sure you want to change the base?
Conversation
8afd635
to
1d2bd29
Compare
7907bc2
to
4b02833
Compare
4b02833
to
095c6d3
Compare
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.
Pull Request Overview
This PR introduces fee payer (sponsor) signing support for Sui transactions, allowing gas fees to be paid by a third party rather than the transaction sender. This implements a gas tank pattern where a sponsor can cover the transaction costs.
- Adds fee payer signature handling in transaction builders and core transaction classes
- Introduces
sponsor
field to gas data configuration for identifying the fee payer - Updates broadcast functionality to handle transactions with multiple signers (sender + fee payer)
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
test/unit/transactionBuilder/feePayer.ts | Comprehensive test suite for fee payer functionality including success and failure scenarios |
src/sui.ts | Updates broadcast logic to handle transactions with fee payer signatures |
src/lib/utils.ts | Adds utility method for executing transactions with multiple signers |
src/lib/transferBuilder.ts | Integrates fee payer signature handling in transfer transaction builds |
src/lib/transactionBuilder.ts | Core fee payer signing logic and sponsor validation |
src/lib/transaction.ts | Fee payer signature storage, serialization, and broadcast format updates |
src/lib/tokenTransferBuilder.ts | Integrates fee payer signature handling in token transfer builds |
src/lib/mystenlab/builder/TransactionDataBlock.ts | Adds sponsor field support to gas configuration schema |
src/lib/iface.ts | Defines GasData interface with optional sponsor field |
src/lib/customTransactionBuilder.ts | Integrates fee payer signature handling in custom transaction builds |
import { KeyPair } from '../../../src/lib/keyPair'; | ||
import { SuiTransactionType } from '../../../src/lib/iface'; | ||
|
||
describe('Sui Fee Payer (Gas Tank) Builder', () => { |
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.
There's a typo in the title: 'sigining' should be 'signing' in the PR title.
Copilot uses AI. Check for mistakes.
const signable = this.transaction.signablePayload; | ||
const signature = signer.signMessageinUint8Array(signable); | ||
const signatureBuffer = Buffer.from(signature); | ||
this.transaction.addSignature({ pub: signer.getKeys().pub }, signatureBuffer); | ||
this.transaction.setSerializedSig({ pub: signer.getKeys().pub }, signatureBuffer); |
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 signImplementation method has been modified to duplicate signing logic that already exists in the Transaction.sign() method. This creates code duplication and potential maintenance issues. Consider calling this.transaction.sign(signer) instead of reimplementing the signing logic.
const signable = this.transaction.signablePayload; | |
const signature = signer.signMessageinUint8Array(signable); | |
const signatureBuffer = Buffer.from(signature); | |
this.transaction.addSignature({ pub: signer.getKeys().pub }, signatureBuffer); | |
this.transaction.setSerializedSig({ pub: signer.getKeys().pub }, signatureBuffer); | |
this.transaction.sign(signer); |
Copilot uses AI. Check for mistakes.
this.setSerializedSig(publicKey, signature); | ||
this.serialize(); |
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 addSignature method now calls setSerializedSig, but this creates a dependency where addSignature must always be called after setSerializedSig is available. This could cause issues if the calling order changes. Consider whether this logic belongs in addSignature or if the serialization should be handled separately.
this.setSerializedSig(publicKey, signature); | |
this.serialize(); | |
// Serialization should be handled separately after adding the signature. |
Copilot uses AI. Check for mistakes.
result['sponsorSignature'] = toB64(this._serializedFeePayerSig); | ||
} | ||
|
||
return JSON.stringify(result); |
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 toBroadcastFormat method now returns JSON instead of the previous string format. This appears to be a breaking change that could affect existing consumers of this API. Ensure this change is intentional and properly documented as a breaking change.
Copilot uses AI. Check for mistakes.
const signatures = feePayerSignature ? [senderSignature, feePayerSignature] : [senderSignature]; | ||
return this.executeTransactionBlock(url, serializedTx, signatures); |
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.
[nitpick] The executeTransactionBlockWithMultipleSigners method creates an array and then immediately passes it to executeTransactionBlock. This adds unnecessary complexity. Consider simplifying the logic or making the conditional behavior more explicit.
const signatures = feePayerSignature ? [senderSignature, feePayerSignature] : [senderSignature]; | |
return this.executeTransactionBlock(url, serializedTx, signatures); | |
if (feePayerSignature) { | |
return this.executeTransactionBlock(url, serializedTx, [senderSignature, feePayerSignature]); | |
} else { | |
return this.executeTransactionBlock(url, serializedTx, [senderSignature]); | |
} |
Copilot uses AI. Check for mistakes.
const feePayerAddress = feePayerKeyPair.getAddress(); | ||
|
||
const txBuilder = factory.getTransferBuilder(); | ||
txBuilder.type(SuiTransactionType.Transfer); |
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.
are we going to stick with this transaction type, or create a new one?
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.
we will create new one in another ticket
Please update ticket and PR descriptions for better clarity and easier review. |
sure will add more details |
This change allow sponsor/fee address signing in case of sui, currently sui fee amount is paid from sender address so there was no need set fee address and fee signature, but for new sponsored transaction we will provide fee adress and fee signature as well
TICKET: WIN-6028