Skip to content
This repository has been archived by the owner on Nov 5, 2023. It is now read-only.

Add accounts, blocks, & network status method tests #512

Merged

Conversation

JohnGuilding
Copy link
Collaborator

@JohnGuilding JohnGuilding commented Feb 8, 2023

What is this PR doing?

Add tests for provider Accounts, Blocks & Network Status Methods.

Relevant methods:

  • getBalance - no test added as implicitly tested in many places
  • getCode - test added
  • getStorageAt - test added
  • getTransactionCount - method & test added
  • getBlock - test added
  • getBlockWithTransactions - test added
  • getNetwork - test added
  • getBlockNumber - test added
  • getGasPrice - test added
  • getFeeData - test added
  • .ready - test added

How can these changes be manually tested?

Run integration tests

Does this PR resolve or contribute to any issues?

#380

Checklist

  • I have manually tested these changes
  • Post a link to the PR in the group chat

Guidelines

  • If your PR is not ready, mark it as a draft
  • The resolve conversation button is for reviewers, not authors
    • (But add a 'done' comment or similar)

@github-actions github-actions bot added the contracts Smart contract related label Feb 8, 2023
@JohnGuilding JohnGuilding force-pushed the bls-provider-accounts-blocks-and-network-status-methods branch from 0c53981 to ed6766a Compare February 14, 2023 15:56
@JohnGuilding JohnGuilding marked this pull request as ready for review February 14, 2023 15:56
Copy link
Collaborator

@voltrevo voltrevo left a comment

Choose a reason for hiding this comment

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

Couple of things, looks good otherwise

Comment on lines 421 to 422
const expectedTransactionCountAtEaliestBlockTag =
await blsProvider.getTransactionCount(address, "earliest");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the same expression as transactionCountAtEaliestBlockTag. Should we be using regularProvider here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, changed

await blsProvider.getTransactionCount(address, "earliest");

// Assert
expect(transactionCount).to.equal(expectedTransactionCount);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is the blsProvider expected to give the same transaction count as regularProvider?

Shouldn't blsProvider be calling nonce() on the contract? And regularProvider would just return zero I suppose? Or error?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Does blsSigner.getAddress() return an EOA account somehow? I'd expect a BLSWallet.

Copy link
Collaborator Author

@JohnGuilding JohnGuilding Feb 15, 2023

Choose a reason for hiding this comment

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

Ah yeah missed this completely. Because blsProvider was just calling the ethers version of getTransactionCount, both calls were returning 0.

Does blsSigner.getAddress() return an EOA account somehow? I'd expect a BLSWallet.

blsSigner.getAddress() does return the bls wallet address.

Solution was to add a new getTransactionCount method onto the blsProvider itself that follows similar logic to BlsWalletWrapper, with the addition of being able to query by block tag. let me know what you think


// Act
const storage1 = await blsProvider.getStorageAt(mockERC20.address, 1);
console.log(storage1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

console.log

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed

@JohnGuilding JohnGuilding linked an issue Feb 16, 2023 that may be closed by this pull request
Copy link
Contributor

@blakecduncan blakecduncan left a comment

Choose a reason for hiding this comment

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

👍

@JohnGuilding JohnGuilding force-pushed the bls-provider-accounts-blocks-and-network-status-methods branch from 2b96f5c to c9c03a6 Compare February 21, 2023 09:55
@JohnGuilding JohnGuilding merged commit af44452 into main Feb 21, 2023
@JohnGuilding JohnGuilding deleted the bls-provider-accounts-blocks-and-network-status-methods branch February 21, 2023 10:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
clients contracts Smart contract related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BlsProvider Accounts, Blocks & Network Status Methods
3 participants