Skip to content

Integrate Tendermint mempool - #34

Merged
tzdybal merged 13 commits into
mainfrom
tzdybal/mempool
Apr 16, 2021
Merged

Integrate Tendermint mempool#34
tzdybal merged 13 commits into
mainfrom
tzdybal/mempool

Conversation

@tzdybal

@tzdybaltzdybal commented Mar 30, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Resolves#5.

@tzdybaltzdybal self-assigned this Mar 30, 2021
@codecov-io

codecov-io commented Apr 7, 2021

Copy link
Copy Markdown

Codecov Report

Merging #34 (2331bb5) into main (e553342) will increase coverage by 6.41%.
The diff coverage is 49.27%.

Impacted file tree graph

@@ Coverage Diff @@## main #34 +/- ##
==========================================
+ Coverage 43.82% 50.24% +6.41% 
==========================================
Files 4 6 +2 Lines 251 412 +161 ==========================================
+ Hits 110 207 +97 - Misses 123 172 +49 - Partials 18 33 +15
Impacted FilesCoverage Δ
rpcclient/local.go27.16% <35.48%> (+20.91%)⬆️
node/mempool.go51.61% <51.61%> (ø)
node/node.go58.82% <65.11%> (ø)
p2p/client.go72.58% <100.00%> (+0.22%)⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e553342...2331bb5. Read the comment docs.

@tzdybal

tzdybal commented Apr 12, 2021

Copy link
Copy Markdown
ContributorAuthor

I'm marking this as ready for review. For better visibility I'll create separate PR to remove use of reflect (ref: #38).

Currently I'm working on testing, so more tests will also be submitted in another PRs. Available unit test shows that after BroadcastTx executed against one node, another node receives Tx via full stack (RPC, ABCI/Mempool, P2P).

@tzdybal
tzdybal marked this pull request as ready for review April 12, 2021 09:00
@tzdybal
tzdybal requested review from Wondertan and liamsiApril 12, 2021 09:00
@tzdybaltzdybal mentioned this pull request Apr 12, 2021
Merged
liamsi
liamsi reviewed Apr 13, 2021
View reviewed changes

@liamsiliamsi left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks great. Left some minor comments. I'll review #39 now.

Comment threadrpcclient/local_test.go Outdated
Comment threadnode/node.go Outdated
Comment threadnode/node.go Outdated
Comment on lines +33 to +35
Mempool mempool.Mempool
mempoolIDs *mempoolIDs
incommingTxCh chan *p2p.Tx

@liamsiliamsiApr 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So these 3 fields basically acts as the mempool reactor. I'm wondering if we should capture them in a struct together too 🤔

Comment threadp2p/client.go
Comment threadrpcclient/local.go
Comment threadrpcclient/local.go
func (l *Local) BroadcastTxCommit(ctx context.Context, tx types.Tx) (*ctypes.ResultBroadcastTxCommit, error) {
// needs mempool
panic("not implemented!")
subscriber := "" //ctx.RemoteAddr()

@liamsiliamsiApr 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What does this line mean/do?

@tzdybaltzdybalApr 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Added comment to clarify this.

@liamsiliamsiApr 15, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting, I didn't know this (or maybe I forgot). Thanks for adding the comment. It clarifies that line.

@tzdybal
tzdybal requested a review from liamsiApril 15, 2021 15:17
liamsi
liamsi approved these changes Apr 15, 2021
View reviewed changes

@liamsiliamsi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@tzdybal
tzdybal merged commit 2ecdb45 into mainApr 16, 2021
@tzdybal
tzdybal deleted the tzdybal/mempool branch May 2, 2023 19:40
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liamsiliamsiliamsi approved these changes
@WondertanWondertanAwaiting requested review from Wondertan

Assignees

@tzdybaltzdybal

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Mempool

3 participants

@tzdybal@codecov-io@liamsi