swarm: repeated --ens-api CLI flag (#14386) #142

janos · 2017-12-01T15:19:51Z

This PR implement a CLI flag that can be repeated to allow multiple ENS
resolvers for different TLDs.

New flag --ens-endpoint can be repeated and accepts value with
format [tld:][contract-addr@]url.

MultiResolver is implemented to support resolutions based on their TLDs.
Each TLD can have multiple resolvers, and the resolution from the
first one in the sequence will be returned. If the TLD does not have its own resolver, a default resolver will be used.

ENSClientConfig is implemented to provide configuration parameters for ens resolvers on Swarm initialization.

Function detectEnsAddr has been moved from cmd/swarm/main.go to swarm/swarm.go.

Implement a CLI flag that can be repeated to allow multiple ENS resolvers for different TLDs.

lmars · 2017-12-02T00:51:12Z

Should this replace the --ens-api flag?

janos · 2017-12-02T12:13:37Z

Hi Lewis, since the flag --ens-endpoint can be repeated and can contain only the endpoint url/path, flags it can be simplified, that there is only one flag --ens-api which would function the same way as --ens-endpoint does now, just with the same default value, as the --ens-api does have now. One small problem is with --ens-addr, which can not be used with repeated --ens-api flag. I can "merge" --ens-endpoint functionality into --ens-api (default value) flag, and restrict --ens-addr to be allowed only if --ens-api is specified once.

Would such change make sense?

lmars · 2017-12-03T19:17:36Z

@janos so personally, I think the command line interface should be for "simple" use cases (e.g. I just want to point Swarm at mainnet / ropsten / rinkeby etc. to resolve all addresses), and we should promote using a config file for more complex use cases (@holisticode is currently working on improving the config file).

A config file can be self documenting with comments and support structures like maps and arrays in a natural way, whereas command line flags are quite cumbersome and error prone.

If we do expand the CLI args, I'd at least like to see only one way to configure ENS (either multiple --ens-endpoint flags or a modified --ens-api flag).

Allow multiple --ens-api flags to be specified with value format [tld:][contract-addr@]url. Backward compatibility with only one --ens-api flag and --ens-addr flag is preserved and conflict cases are handled: - multiple --ens-api with --ens-addr returns an error - single --ens-api with contract address and --ens-addr with different contract address returns an error Previously implemented --ens-endpoint is removed. Its functionality is replaced with multiple --ens-api flags.

janos · 2017-12-04T11:55:23Z

@lmars the latest change addresses the last part of you comment. Flag --ens-endpoint is removed and --ens-api can be repeated.

lmars · 2017-12-04T20:39:49Z

cmd/swarm/main.go

+		case 1:
+			// Check if only one --ens-api is specified in order to use --ens-addr value
+			// to preserve the backward compatibility with single --ens-api flag.
+			// Multiple


This comment is incomplete.

lmars · 2017-12-04T20:40:51Z

cmd/swarm/main.go

 		Name:  "ens-api",
-		Usage: "URL of the Ethereum API provider to use for ENS record lookups",
-		Value: node.DefaultIPCEndpoint("geth"),
+		Usage: "ENS API endpoint for a TLD and with contract address, can be repeated, format [tld:][contract-addr@]url",


I guess we should deprecate --ens-addr then?

lmars · 2017-12-04T20:44:40Z

cmd/swarm/main.go

+			// Check if only one --ens-api is specified in order to use --ens-addr value
+			// to preserve the backward compatibility with single --ens-api flag.
+			// Multiple
+			c := parseFlagEnsAPI(ensAPIs[0])


We currently treat --ens-api '' as an indication to disable the use of ENS so we should check for that case here (but to be honest, we should have an explicit flag like --no-ens instead).

lmars · 2017-12-04T20:46:29Z

cmd/swarm/main.go

 	}
 	if err := stack.Register(boot); err != nil {
 		utils.Fatalf("Failed to register the Swarm service: %v", err)
 	}
 }

+func parseFlagEnsAPI(s string) swarm.ENSClientConfig {


Add a comment to describe what this function is doing.

lmars · 2017-12-04T20:49:18Z

swarm/api/api.go

+func MultiResolverOptionWithResolver(r Resolver, tld string) MultiResolverOption {
+	return func(m *MultiResolver) {
+		if _, ok := m.resolvers[tld]; !ok {
+			m.resolvers[tld] = []Resolver{}


There is no need for this assignment.

lmars · 2017-12-04T20:55:41Z

swarm/api/api.go

+// NewMultiResolver creates a new instance of MultiResolver.
+func NewMultiResolver(opts ...MultiResolverOption) (m *MultiResolver) {
+	m = &MultiResolver{
+		resolvers: map[string][]Resolver{},


Use make(map[string][]Resolver) to initialise the map (that is the convention in the go-ethereum codebase).

lmars · 2017-12-04T20:56:39Z

swarm/api/api.go

+// the Hash from the the first one which does not return error
+// will be returned.
+func (m MultiResolver) Resolve(addr string) (h common.Hash, err error) {
+	rs, _ := m.resolvers[""]


No need to assign _ here.

lmars · 2017-12-04T20:57:50Z

swarm/api/api.go

+func (m MultiResolver) Resolve(addr string) (h common.Hash, err error) {
+	rs, _ := m.resolvers[""]
+	if i := strings.LastIndex(addr, "."); i >= 0 {
+		rstld, ok := m.resolvers[addr[i+1:]]


This would be more readable using path.Ext.

…n code

…solve

janos · 2017-12-11T16:46:19Z

@holisticode could you review this pull request as it is related in some parts to this PR ethereum/go-ethereum#15548?

Merge with changes that implement config file PR #15548. Field *EnsApi string* in swarm/api.Config is replaced with *EnsAPIs []string*. A new field *EnsDisabled bool* is added to swarm/api.Config for easy way to disable ENS resolving with config file. Signature of function swarm.NewSwarm is changed and simplified.

janos · 2017-12-13T09:52:50Z

@holisticode this PR is updated with the changes that you did regarding config file feature. Could you review the changes?

There is one new filed EnsDisabled in swarm/api.Config. If that is ok, would it make sense to have cli flag --ens-disabled (as @lmars already suggested) and env ver SWARM_ENS_DISABLED?

One backward incompatible change is replacing Config field EnsApi string with EnsAPIs []string. Config file feature has been added recently, but if backward compatibility is needed, EnsApi field can be added and handled.

Fix a conflict in cmd/swarm envVarsOverride function.

gbalint · 2017-12-18T15:01:10Z

For the note: team decision is that we disable ENS by default

Specifying ENS API CLI flag, env variable or configuration field is required for ENS resolving. Backward compatibility is preserved with --ens-api="" CLI flag value.

holisticode · 2017-12-18T16:31:26Z

swarm/swarm_test.go

+			}
+		})
+	}
+}


Can we add a test for invalid format via command line or env var params? E.g. --ens-api ":<addr>" or --ens-api "test-tld:" ? Can you think of other invalid ones? Are we also testing with multiple addresses per TLD? With no default resolver?

👍 Will add test cases for invalid formats.
Test cases for multiple resolvers are handled in swarm/api.TestMultiResolver with various combinations.

great thanks.

Validations for missing tld, contract address or url is added to cmd/swarm.validateConfg.

holisticode · 2017-12-18T16:33:56Z

cmd/swarm/main.go

 		Name:   "ens-api",
-		Usage:  "URL of the Ethereum API provider to use for ENS record lookups",
+		Usage:  "ENS API endpoint for a TLD and with contract address, can be repeated, format [tld:][contract-addr@]url",


Have you tried to see if it works when the option is provided via config file?

What is the behavior if only addresses with TLDs are provided, do we end up without a default one? Is it even possible to identify the default one if no default has been provided?

The toml configuration file is loading the values this way:

EnsAPIs = [
"endpoint1",
"test:endpoint2",
]

The ENS API endpoint configuration is implemented to be most explicit. If no default endpoint is provided, no assumption are made. Could there be a case that no default resolver is required?

I am concerned about the other way around; let's say user provides .eth:[email protected], what happens to .test and other TLDs?

My view is that in that case, other domains except under .eth TLD should not be resolved, as that is configured by the user. In this particular case, one may assume that provided endpoint which is ti only one can be used for other TLDs. But what is the case if endpoints for two or more TLDs are provided, but not a default endpoint? One option is not to have the default resolver, and the order one is to add all resolvers as default resolvers. Maybe the later behaviour is a better solution?

Yes, that is right. I would also like to hear more opinions.

I think if the user doesn't provide a default (i.e. a value without the tld: part), then names which don't match an endpoint should not be resolved.

For example, if I explicitly start swarm with:

--ens-api .eth:<addr>@geth.ipc --ens-api .test:<addr>@testnet/geth.ipc

I would not expect say .foo requests to be resolved, rather an error be returned like no ENS endpoint configured to resolve .foo names.

@lmars the functionality is now similar, just the error returned from MultiResolver.Resolve should include the information about the TLD, currently it is just an error (var errNoResolver) with "no resolver" message.

ok, so @janos when you have changed the error message please ping, then I can mark this as approved from my side. Thanks.

There is a new error type NoResolverError for more verbose error message.

holisticode · 2017-12-18T16:39:31Z

swarm/api/api.go

+type MultiResolver struct {
+	resolvers map[string][]Resolver
+}
+


why do we need multiple resolvers per TLD if only the first resolution is returned?

As per requirement in this comment ethereum/go-ethereum#14386 (comment) for sequence of resolvers.

holisticode · 2017-12-18T16:41:19Z

swarm/swarm.go

@@ -76,7 +81,7 @@ func (self *Swarm) API() *SwarmAPI {

 // creates a new swarm service instance
 // implements node.Service
-func NewSwarm(ctx *node.ServiceContext, backend chequebook.Backend, ensClient *ethclient.Client, config *api.Config, swapEnabled, syncEnabled bool, cors string) (self *Swarm, err error) {
+func NewSwarm(ctx *node.ServiceContext, backend chequebook.Backend, config *api.Config) (self *Swarm, err error) {


Good simplifying the func interface here, thanks

holisticode · 2017-12-18T16:46:20Z

swarm/swarm.go

+		opts := []api.MultiResolverOption{}
+		for _, c := range config.EnsAPIs {
+			tld, endpoint, addr := parseEnsAPIAddress(c)
+			r, err := newEnsClient(endpoint, addr, config)


I wonder if we should test for invalid formats before we actually try to connect to ENS? What do you think?

Function parseEnsAPIAddress does not return error as the format is very simple, permissive in the endpoint part and assumes any value for tld and contract address. I would put format validation much sooner during execution, just after the config load in cmd/swarm.buildConfig. Would that be ok?

Yeah I think that would be a good idea.

MultiResolver needs to provide information about TLD that has no resolver configured for.

janos · 2017-12-19T14:42:24Z

Thanks @holisticode.

@lmars are you still approving the PR, even with the latest changes?

lmars · 2017-12-19T21:17:40Z

cmd/swarm/config.go

@@ -101,6 +102,8 @@ func buildConfig(ctx *cli.Context) (config *bzzapi.Config, err error) {
 	config = envVarsOverride(config)
 	//override settings provided by command line
 	config = cmdLineOverride(config, ctx)
+	//validate configuration parameters
+	err = validateConfig(config)


This shadows the previous assignment of err which is now not being checked (we should probably avoid named return arguments unless they are really needed)

Fixed by returning right after the previous error assignment if it is not nil.

lmars · 2017-12-19T21:18:39Z

cmd/swarm/config.go

 	}

-	if ensaddr := ctx.GlobalString(EnsAddrFlag.Name); ensaddr != "" {
+	if ensaddr := ctx.GlobalString(DeprecatedEnsAddrFlag.Name); ensaddr != "" {
 		currentConfig.EnsRoot = common.HexToAddress(ensaddr)


We should print a deprecation notice here.

The depreciation message is handled in checkDeprecated function on line 319.

lmars · 2017-12-19T22:45:08Z

cmd/swarm/config.go

@@ -97,10 +98,15 @@ func buildConfig(ctx *cli.Context) (config *bzzapi.Config, err error) {
 	config = bzzapi.NewDefaultConfig()
 	//first load settings from config file (if provided)
 	config, err = configFileOverride(config, ctx)
+	if err != nil {
+		return err


Shouldn't this be return nil, err?

Oh right, fixed. :( Thank you!

zelig · 2017-12-25T06:39:46Z

this looks good to me. ready for PR on main repo?

janos · 2017-12-25T09:15:21Z

@zelig PR created ethereum/go-ethereum#15748.

gbalint · 2018-02-23T07:30:46Z

merged to ethereum/master

swarm: add CLI --ens-endpoint flag (#14386)

057af8c

Implement a CLI flag that can be repeated to allow multiple ENS resolvers for different TLDs.

lmars reviewed Dec 4, 2017

View reviewed changes

janos added 7 commits December 4, 2017 22:20

swarm: add comment for parseFlagEnsAPI and fix a mistake in comment i…

b33a051

…n code

swarm: check if "--ens-api ''" is specified in order to disable ENS

15ad6f2

swarm/api: remove unneeded assignment in MultiResolverOptionWithResolver

34edbc8

swarm/api: initialize map with make to comply with the convention

a758b5c

swarm/api: remove unneeded blank assignement

3732c15

swarm: deprecate --ens-addr CLI flag with a warning message

e451b65

swarm/api: use path.Ext instead strings.LastIndex in MultiResolver.Re…

1dc19de

…solve

janos changed the title ~~swarm: add CLI --ens-endpoint flag (#14386)~~ swarm: repeated --ens-api CLI flag (#14386) Dec 4, 2017

lmars approved these changes Dec 6, 2017

View reviewed changes

nonsense force-pushed the master branch from 58101a0 to e85b68e Compare December 6, 2017 15:25

janos requested a review from holisticode December 11, 2017 16:44

gbalint assigned janos Dec 12, 2017

nonsense force-pushed the master branch from 32516c7 to 732f546 Compare December 13, 2017 10:50

cmd/swarm: Merge branch 'master' into multiple-ens-endpoints

47a8014

Fix a conflict in cmd/swarm envVarsOverride function.

gbalint added the discussion label Dec 18, 2017

cmd/swarm, swarm: disable ENS API by default

c0a4d9e

Specifying ENS API CLI flag, env variable or configuration field is required for ENS resolving. Backward compatibility is preserved with --ens-api="" CLI flag value.

holisticode reviewed Dec 18, 2017

View reviewed changes

janos added 2 commits December 18, 2017 23:07

swarm/api: implement NoResolverError with information about TLD

0d6a735

MultiResolver needs to provide information about TLD that has no resolver configured for.

cmd/swarm: add validation for EnsAPIs configuration parameter

dd5ae4f

holisticode approved these changes Dec 19, 2017

View reviewed changes

lmars reviewed Dec 19, 2017

View reviewed changes

cmd/swarm: return error early in buildConfig function

820cf09

janos mentioned this pull request Dec 25, 2017

swarm: repeated --ens-api CLI flag (#14386) ethereum/go-ethereum#15748

Merged

janos added the submitted to ethereum/go-ethereum label Dec 25, 2017

gbalint added master http/rpc/ipc labels Jan 25, 2018

janos added 2 commits February 22, 2018 14:23

swarm, cmd/swarm: Merge branch 'master' into multiple-ens-endpoints

a3a0735

swarm, cmd/swarm: Merge branch 'master' into multiple-ens-endpoints

6a9730e

gbalint closed this Feb 23, 2018

swarm: repeated --ens-api CLI flag (#14386) #142

swarm: repeated --ens-api CLI flag (#14386) #142

Conversation

janos commented Dec 1, 2017

lmars commented Dec 2, 2017

janos commented Dec 2, 2017

lmars commented Dec 3, 2017

janos commented Dec 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janos commented Dec 11, 2017

janos commented Dec 13, 2017

gbalint commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holisticode Dec 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janos commented Dec 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zelig commented Dec 25, 2017

janos commented Dec 25, 2017

gbalint commented Feb 23, 2018

holisticode Dec 18, 2017 •

edited

Loading