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 provider.getTransactionCount #101

Merged
merged 7 commits into from
Apr 24, 2022

Conversation

iadnanmalik
Copy link
Contributor

@iadnanmalik iadnanmalik commented Apr 24, 2022

Closes #94

The getTransactionCount function takes in a string and a blockTag object and returns a number which is the count of the transactions. Format of the already written code has been followed.

Any suggestions or changes are most welcome. :)

@vercel
Copy link

vercel bot commented Apr 24, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
essential-eth ✅ Ready (Inspect) Visit Preview Apr 24, 2022 at 8:19PM (UTC)

@@ -33,7 +33,8 @@ type RPCMethodName =
| 'eth_chainId'
| 'eth_gasPrice'
| 'eth_getBalance'
| 'eth_getTransactionByHash';
| 'eth_getTransactionByHash'
| 'eth_getTransactionCount';
Copy link
Owner

Choose a reason for hiding this comment

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

Awesome addition! Looks clean 🧼

const ethersProvider = new ethers.providers.StaticJsonRpcProvider(rpcUrl);
const web3Provider = new Web3(rpcUrl);
const [eeTC, ethersTC, web3TC] = await Promise.all([
eeProvider.getTransactionCount(address, blockTag),
Copy link
Owner

Choose a reason for hiding this comment

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

This is SUCH a good test since it covers web3 and ethers both 🙏

Copy link
Owner

@dawsbot dawsbot left a comment

Choose a reason for hiding this comment

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

It's almost perfect from the first glance!

I'm going to add the tsdoc comments and merge it 🙏

@@ -300,6 +300,16 @@ export class JsonRpcProvider {
blockNumber.number - cleanedTransaction.blockNumber + 1;
return cleanedTransaction;
}

public async getTransactionCount(
Copy link
Owner

Choose a reason for hiding this comment

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

The only thing I can think of to add would be tsdoc comments above this function, which get auto-compiled into the live documentation via docusaurus.

I should add a contributing guideline, so this is not expected for you to know!

I'll take care of this from here and merge this @iadnanmalik 🙏

Copy link
Contributor

Choose a reason for hiding this comment

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

Haha same brainwaves, thought I'd beat you to it! Resolving my comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you so much for your kind words.
This was my first ever open source contribution ❤️

Copy link
Contributor

@arimgibson arimgibson left a comment

Choose a reason for hiding this comment

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

Ignore this, as Dawson already mentioned

This is looking good, thank you! I'll give it a closer look later when I get a chance. Only thing that stood out immediately was needing some inline TypeDoc documentation. Here's a good example: https://github.com/Earnifi/essential-eth/blob/6f549292b931aaac96bfd1d05bd9b997d0a521e3/src/providers/JsonRpcProvider.ts#L30

Let me know if you have any questions!

const transactionCount = (await this.post(
buildRPCPostBody('eth_getTransactionCount', [address, blockTag]),
)) as string;
return tinyBig(hexToDecimal(transactionCount)).toNumber();
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure tinyBig is needed here since we're always returning a decimal which is going to be within the JavaScript max range

image

@@ -0,0 +1,27 @@
import { debug } from 'console';
Copy link
Owner

Choose a reason for hiding this comment

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

I think this "debug" was left-over from testing. I'll remove this before merging too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes my bad! 😅

import { BlockTag } from '../../types/Block.types';
import { rpcUrls } from './rpc-urls';

const address = '0x0000000000000000000000000000000000000000';
Copy link
Owner

Choose a reason for hiding this comment

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

Turns out this address has 0 transactions in most blocks, so we want to use a much more active address for tests.

One example could be a hot-wallet for coinbase since that has a ton of traffic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we try something other than TinyBig?
With some different addresses?

describe('provider.getBalance gno', () => {
const rpcUrl = rpcUrls.gno;
it('should get latest equal to ethers', async () => {
await testGetTC(rpcUrl, 'latest');
Copy link
Owner

@dawsbot dawsbot Apr 24, 2022

Choose a reason for hiding this comment

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

These test-cases could cover more options.

One would be passing in a blockNumber as a decimal. Here is an example of a test which does not pass:

// on mainnet
  it.only('should tx count up to block number equal to ethers and web3', async () => {
    await testGetTC(rpcUrl, 14649390);
  });

@dawsbot
Copy link
Owner

dawsbot commented Apr 24, 2022

Wahoo, your first PR merged @iadnanmalik ! I did a few changes to it which I will try to summarize in a "contribution guidelines" soon :)

Feel free to take on another issue if you'd like! Let me know and I will assign the issue for any you are interested in 🙏

@dawsbot dawsbot changed the title Added getTransactionCount, followed by a jest test ✨ Add provider.getTransactionCount Apr 24, 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.

Add getTransactionCount() method to JsonRpcProvider
3 participants