Conversation
|
Size Change: +51 kB (+1.47%) Total Size: 3.52 MB
|
There was a problem hiding this comment.
Pull request overview
This PR implements a new addSacTransferOperation method on TransactionBuilder that allows constructing SAC (Stellar Asset Contract) transfer transactions without requiring RPC simulation. The implementation handles the creation of authorization entries and ledger footprints based on the asset type and destination type. Additionally, the PR fixes a bug where TransactionBuilder.build() was not including the resource fee in the total transaction fee for Soroban transactions.
Changes:
- Added
addSacTransferOperationmethod to construct SAC transfer operations with appropriate auth entries and footprints for different asset/destination combinations - Fixed
TransactionBuilder.build()to include resource fees in the total transaction fee whensorobanDatais present - Refactored fee-bump transaction logic to correctly calculate inclusion fees by subtracting resource fees
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| types/index.d.ts | Added TypeScript definitions for SorobanFees interface and addSacTransferOperation method signature |
| src/transaction_builder.js | Implemented addSacTransferOperation with footprint construction logic and fixed fee calculation in build() method |
| test/unit/transaction_builder_test.js | Added comprehensive tests for different asset/destination combinations and edge cases; updated existing fee-bump test expectations |
| CHANGELOG.md | Documented the new feature and bug fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { SignerKey } from './signerkey'; | ||
| import { Memo } from './memo'; | ||
| // eslint-disable-next-line no-unused-vars | ||
| import { Asset } from './asset'; |
There was a problem hiding this comment.
Is ESLint complaining that we only use this import in the doc portion?
There was a problem hiding this comment.
Yeah the ESLint rules does not know that its being used for the jsdoc
| instructions: sorobanFees | ||
| ? sorobanFees.instructions | ||
| : defaultPaymentFees.instructions, | ||
| diskReadBytes: sorobanFees | ||
| ? sorobanFees.readBytes | ||
| : defaultPaymentFees.readBytes, | ||
| writeBytes: sorobanFees | ||
| ? sorobanFees.writeBytes | ||
| : defaultPaymentFees.writeBytes |
There was a problem hiding this comment.
Is it OK to have 0 for these?
| }), | ||
| ext: new xdr.SorobanTransactionDataExt(0), | ||
| resourceFee: new xdr.Int64( | ||
| sorobanFees ? sorobanFees.resourceFee : defaultPaymentFees.resourceFee |
| ); | ||
| }); | ||
|
|
||
| it('credit asset + source is issuer: omits source trustline', function () { |
There was a problem hiding this comment.
We could group tests in a nested describe block to make it clear they are related and reuse variables.
| const args = [ | ||
| nativeToScVal(source, { type: 'address' }), | ||
| nativeToScVal(destination, { type: 'address' }), | ||
| nativeToScVal(amount, { type: 'i128' }) | ||
| ]; |
There was a problem hiding this comment.
You can combine these 😉
| const args = [ | |
| nativeToScVal(source, { type: 'address' }), | |
| nativeToScVal(destination, { type: 'address' }), | |
| nativeToScVal(amount, { type: 'i128' }) | |
| ]; | |
| const args = nativeToScVal([source, destination, amount], { | |
| type: ['address', 'address', 'i128' ] | |
| }).vec(); |
| }) | ||
| }); | ||
|
|
||
| const footprint = new xdr.LedgerFootprint({ |
There was a problem hiding this comment.
you can/should use Contract.getFootprint for this one for brevity
Why
Users have requested a way to build SAC transfer transactions without the need to rely on an RPC. This would allow for lightweight clients to be built.
What
TransactionBuilder.addSacTransferOperationthat constructs the transfer operation with the appropriate authorization and ledger footprint.resourceFeeif it containssorobanData