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

[MINOR] migrate TransactionPool (& affiliated test) from 'core' to 'eth' #1251

Merged
merged 2 commits into from
Apr 15, 2019

Conversation

smatthewenglish
Copy link
Contributor

@smatthewenglish smatthewenglish commented Apr 10, 2019

PR description

Necessary pre-condition of using SyncState in TransactionPool as a filtering mechanism.

The gotcha was adding these lines:

testImplementation project(path: ':config', configuration: 'testSupportArtifacts')
integrationTestImplementation project(path: ':config', configuration: 'testSupportArtifacts')

to ethereum/eth/build.gradle, since TransactionPoolTest has ExecutionContextTestFixture which requires GenesisConfigFile, or anyway one of those files in tech.pegasys.pantheon.config.

Fixed Issue(s)

@@ -96,7 +96,7 @@ public boolean addRemoteTransaction(final Transaction transaction) {
return addTransaction;
}

boolean addLocalTransaction(final Transaction transaction) {
public boolean addLocalTransaction(final Transaction transaction) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You should move all of the TransactionPool related classes to eth. I believe that includes: TransactionPool, PendingTransactions, PendingTransactionListener, PendingTransactionDroppedListener.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah -- I was initially wondering about whether or not to do that -- I've updated the branch accordingly

@smatthewenglish smatthewenglish force-pushed the boris branch 4 times, most recently from 9a01b71 to 3e6a1e9 Compare April 10, 2019 21:09
@@ -55,4 +55,7 @@ dependencies {
testImplementation 'org.mockito:mockito-core'

jmhImplementation project(path: ':ethereum:core', configuration: 'testSupportArtifacts')

testImplementation project(path: ':config', configuration: 'testSupportArtifacts')
Copy link
Contributor

Choose a reason for hiding this comment

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

This like should be alphabetized with the rest of the testImplementation dependenceies. Between line 45 and 46

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch -- thank you.

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

Successfully merging this pull request may close these issues.

3 participants