Skip to content
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

Merged
merged 10 commits into from
Jun 18, 2021
Merged

Add Configuration parameter 'SkipGethAdmin' to support skipping 'admin_peers' checks #38

merged 10 commits into from
Jun 18, 2021

Conversation

akramhussein
Copy link
Contributor

@akramhussein akramhussein commented Jun 10, 2021

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

  • Add a Configuration boolean parameter skipGethAdmin
  • If set to true, skip calling admin.* APIs and return early with empty values.

Open questions

  • 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?
  • 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?
  • Should I add a line in the README?

@akramhussein akramhussein changed the title Add Configuration parameter 'DisableGethAdmin' to support skipping 'admin_peers' checks Add Configuration parameter 'SkipGethAdmin' to support skipping 'admin_peers' checks Jun 10, 2021
@akramhussein akramhussein marked this pull request as draft June 10, 2021 22:04
@akramhussein akramhussein marked this pull request as ready for review June 10, 2021 22:10
Copy link
Contributor

@septerr septerr left a 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/configuration.go Outdated Show resolved Hide resolved
configuration/configuration_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@septerr septerr left a 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. 🙏 🙏 🙏

configuration/configuration.go Outdated Show resolved Hide resolved
@septerr septerr merged commit 1eab851 into coinbase:master Jun 18, 2021
@akramhussein akramhussein deleted the config-skip-geth-admin branch June 21, 2021 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants