-
Notifications
You must be signed in to change notification settings - Fork 93
Conduit: Algod importer implementation #1135
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
Conversation
Codecov Report
@@ Coverage Diff @@
## conduit #1135 +/- ##
==========================================
Coverage ? 60.65%
==========================================
Files ? 56
Lines ? 8435
Branches ? 0
==========================================
Hits ? 5116
Misses ? 2857
Partials ? 462 Help us with your feedback. Take ten seconds to tell us how you rate us. |
docs/rfc/0003-importer-interface.md
Outdated
type Importer interface { | ||
// GetBlock given any round number rnd fetches the block at that round | ||
// It returns an object of type BlockExportData defined in exporters plugin | ||
GetBlock(rnd uint64) (*exporters.BlockExportData, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the processor take block of type exporters.BlockExportData too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should. I think we're still trying to figure out what the best data format is to pass between all of the plugin.
Some discussion happening in #1119 (comment)
c6b9cb0
to
027ab87
Compare
cb0acac
to
777f722
Compare
777f722
to
6e4fe83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left some minor comments.
return err | ||
} | ||
if u.Scheme != "http" && u.Scheme != "https" { | ||
algodImp.cfg.NetAddr = "http://" + algodImp.cfg.NetAddr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we log this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its a good idea to log it
s = "" | ||
} | ||
|
||
func MockAlgodServerReturnsEmptyBlock() *httptest.Server { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have somewhat similar code in https://github.com/algorand/indexer/blob/develop/fetcher/fetcher_test.go#L103
Also, Will mentioned that block-generator might be doing the same. We should look to refactor these into commonly used conventions for mocking Algod in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Also MockAlgodServerReturnsEmptyBlock()
is intentionally exported for other plugins to utilize in testing.
…god-importer-impl merge conduit into branch
Co-authored-by: Eric Warehime <[email protected]>
Summary
Defines interface for importer plugins and a sample implementation of importer interface using
algod
rest endpoint.