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

feat: cache root testnet accounts and destroy the ones created in tests #43

Merged
merged 70 commits into from
Sep 7, 2021

Conversation

willemneal
Copy link
Contributor

@willemneal willemneal commented Aug 23, 2021

  1. Cache root testnet accounts.

  2. Clean up test accounts like alice and bob at the end of the test run. This involved adding an AccountManager and subclass TestnetManager to keep track of accounts created during the test run. AccountManager also handles all things related to accounts and signing. In the future we may want to use two separate classes for these two needs (spawning/cleaning accounts; handling transactions & signing for a given account).

  3. Restructure the project with the goal of removing circular dependencies. This includes adding new interface NearAccount.

  4. Tests can now skip beforeAll, since the runner instance will wait for the function passed to create to complete before executing run calls. That is, beforeAll is no longer required, and this can be defined at the top level of tests:

    const runner = Runner.create(async ({root}) => ({
      contract: await root.createAndDeploy(
        'status-message',
        path.join(__dirname, 'build', 'debug', 'status_message.wasm'),
      ),
      ali: await root.createAccount('ali'),
    }));
  5. Use jest.concurrent to make tests actually-concurrent (they weren't before) which speeds up test runs significantly and ensures that all near-runner works as designed

  6. Use near-units to wrap account balances and clean up assertions about account balances. Example:

    expect(originalBalance.toBigInt()).toBeLessThan(newBalance.toBigInt());
  7. Add new JSON RPC wrapper

@willemneal willemneal force-pushed the feat/account-manager branch from c912bfa to a8ad2a4 Compare August 23, 2021 22:20
@willemneal willemneal marked this pull request as ready for review August 24, 2021 15:38
@willemneal willemneal requested a review from chadoh August 24, 2021 15:38
@willemneal willemneal force-pushed the feat/account-manager branch from 0f89471 to f88daa6 Compare August 24, 2021 19:55
src/account/account-manager.ts Outdated Show resolved Hide resolved
src/account/account-manager.ts Outdated Show resolved Hide resolved
src/account/account-manager.ts Outdated Show resolved Hide resolved
src/account/account-manager.ts Outdated Show resolved Hide resolved
src/account/account-manager.ts Outdated Show resolved Hide resolved
src/account/account-manager.ts Outdated Show resolved Hide resolved
src/account/account-manager.ts Outdated Show resolved Hide resolved
src/account/account-manager.ts Show resolved Hide resolved
src/account/utils.ts Outdated Show resolved Hide resolved
src/account/account.ts Outdated Show resolved Hide resolved
src/account/near-account.ts Show resolved Hide resolved
src/account/near-account.ts Outdated Show resolved Hide resolved
src/account/utils.ts Outdated Show resolved Hide resolved
src/account/utils.ts Outdated Show resolved Hide resolved
src/jsonrpc.ts Outdated
const stateStaked = new BN(state.storage_usage).mul(costPerByte);
const staked = new BN(state.locked);
const totalBalance = new BN(state.amount).add(staked);
const availableBalance = totalBalance.sub(BN.max(staked, stateStaked));
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this math to me? Why add staked to amount just to subtract it again? Why wouldn't we always want:

Suggested change
const availableBalance = totalBalance.sub(BN.max(staked, stateStaked));
const availableBalance = totalBalance.sub(staked).sub(stateStaked);

Or, equivalently:

Suggested change
const availableBalance = totalBalance.sub(BN.max(staked, stateStaked));
const availableBalance = BN(state.amount).sub(stateStaked);

Copy link
Contributor

Choose a reason for hiding this comment

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

near/near-api-js#435

Seems like maybe the same bucket of tokens can be used for storage staking and delegation/validator staking?

src/runtime/index.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/runtime/runtime.ts Outdated Show resolved Hide resolved
@willemneal willemneal force-pushed the feat/account-manager branch from 5b67eb9 to 44145aa Compare August 26, 2021 17:42
Copy link
Contributor

@chadoh chadoh left a comment

Choose a reason for hiding this comment

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

I updated the description of this PR with what I think are the main things that were done here. Feel free to edit my descriptions and add others.

I lost the ability to make sense of this PR a long time ago, so I'm marking as Approved so we can merge it and move on (after tests are fixed, obviously). Let's make smaller PRs from here on out, please.

scripts/delete-accounts.ts Show resolved Hide resolved
src/jsonrpc.ts Outdated Show resolved Hide resolved
@chadoh chadoh force-pushed the feat/account-manager branch from 5213deb to 35d301a Compare September 6, 2021 19:05
Willem Wyndham and others added 2 commits September 7, 2021 09:59
src/jsonrpc.ts Outdated Show resolved Hide resolved
src/jsonrpc.ts Outdated Show resolved Hide resolved
src/jsonrpc.ts Outdated Show resolved Hide resolved
Willem Wyndham and others added 3 commits September 7, 2021 10:50
Co-authored-by: Chad Ostrowski <221614+chadoh@users.noreply.github.com>
@willemneal willemneal merged commit 2e705b1 into main Sep 7, 2021
This was linked to issues Sep 7, 2021
@chadoh chadoh deleted the feat/account-manager branch September 7, 2021 17:23
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.

Add more to accounts interface [Feature] Cache testnet accounts
2 participants