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.

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

Add built-in transferCoinTransaction() function #20

Merged
merged 2 commits into from
Oct 12, 2023
Merged

Conversation

0xmaayan
Copy link
Collaborator

Description

Adds a built in transferCoinTransaction to Aptos.

const aptos = new Aptos()
const transaction = await aptos.transferCoinTransaction()

This function generates a transfer coin transaction that can be simulated and/or sign and submit. That is to have support for wallets and developers.

In addition, added a signAndSubmitTransaction API function for a single signer transaction to ease the user submission flow

Test Plan

coin
    ✓ it generates a transfer coin transaction with AptosCoin coin type (802 ms)
    ✓ it generates a transfer coin transaction with a custom coin type (481 ms)
    ✓ it transfers APT coin aomunt from sender to recipient (1609 ms)

@0xmaayan 0xmaayan requested a review from a team as a code owner October 11, 2023 21:08
Comment on lines 301 to 308
/**
* Queries the count of an account's coins
* Queries the count of an account's coins aggregated
*
* @param accountAddress The account address we want to get the total count for
* @returns An object { count : number }
* @returns An object { count : number } where `number` is the aggregated count of all account's coin
*/
async getAccountCoinsCount(args: { accountAddress: HexInput }): Promise<GetAccountCoinsCountResponse> {
const count = getAccountCoinsCount({ aptosConfig: this.config, ...args });
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 this query may be confusing. Is it supposed to be the number of coin types? Or combined between multiple coin types?

Let's be concrete

If I have 5 DogCoin and 7 CatCoin, would the number be:

  • 2 (the number of types of coins I have)
  • 12 (the aggregate number between both coins, which isn't really a useful value)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is an existing query in sdk v1. I believe it returns the former. can validate it in a second PR as it is not really relevant to this PR

src/api/coin.ts Outdated Show resolved Hide resolved
src/api/coin.ts Outdated Show resolved Hide resolved
src/internal/coin.ts Outdated Show resolved Hide resolved
tests/e2e/api/coin.test.ts Outdated Show resolved Hide resolved
Comment on lines +21 to +22
const txnDeserializer = new Deserializer(transaction.rawTransaction);
const rawTransaction = RawTransaction.deserialize(txnDeserializer);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is an interesting way of handling it, where you need to deserialize by passing the deserializer in. Might be interesting if we also just provide a way to deserialize the bytes directly (if you only have that).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe @xbtmatt has some thoughts about it

tests/e2e/api/coin.test.ts Outdated Show resolved Hide resolved
Comment on lines +69 to +70
const recipientCoins = await aptos.getAccountCoinsData({ accountAddress: recipient.accountAddress.toString() });
const senderCoinsAfter = await aptos.getAccountCoinsData({ accountAddress: sender.accountAddress.toString() });
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to consider unioning HexInput with AccountAddress or only taking in AccountAddress, since this seems to be a common pattern that is being repeated around.

The wallet adapter can be less stringent probably.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think most users will get this data from some servers, that will return the address as a string (or in some cases as Uint8Array) - that was the thought behind accepting HexInput as an input and not force them initialize AccountAddress when they want to query for an address

const aptos = new Aptos(config);
const sender = Account.generate({ scheme: SigningScheme.Ed25519 });
const recipient = Account.generate({ scheme: SigningScheme.Ed25519 });
await aptos.fundAccount({ accountAddress: sender.accountAddress.toString(), amount: 100000000 });
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we make these numbers constants for the test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, we need a test framework. will work on it after we have full test cover to our code (not a P0 for alpha version)

import { APTOS_COIN } from "../utils/const";
import { generateTransaction } from "./transaction_submission";

export async function transaferCoinTransaction(args: {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, we can't call it just transfer because everything is unioned together on a single interface?

Copy link
Collaborator Author

@0xmaayan 0xmaayan Oct 11, 2023

Choose a reason for hiding this comment

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

in the future, we will have support for FA, NFT, Object, etc. since they all under the same namespace we can't call each transfer. we can always have each related transfer function under each namespace, but that's just confusing for the user imo.
also, I think transferCoin makes sense, as this function transfers coin

@0xmaayan 0xmaayan requested a review from a team October 11, 2023 23:01
@0xmaayan 0xmaayan enabled auto-merge (squash) October 12, 2023 00:39
@0xmaayan 0xmaayan merged commit a5ba456 into main Oct 12, 2023
10 checks passed
@0xmaayan 0xmaayan deleted the transfer_coin branch October 12, 2023 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants