-
Notifications
You must be signed in to change notification settings - Fork 70
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
Add Configuration parameter 'SkipGethAdmin' to support skipping 'admin_peers' checks #38
Add Configuration parameter 'SkipGethAdmin' to support skipping 'admin_peers' checks #38
Conversation
…dmin_peers' checks
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.
Test TestStatus_NotSyncing uses a mock to fetch testdata/peers.json but the current PR skips the call so never reaches that code - what is the best way to address this?
Ah, how to test a function is not called in Go! Following should work :)
adminPeersSkipped := true
mockJSONRPC.On(
"CallContext",
ctx,
mock.Anything,
"admin_peers",
).Return(
nil,
).Run(
func(args mock.Arguments) {
adminPeersSkipped = false
},
).Maybe()
assert.True(t, adminPeersSkipped)
Is this the right approach via an environment variable?
Is storing this value in Client the correct place?
Should the skip happen higher up in the call chain?
Your current approach is fine :)
Should I add a line in the README?
Yes please!
… configuration is set to skipAdminCalls
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.
Looks great!! Left a minor field name change suggestion. 🙏 🙏 🙏
Motivation
This PR follows on from a discussion with @septerr:
It is not always possible to run your own nodes and in fact most dApps do not. For early development work, developers use node hosting services like QuikNode, Alchemy etc. However, these services do not permit usage of the
admin
Geth APIs for security reasons which is called by the/network/status
endpoint to gather connected node peers.While this PR is a bit hacky and goes against the current spirit of Rosetta of running your own node, without addressing this it is not possible to use
rosetta-ethereum
with a remote node that is not in your control.Solution
boolean
parameterskipGethAdmin
true
, skip callingadmin.*
APIs and return early with empty values.Open questions
TestStatus_NotSyncing
uses a mock to fetchtestdata/peers.json
but the current PR skips the call so never reaches that code - what is the best way to address this?Client
the correct place?skip
happen higher up in the call chain?