Skip to content

Get rid of reflect in mempool code - #39

Merged
tzdybal merged 4 commits into
mainfrom
tzdybal/copy_mempool
Apr 16, 2021
Merged

Get rid of reflect in mempool code#39
tzdybal merged 4 commits into
mainfrom
tzdybal/copy_mempool

Conversation

@tzdybal

@tzdybaltzdybal commented Apr 12, 2021

Copy link
Copy Markdown
Contributor

This PR copies the mempool dependency into code tree and integrate the changes, to avoid using reflect in mempool related code.

Depends on #34.
Relates to #37.
Resolves#38.

@tzdybal
tzdybal requested review from Wondertan and liamsiApril 12, 2021 10:04
Comment threadnode/node.go
v := reflect.Indirect(reflect.ValueOf(next.Value))
f := v.FieldByName("tx")
tx := f.Bytes()
memTx := next.Value.(*mempool.MempoolTx)

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 happens if the cast fails?

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.

We will see a panic in tests and will be able to fix the issue.
Initially I created idiomatic error handling for this cast... But in this particular case I think we should just panic.

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.

I see, I briefly thought this was the other way around: you could send a Tx and crash the process / node but in this case the process is in control.

@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.

It would be helpful for reviewers to highlight which files were and how they were modified in github comments.

Comment threadmempool/clist_mempool.go
Comment threadmempool/clist_mempool.go
Comment threadmempool/mempool.go
@tzdybal
tzdybal changed the base branch from tzdybal/mempool to mainApril 16, 2021 06:23
@tzdybal
tzdybalforce-pushed the tzdybal/copy_mempool branch from 6d91a8c to a33ee44CompareApril 16, 2021 06:27
@codecov-io

Copy link
Copy Markdown

Codecov Report

Merging #39 (a33ee44) into main (2ecdb45) will increase coverage by 7.86%.
The diff coverage is 69.64%.

Impacted file tree graph

@@ Coverage Diff @@## main #39 +/- ##
==========================================
+ Coverage 51.68% 59.54% +7.86% 
==========================================
Files 6 10 +4 Lines 416 749 +333 ==========================================
+ Hits 215 446 +231 - Misses 168 251 +83 - Partials 33 52 +19 
Impacted FilesCoverage Δ
rpcclient/local.go27.16% <ø> (ø)
mempool/metrics.go15.15% <15.15%> (ø)
mempool/errors.go25.00% <25.00%> (ø)
mempool/clist_mempool.go76.53% <76.53%> (ø)
mempool/mempool.go81.25% <81.25%> (ø)
node/node.go66.19% <100.00%> (-0.47%)⬇️

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 2ecdb45...a33ee44. Read the comment docs.

@tzdybal
tzdybal merged commit a1820ca into mainApr 16, 2021
@tzdybal
tzdybal deleted the tzdybal/copy_mempool branch May 2, 2023 19:40
Sign up for freeto 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.

Get rid of reflection in mempool

3 participants

@tzdybal@codecov-io@liamsi