Skip to content

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!

@akramhussein akramhussein requested a review from septerr June 18, 2021 15:32
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. 🙏 🙏 🙏

@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