Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

feat: Add remaining txpool methods #3719

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

amandesai01
Copy link

@amandesai01 amandesai01 commented Sep 24, 2022

Resolves #763

Breaking down problem to these sub-tasks:

  • Refactor processMap
    • Rename to something better
    • move to TransactionPool class
  • create txpool_contentFrom
  • create txpool_inspect
  • create txpool_status

@amandesai01 amandesai01 marked this pull request as draft September 24, 2022 23:55
@amandesai01
Copy link
Author

Not sure exactly which params should be displayed for txpool_inspect. Rest can be merged. Also, not sure with naming of processMap().

@amandesai01 amandesai01 marked this pull request as ready for review September 25, 2022 10:55
Copy link
Contributor

@MicaiahReid MicaiahReid left a comment

Choose a reason for hiding this comment

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

First I want to say thank you so much for making this PR! This is great work and is really appreciated.

I have a few small comments, a lot of which is about legacy code that I'd like to get fixed up before merging 😄

Aside from those comments, I think I'd like to see slightly more robust tests. I think the previous test for txpool_content are a good starting point to work off of.

If you come to a point where this is too much, let us know, we'd be happy to take over. But we really appreciate the work you've done and would love to have you take it over the finish line.

// The nonce keys are actual decimal numbers (as strings) and not
// hex literals (based on what geth returns).
const nonce = transaction.nonce.toBigInt().toString();
res[from][nonce] = transaction.toJSON() as Ethereum.Pool.Transaction<"private">;
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we will still need to pass in a Common to the transaction's toJSON field. This is used in the legacy transaction type to determine if the user is running Ganache with EIP-2718 activated. If not, transaction types did not yet exist and should not be included on the return object.

@@ -453,6 +455,33 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> {
return null;
}

public processMap(map: Map<string, Heap<TypedTransaction>>, address?: Address) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably should have come up with a better name for this function when we first reviewed it. But especially now that it's being used elsewhere, I think we should rename this. The trouble is deciding on the name! Perhaps groupTransactionsByOriginAndNonce? @davidmurdoch opinions?

@@ -453,6 +455,33 @@ export default class TransactionPool extends Emittery<{ drain: undefined }> {
return null;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we'll need a js-doc comment here. Could you add something like this? (feel free to refine)

Suggested change
/**
* Iterates over all transactions in each Heap for each key in the map.
* Returns the transactions grouped by origin and nonce in the format: `{
* origin1: { nonceA: ...txData, ... }, ...}`
*
* If `address` is specified, filters the result to only show transactions
* from `address`
* @param map
* @param address
* @returns Transactions grouped by origin and nonce.
*/

for (let i = 0; i < length; ++i) {
const transaction = array[i];

if(address && transaction.from.toString() != address.toString()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We're getting transaction.from.toString() twice, could we store this as a variable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also store address.toString() outside of the loop so it's only computed once?

@@ -120,4 +120,110 @@ describe("txpool", () => {
assert.deepStrictEqual(queued, {});
});
});

Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason github isn't formatting this file as typescript. I wonder what's going on with that 🤔

src/chains/ethereum/ethereum/src/api.ts Outdated Show resolved Hide resolved
}

/**
* Returns the number of transactions currently pending for inclusion in the next block(s), as well as the ones that are being scheduled for future execution only.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Returns the number of transactions currently pending for inclusion in the next block(s), as well as the ones that are being scheduled for future execution only.
* Returns the number of transactions currently pending inclusion in the next blocks, as well as the ones that are scheduled for future execution.

* Returns the number of transactions created by specified address currently pending for inclusion in the next block(s), as well as the ones that are being scheduled for future execution only.
*
* @param address - The account address
* @returns The transactions currently pending or queued in the transaction pool by address.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns The transactions currently pending or queued in the transaction pool by address.
* @returns The transactions from the specified address that are currently pending or queued in the transaction pool.

* @param address - The account address
* @returns The transactions currently pending or queued in the transaction pool by address.
* @example
* ```javascript
Copy link
Contributor

Choose a reason for hiding this comment

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

For some reason these examples aren't working in the generated docs - but the txpool_content example isn't working either. I can look into this!

Copy link
Contributor

Choose a reason for hiding this comment

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

I found it!
We'll need to add this to the top of all three txpool examples to keep the transactions from being mined so they actually stay in the pool:

await provider.send("miner_stop")

Also, for the txpool_contentFrom example can we send another transaction from a different address to showcase the fact that it filters by address?

Co-authored-by: Micaiah Reid <[email protected]>
@amandesai01
Copy link
Author

Hello team. Apologies for leaving this PR un-resolved. Is it still needed? If yes, I can complete it. Thanks in advance.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Add txpool methods to RPC
2 participants