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 eth_pendingTransactions() support #2227

Merged
merged 10 commits into from
Mar 8, 2024
Merged

Conversation

casiojapi
Copy link
Contributor

Description

Motivation and Context

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • Tests for the changes have been added (for bug fixes / features)
  • Requires Activation Code (Hard Fork)
  • Other information:

@Override
public TransactionResultDTO[] eth_pendingTransactions() {
JsonNode content = txPoolModule.content();
JsonNode pendingTransactionsNode = content.get(TxPoolModuleImpl.PENDING);

Check notice

Code scanning / CodeQL

Use of default toString() Note

Default toString(): TxHashParam inherits toString() from Object, and so is not suitable for printing.
return new TransactionResultDTO[0];
}

List<TransactionResultDTO> transactionDTOs = new ArrayList<>();

Check notice

Code scanning / CodeQL

Use of default toString() Note

Default toString(): TxHashParam inherits toString() from Object, and so is not suitable for printing.
@casiojapi casiojapi changed the title add eth_pendingTransaction support add eth_pendingTransactions() support Jan 23, 2024
rskj-core/src/main/java/org/ethereum/rpc/Web3Impl.java Outdated Show resolved Hide resolved
List<Transaction> pendingTransactions = web3InformationRetriever.getTransactions("pending");

// get list of accounts managed by the node
Set<String> managedAccountSet = new HashSet<>(Arrays.asList(personalModule.listAccounts()));
Copy link
Contributor

Choose a reason for hiding this comment

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

personalModule.listAccounts() throws an exception if personal wallet is disabled.

Alternatively, we could add a new pendingTransactions() method to EthModuleWallet interface and properly implement in derived classes, and then make use of it in this eth_pendingTransactions() like this getEthModule().pendingTransactions(), wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

you are still using personalModule.listAccounts() in this context which would throw an error if the personal module is disabled - eg by default in Testnet and Mainnet. What about moving this stuff to EthModuleWallet as was suggested in he previous comment?

List<Transaction> pendingTransactions = web3InformationRetriever.getTransactions("pending");

// get list of accounts managed by the node
Set<String> managedAccountSet = new HashSet<>(Arrays.asList(personalModule.listAccounts()));
Copy link
Contributor

Choose a reason for hiding this comment

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

you are still using personalModule.listAccounts() in this context which would throw an error if the personal module is disabled - eg by default in Testnet and Mainnet. What about moving this stuff to EthModuleWallet as was suggested in he previous comment?

@@ -189,6 +187,10 @@
}
}

public List<Transaction> ethPendingTransactions() {

Check notice

Code scanning / CodeQL

Missing Override annotation Note

This method overrides
EthModuleWallet.ethPendingTransactions
; it is advisable to add an Override annotation.
@casiojapi
Copy link
Contributor Author

pipeline:run

1 similar comment
@Vovchyk
Copy link
Contributor

Vovchyk commented Feb 8, 2024

pipeline:run

@@ -1863,7 +1863,7 @@ private EthModuleWallet getEthModuleWallet() {
if (wallet == null) {
ethModuleWallet = new EthModuleWalletDisabled();
} else {
ethModuleWallet = new EthModuleWalletEnabled(wallet);
ethModuleWallet = new EthModuleWalletEnabled(wallet, transactionPool);
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
ethModuleWallet = new EthModuleWalletEnabled(wallet, transactionPool);
ethModuleWallet = new EthModuleWalletEnabled(wallet, getTransactionPool());

better to use the getter instead of the field

@Vovchyk Vovchyk force-pushed the support-eth-pending-txs branch from 297cf4b to 5b7754c Compare March 7, 2024 16:08
Copy link

sonarqubecloud bot commented Mar 7, 2024

@Vovchyk Vovchyk merged commit 51c4fc3 into master Mar 8, 2024
10 checks passed
@Vovchyk Vovchyk deleted the support-eth-pending-txs branch March 8, 2024 08:17
@aeidelman aeidelman added this to the Arrowhead 6.1.0 milestone Apr 8, 2024
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.

3 participants