Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement SAC Transfer Functionality #27

Closed
wants to merge 3 commits into from
Closed

Implement SAC Transfer Functionality #27

wants to merge 3 commits into from

Conversation

ShantelPeters
Copy link
Contributor

This PR implements the functionality to enable interactions with Stellar Asset Contracts (SACs) in the Basic Payment App.

Changes Made
  • Implemented createContractTransferTransaction function in src/lib/stellar/transactions.js
  • Function handles both regular payments and SAC transfers
  • Detects contract addresses using StrKey.isValidContract()
  • Uses Contract.call() for SAC transfers
  • Maintains existing functionality for regular payments
Addressing Previous Comments
  • Moved the SAC transfer logic from walletStore.sign() to a separate function in transactions.js
  • Transaction is now built before being presented to the user for signing
  • The function checks whether the recipient address is a contract or a regular address
  • Consistent with other transaction-creating functions in the module

TODO

  • Update previewPaymentTransaction in +page.svelte to use the new createContractTransferTransaction function
  • Implement RPC server simulation for contract interactions (if required)

Testing

  • Tested with both regular Stellar accounts and contract addresses
  • Verified correct handling of different asset types

Please review the changes and let me know if any further modifications are needed.

ElliotFriend
ElliotFriend reviewed Oct 7, 2024
View reviewed changes
Copy link
Contributor

@ElliotFriend ElliotFriend left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, there @ShantelPeters! This is looking pretty good so far. Couple comments and notes, but I think you're gettin' there.

Thanks again!

src/lib/stores/walletStore.js
Comment on lines -49 to +51
// Don't include this in a real-life production application.
// It's just here to make the secret key accessible in case
// we need to do some manual transactions or something.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's keep this in here please.

src/lib/stores/walletStore.js
Comment on lines -96 to +135
sign: async ({ transactionXDR, network, pincode }) => {
signPayment: async ({ destination, amount, asset, network, pincode }) => {
try {
const keyManager = setupKeyManager()
const { publicKey } = get(walletStore)
const sourceAccount = new Account(publicKey, '0') // you have to Replace '0' with actual sequence number

let transaction;

if (contacts.isContractAddress(destination)) {
// Handle SAC transfer
const contract = new Contract(destination);
const transferOp = contract.call(
'transfer',
xdr.ScVal.scvAddress(Address.fromString(publicKey).toScAddress()),
xdr.ScVal.scvAddress(Address.fromString(destination).toScAddress()),
xdr.ScVal.scvI128(new xdr.Int128Parts({
lo: xdr.Uint64.fromString(amount),
hi: xdr.Int64.fromString('0')
})),
xdr.ScVal.scvSymbol(asset.getCode())
);

transaction = new TransactionBuilder(sourceAccount, { fee: '100', networkPassphrase: network })
.addOperation(transferOp)
.setTimeout(30)
.build();
} else {
// Handle regular payment
transaction = new TransactionBuilder(sourceAccount, { fee: '100', networkPassphrase: network })
.addOperation(Operation.payment({
destination: destination,
asset: asset,
amount: amount
}))
.setTimeout(30)
.build();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these changes are leftover from the previous PR? If so, let's remove them here. Philosophically, we really want this function to take a transaction that the user has already approved, and only attach a signature. We don't want to build the transaction after they've approved "something." The tx building (and yes, simulating SAC transfers) need to be done prior to them pushing the sign button, so they know exactly what they're signing.

src/lib/stellar/transactions.js
fee: maxFeePerOperation,
});

if (StrKey.isValidContract(destination)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're doing the right check (that destination is a valid contract), but I think we want to make that check before we call this function. Probably somewhere in the routes/dashboard/send/+page.svelte page. Probably by adding some permutation in the previewPaymentTransaction function.

src/lib/stellar/transactions.js
@@ -331,3 +332,58 @@ export async function createPathPaymentStrictReceiveTransaction({
network_passphrase: networkPassphrase,
}
}

/**
* Constructs and returns a Stellar transaction for transferring assets to a contract or account.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this application is intended to be a demonstrative resource, I think we should keep this function limited in scope to only performing SAC transfers. We have the other payment operation transaction already being handled by another function. Let's make this one focused on SAC transfers, solely.

src/lib/stellar/transactions.js
if (StrKey.isValidContract(destination)) {
// Transfer to a contract
const [assetCode, assetIssuer] = asset.split(':');
const contract = new Contract(destination);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I mentioned before, checking if destination is a contract, is the right check for this flow, but here the contract will need to be the contract for the asset. You'll need to use something like:

const contractId = new Asset(asset.split(':')[0], asset.split(':')[1]).contractId(Networks.TESTNET);
const contract = new Contract(contractId);

Then your use of contract.call(...) should work. Or, you could use the invokeContractFunction operation to add the operation to the transaction.

src/lib/stellar/transactions.js
Comment on lines +363 to +369
Address.fromString(source).toScVal(),
Address.fromString(destination).toScVal(),
xdr.ScVal.scvI128(new xdr.Int128Parts({
lo: xdr.Uint64.fromString(amountBigInt.toString(16).padStart(16, '0').slice(-16)),
hi: xdr.Int64.fromString(amountBigInt.toString(16).slice(0, -16) || '0')
})),
xdr.ScVal.scvSymbol(assetCode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use the nativeToScVal() function to make building these a little bit easier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ElliotFriend Okay sir I’m making the required changes

src/lib/stellar/transactions.js
);
}

const builtTransaction = transaction.setTimeout(standardTimebounds).build();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for contract invocations, you'll need to simulate the transaction. The rpc server has the prepareTransaction() function that will make this process pretty easy on you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sir I’m on it please

@ElliotFriend ElliotFriend linked an issue Oct 7, 2024 that may be closed by this pull request
Enable interactions with Stellar Asset Contracts #15
Open
@ShantelPeters ShantelPeters closed this by deleting the head repository Oct 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElliotFriend ElliotFriend ElliotFriend left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable interactions with Stellar Asset Contracts
3 participants
@ShantelPeters @ElliotFriend @akintewe