Skip to content

VDB-1368: add new geth patch as diff source#119

Merged
elizabethengelman merged 19 commits intostagingfrom
VDB-1368-add-new-geth-patch-as-diff-source
Aug 11, 2020
Merged

VDB-1368: add new geth patch as diff source#119
elizabethengelman merged 19 commits intostagingfrom
VDB-1368-add-new-geth-patch-as-diff-source

Conversation

@elizabethengelman
Copy link
Copy Markdown

@elizabethengelman elizabethengelman commented Aug 3, 2020

companion PR in transformers repo to add an extractDiffs dockerfile: sky-ecosystem/vdb-mcd-transformers#259

@elizabethengelman elizabethengelman changed the title Vdb 1368 add new geth patch as diff source VDB-1368: add new geth patch as diff source Aug 3, 2020
@elizabethengelman elizabethengelman force-pushed the VDB-1368-add-new-geth-patch-as-diff-source branch from 35f4895 to 3321fa1 Compare August 6, 2020 14:26
Copy link
Copy Markdown

@rmulhol rmulhol left a comment

Choose a reason for hiding this comment

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

😤

)

// Method is our custom method struct
type Method struct {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Not sure I follow why this file is included in this PR?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think was probably added in due to a rebasing error. I'll remove it. ✂️

for _, accountStorage := range account.Storage {
rawDiff, formatErr := types.FromGethStateDiff(account, &stateDiff, accountStorage)
if formatErr != nil {
errs <- formatErr
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

do we potentially want to return after this? Not sure if sending an error will lead the caller to try and close the channel (leading to a panic on subsequent sends)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This piece is a little fuzzy for me - I think you're right, that if we were to send an error to the errs channel, then it's possible that the next accountStorage we're iterating through could try to send a diff (or an error) to a channel that's already been closed... I think. Also, it probably makes sense to stop processing diffs if the system is going to shut down anyway.

@@ -1,48 +0,0 @@
FROM golang:alpine as builder
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Probably good to remove the build/deploy here as well

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

good catch!

go.mod Outdated
github.com/onsi/gomega v1.10.0
github.com/oschwald/maxminddb-golang v1.5.0 // indirect
github.com/pborman/uuid v1.2.0 // indirect
github.com/pkg/errors v0.8.1
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Is this required bc of Geth? For some reason I feel like at some point we accidentally required this at some point when we really wanted the core errors package

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🤔 Not sure why that ended up in there, don't think it's necessary though.

@elizabethengelman elizabethengelman force-pushed the VDB-1368-add-new-geth-patch-as-diff-source branch from 541c51b to 816e343 Compare August 11, 2020 15:42
@elizabethengelman elizabethengelman merged commit 89a4c0b into staging Aug 11, 2020
@elizabethengelman elizabethengelman deleted the VDB-1368-add-new-geth-patch-as-diff-source branch August 11, 2020 18:29
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.

2 participants