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

[SDKv2] Update the Faucet class and fix param naming #10

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

0xmigo
Copy link
Contributor

@0xmigo 0xmigo commented Oct 11, 2023

Description

  1. Fix naming of the REST param from accountAddress to address
  2. Add unit test
  3. Add Faucet to Aptos

Test Plan

pnpm test

Related Links

Copy link
Contributor Author

0xmigo commented Oct 11, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@0xmigo 0xmigo changed the title Update the Faucet class and fix param naming [SDKv2] Update the Faucet class and fix param naming Oct 11, 2023
@0xmigo 0xmigo requested review from 0xmaayan, gregnazario, heliuchuan, xbtmatt and a team October 11, 2023 02:15
@0xmigo 0xmigo marked this pull request as ready for review October 11, 2023 02:15
Copy link
Contributor

@xbtmatt xbtmatt left a comment

Choose a reason for hiding this comment

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

run pnpm fmt && pnpm lint

src/api/faucet.ts Outdated Show resolved Hide resolved
src/internal/faucet.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@gregnazario gregnazario left a comment

Choose a reason for hiding this comment

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

Let's not add anything without a test now that we have CI working

src/internal/faucet.ts Outdated Show resolved Hide resolved
@0xmigo 0xmigo force-pushed the jin/Update_fix_faucet branch from 73d82cd to b02f106 Compare October 11, 2023 16:13
src/api/faucet.ts Outdated Show resolved Hide resolved
tests/e2e/helper.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@0xmaayan 0xmaayan left a comment

Choose a reason for hiding this comment

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

can we add tests for faucet query?
fund an account and make sure this account exist on chain

amount: number;
timeoutSecs?: number;
}): Promise<Array<string>> {
const { aptosConfig, accountAddress, amount } = args;
const timeoutSecs = args.timeoutSecs ?? DEFAULT_TXN_TIMEOUT_SEC;
const { data } = await postAptosFaucet<any, Array<string>>({
aptosConfig,
body: null,
path: "mint",
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 land Greg's stuff first? It switches us to fund.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use fund

@0xmigo 0xmigo force-pushed the jin/Update_fix_faucet branch from b02f106 to d19eaf5 Compare October 11, 2023 17:24
@0xmigo 0xmigo requested a review from 0xmaayan October 11, 2023 17:27
Copy link
Collaborator

@0xmaayan 0xmaayan left a comment

Choose a reason for hiding this comment

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

lgtm

const testAccount = Account.generate({ scheme: SigningScheme.Ed25519 });

// Fund the account
await aptos.fundAccount({ accountAddress: testAccount.accountAddress.toString(), amount: 10_000_000 });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, this I see is why you don't want to pass the string, and instead the account address, we'll have to review this whole input strategy afterwards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. Including the args as input param on some places, we can revisit after Alpha :)

@gregnazario gregnazario merged commit 659c2f4 into main Oct 11, 2023
5 checks passed
@gregnazario gregnazario deleted the jin/Update_fix_faucet branch October 11, 2023 17:30
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.

5 participants