Skip to content

Conversation

@akramhussein
Copy link
Contributor

Motivation

When connecting to remote/hosted nodes, they often require some form of authentication. This is either in the URL as a query or url parameter.

However, in the case of BlockDaemon, it’s an HTTP header X-Auth-Token.

Solution

I found the only way to address this was to add another environment variable GETH_HEADERS that allows you to pass in a comma separated list of key:values.

Open questions

None

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 suggestion to rename a field.

Would appreciate a test in configuration_test.go to make sure env vars are correctly parsed and in client_test.go to ensure that headers are set appropriately :) 🙏

@akramhussein
Copy link
Contributor Author

I've added some checks to the configuration_test.go -> let me know if that's ok, happy to add more or change.

@akramhussein akramhussein requested a review from septerr June 22, 2021 01:09
@septerr
Copy link
Contributor

septerr commented Jun 22, 2021

@akramhussein configuration tests look good! can we add a test for the client as well i.e. when the configuration contains geth headers, they get set in the geth calls? 🙏

Also, sometimes the builds get stuck like they have on your last commit. Pushing a new commit or an empty commit usually resolves the issue. :)

@akramhussein
Copy link
Contributor Author

Sure - where should this test go, in client_test.go?

@akramhussein
Copy link
Contributor Author

I might need some guidance here as go isn't a language I have much experience with.

In client_test.go I've added a test like below, but I'm not actually checking anything right now.

The other tests use a mock JSONRPC and client GraphQL to run checks on, however GethHeaders is only set via NewClient.

func TestHeadersSet(t *testing.T) {
	cfg := &configuration.Configuration{
		Params:                 params.MainnetChainConfig,
		GethURL:                "http://blah",
		SkipGethAdmin:          true,
		GethHeaders: []*HTTPHeader{
			{Key: "X-Auth-Token", Value: "12345-ABCDE"},
			{Key: "X-Api-Version", Value: "2"},
		},
	}

	c, err := NewClient(cfg.GethURL, cfg.Params, cfg.SkipGethAdmin, cfg.GethHeaders)

	ctx := context.Background()
	
	rawTx, err := ioutil.ReadFile("testdata/submitted_tx.json")
	assert.NoError(t, err)

	tx := new(types.Transaction)
	assert.NoError(t, tx.UnmarshalJSON(rawTx))

	assert.NoError(t, c.SendTransaction(
		ctx,
		tx,
	))
}

Further the interface for the Client that is constructed (rpc.DialHTTPWithClient) and the that defined in the parent Client struct are different. Finally, fetching the headers does not seem supported on rpc.DialHTTPWithClient.

@akramhussein
Copy link
Contributor Author

Hi @septerr - do you have any thoughts on above?

Also, I've tried to re-trigger a build with empty commits but doesn't seem to work :(

@septerr
Copy link
Contributor

septerr commented Jul 2, 2021

Hi @akramhussein ,

Let's shelve the client test for now. I am checking with some folks about why the build is not working. Sorry for the delay in response.

@septerr
Copy link
Contributor

septerr commented Jul 2, 2021

Hi @akramhussein, if it is not too much trouble, can you try creating a new PR with these changes to get past the build issue? 🙏 🙏 🙏

@akramhussein
Copy link
Contributor Author

Hi @septerr - tried it here #49 (not sure it has helped though). They are the same commits. I can try and generate new commit shas. However, the PR #47 is also having same issue so don't think it's that.

@septerr
Copy link
Contributor

septerr commented Jul 2, 2021

Ahhh! Thanks for the info! I'll keep trying to work with folks to figure out what's going on.

@shrimalmadhur shrimalmadhur merged commit 68306c7 into coinbase:master Jul 22, 2021
@akramhussein akramhussein deleted the geth-headers branch July 22, 2021 19:19
akramhussein added a commit to akramhussein/rosetta-ethereum that referenced this pull request Nov 8, 2021
shrimalmadhur pushed a commit that referenced this pull request Nov 8, 2021
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.

3 participants