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 TS component of the First NFT with SDK tutorial #3428

Merged
merged 1 commit into from
Aug 25, 2022
Merged

Conversation

banool
Copy link
Contributor

@banool banool commented Aug 24, 2022

Description

This PR updates the first NFT with SDK example to include Typescript. To facilitate this, I've done some other stuff:

Test Plan

SDK tests:

cd ecosystem/typescript/sdk
yarn test

Examples:

cd ecosystem/typescript/sdk/examples/typescript
yarn test

Docs site:

cd developer-docs-site
yarn start

This change is Reviewable

@banool banool force-pushed the banool/ts_first_nft branch from 649df61 to 8f8a7af Compare August 24, 2022 23:44
@banool banool changed the base branch from main to banool/rust_sdk_first_transaction_dev_doxs August 24, 2022 23:44
* Official Aptos Rust SDK -- TBA

## Step 2: Run the Example

Each SDK provides an examples directory. This tutorial covers the `simple-nft` example.

<Tabs>
Clone `aptos-core`:
Copy link
Contributor

Choose a reason for hiding this comment

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

just curious why we add a new doc instead of revising existing first_nft doc using the SDK? eventually, I expect we will only have one first_nft.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davidiw could probably speak more about the decision to write a new tutorial. I can see that the new tutorial is heavily inspired by the old one, but I think the intent is to focus less on theory and internals. I suppose it was easier to write a new tutorial afresh in this case.

Base automatically changed from banool/rust_sdk_first_transaction_dev_doxs to main August 25, 2022 07:40
@banool banool changed the base branch from main to banool/coin_client_uniformity August 25, 2022 18:25
@banool banool force-pushed the banool/ts_first_nft branch 3 times, most recently from ec66762 to a345cc7 Compare August 25, 2022 19:29
@banool banool changed the title banool/ts first nft Add TS component of the First NFT with SDK tutorial Aug 25, 2022
@banool banool marked this pull request as ready for review August 25, 2022 19:35
@banool banool force-pushed the banool/ts_first_nft branch from a345cc7 to ce75bb5 Compare August 25, 2022 19:37
Copy link
Contributor

@areshand areshand left a comment

Choose a reason for hiding this comment

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

Looking great, thanks for migrating to SDK

@@ -321,7 +397,7 @@ export class TokenClient {
*/
async getTokenForAccount(account: MaybeHexString, tokenId: TokenTypes.TokenId): Promise<TokenTypes.Token> {
const tokenStore: { type: Gen.MoveStructTag; data: any } = await this.aptosClient.getAccountResource(
account,
account instanceof HexString ? account.hex() : account,
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this? aptosClient handles this already by ensuring hex string is used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well without this it was failing if I used account instead of account.hex() when calling this function. Or perhaps it was the other one for which that was true, in which case this is just for uniformity. I'll investigate AptosClient.

Copy link
Contributor

Choose a reason for hiding this comment

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

interesting. please investigate.

"multisig_transaction": "ts-node multisig_transaction.ts",
"transfer_coin": "ts-node transfer_coin.ts",
"simple_nft": "ts-node simple_nft.ts",
"test": "run-s transfer_coin bcs_transaction multisig_transaction"
Copy link
Contributor

Choose a reason for hiding this comment

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

add simple_nft to test?

@banool banool force-pushed the banool/coin_client_uniformity branch 3 times, most recently from 4853f8a to 859065e Compare August 25, 2022 22:08
@banool banool force-pushed the banool/ts_first_nft branch from ce75bb5 to be81838 Compare August 25, 2022 22:08
@banool banool force-pushed the banool/coin_client_uniformity branch from 859065e to fbfd761 Compare August 25, 2022 22:09
@banool banool force-pushed the banool/ts_first_nft branch from be81838 to 51f0e85 Compare August 25, 2022 22:11
Base automatically changed from banool/coin_client_uniformity to main August 25, 2022 22:34
@banool banool force-pushed the banool/ts_first_nft branch from 51f0e85 to c416eff Compare August 25, 2022 22:39
@banool banool enabled auto-merge (squash) August 25, 2022 22:39
@github-actions
Copy link
Contributor

Forge is running with c416eff973e2163527a23d81c310c87824437e34

✅ Forge test success on c416eff973e2163527a23d81c310c87824437e34

performance benchmark with full nodes : 6261 TPS, 4784 ms latency, 6050 ms p99 latency,no expired txns
Test Ok

@banool banool disabled auto-merge August 25, 2022 23:02
@banool banool merged commit ca6a0a4 into main Aug 25, 2022
@banool banool deleted the banool/ts_first_nft branch August 25, 2022 23:02
banool added a commit that referenced this pull request Aug 25, 2022
0xchloe pushed a commit that referenced this pull request Aug 28, 2022
Markuze pushed a commit to Markuze/aptos-core that referenced this pull request Sep 2, 2022
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.

3 participants