swarm, api, main: RNS resolution #1939

mortelli · 2019-11-08T19:37:32Z

This PR adds the functionality to resolve .rsk addresses as content hashes for Swarm.

How it works

It works in a similar manner to ENS resolution—in fact, it uses a similar service called RIF Name Service (or RNS for short).

this implementation does not uses geth, but rather a go library which is included in the vendor folder.
- the library in question is github.com/rnsdomains/rns-go-lib.
a rns-api flag is added, with a format of [contract-addr@]url in order to activate RNS resolution.
- contract-addr is the address of the RNS resolver contract to use.
- url is the network endpoint.
~~the test includes the marcelosdomain.rsk URI while we secure test.rsk (tagging @vojtechsimetka for this one)~~

Vendoring

There seems to be quite a few changes to vendor. While all the tests pass, @santicomp2014 and I are a bit suspicious of the changes to go.mod.

For the record, our steps were:

start from the same go.mod, go.sum and vendor files and folders as found in master.
go get github.com/rnsdomains/rns-go-lib
make vendor

In particular, we aren't sure why some lines from go.mod are being deleted.

tagging @janos for this one to see if it's possible to verify these steps were correct.

Merge upstream

…olve function

…_rns

janos

Thanks for addressing my comments.

zelig · 2019-11-27T09:22:27Z

I am ready to approve this if it is updated.

janos

LGTM. Maybe @skylenet can check also vendoring dependencies changes?

Eknir

Good work, I left some comments.

Eknir · 2019-11-27T17:55:06Z

api/api.go

@@ -185,15 +191,17 @@ type API struct {
 	feed      *feed.Handler
 	fileStore *storage.FileStore
 	dns       Resolver
+	rns       Resolver


I think it would be great to leave a comment there, explaining what rns is/does. Even better if you can also create those comments for all other fields, but that is not necessary for this PR.

Added a note on the resolvers.
The rest could be added in another PR.

Eknir · 2019-11-27T17:56:20Z

api/api.go

@@ -225,6 +233,20 @@ func (a *API) Store(ctx context.Context, data io.Reader, size int64, toEncrypt b
 // Resolve a name into a content-addressed hash
 // where address could be an ENS name, or a content addressed hash


Update comment

Eknir · 2019-11-27T17:58:30Z

api/api.go

@@ -244,6 +266,14 @@ func (a *API) Resolve(ctx context.Context, address string) (storage.Address, err
 	return resolved[:], nil
 }

+func tld(address string) (tld string) {
+	splitAddress := strings.Split(address, ".")


What if an address has multiple dots?

This only retrieves the last portion of the address.
For example mydomain.subdomain.rsk
will return rsk as the TLD which is the expected functionality in order to resolve the name correctly.

Eknir · 2019-11-27T17:59:08Z

api/api_test.go

+func newRSKTestResolveValidator(addr string) *testResolveValidator {
+	r := &testResolveValidator{}
+	if addr == "swarm.rsk" {
+		hash := common.HexToHash("88ced8ba8e9396672840b47e332b33d6679d9962d80cf340d3cf615db23d4e07")


Where does this hash come from?

This content hash is a hash of a test image, we used it in a test domain, but the actual resolver is mocked right now, this avoids coupling between the rns resolution in Swarm and the actual resolver.
The resolver has its own test in the library that tests the concrete implementation.

Eknir · 2019-11-27T18:02:19Z

api/api_test.go

+// TestRNSResolve tests resolving content from RNS addresses
+func TestRNSResolve(t *testing.T) {
+	rnsAddr := "swarm.rsk"
+	resolvedContent := "88ced8ba8e9396672840b47e332b33d6679d9962d80cf340d3cf615db23d4e07"


Idem. Where does this hash come from?

Idem as above

Eknir · 2019-11-27T18:03:51Z

api/api.go

+
+// Resolve returns a resolver function
+func (f ResolverFunc) Resolve(domain string) (common.Hash, error) { return f(domain) }
+
 // Resolver interface resolve a domain name to a hash using ENS


Update comment

Eknir · 2019-11-27T18:08:36Z

swarm.go

@@ -75,6 +77,7 @@ type Swarm struct {
 	config            *api.Config        // swarm configuration
 	api               *api.API           // high level api layer (fs/manifest)
 	dns               api.Resolver       // DNS registrar


What is the difference between ens and dns?

dns is the wrapper, for resolving multiple tld resolvers, it applies for ens but could be used for other resolvers.
on the contrary rns only resolver rsk domains with one tld.
this could be addressed in this issue #1991

Eknir · 2019-11-27T18:11:52Z

swarm.go

+// parseRnsAPIAddress parses string according to format
+// [contract-addr@]url and returns RNSClientConfig structure
+// with endpoint and contract address
+func parseRnsAPIAddress(s string) (endpoint string, addr common.Address) {


As far as I can see, this function is exactly the same as parseEnsAPIAddress. I think it would be better to refactor those to avoid code multiplication

I agree there is code duplication.
Fixed refactored for parseResolverAPIAddress

skylenet · 2019-11-28T08:10:50Z

LGTM. Maybe @skylenet can check also vendoring dependencies changes?

I just tried to do a make vendor on the current master and it also seems to be deleting some vendored files. Seems like we got rid of some dependencies on previous PRs and forgot to run the vendor command there.

janos

I would like to block this PR to be merged to master before we address some issues that we discovered in 0.5.3 by making a bugfix release. I will unblock as soon as we are done with the release, hopefully tomorrow. Thanks for understanding.

santicomp2014 and others added 23 commits September 27, 2019 11:36

api: WIP api rns resolver

6050689

rns test

a2f4fec

swarm module added rns

b5fb1dc

Merge branch 'master' into swarm_rns

a7fcb59

api: fixed test rns

71ca0c4

Merge pull request #1 from ethersphere/master

914d2ff

Merge upstream

Merge branch 'master' into swarm_rns

17ca207

added rds-swarm mod

99d1211

api: changed resolution to ResolveDomainContent

2bee967

Merge remote-tracking branch 'upstream/master'

07a4f58

Merge branch 'master' into swarm_rns

5d68b10

api: refactor TestRNSResolver function

1ab9295

api: refactor TestRNSResolver function

26239b3

api: replace path.Ext call with publicsuffix.PublicSuffix call in Res…

402c6ee

…olve function

api: remove ToLower call for TLD in Resolve function

2fc2b51

api: refactor TestRNSResolve function

b8a1461

api,cmd: added rns flag

dd9bc01

api: iterate TestRNSResolve function

2e5b687

main: handle rnsAPI flag as single string instead of array

6a5b113

api: add expected error to TestRNSResolve function

10f63f7

vendor: re applied

bcc8900

Merge branch 'swarm_rns' of github.com:santicomp2014/swarm into swarm…

faf5652

…_rns

api: update rns library, add ErrNoContent to resolver test

b437f78

mortelli added the in progress label Nov 8, 2019

santicomp2014 added 6 commits November 11, 2019 13:50

vendor: rollback

0fb8d6b

vendor: fix vendor

badce67

vendor: fix vendor

de3ecbb

vendor: make vendor

8d5e639

vendor,swarm,main: added parseRnsAPIAddress and updated vendor

dffe427

Merge branch 'master' into rns-resolution

47cdaf5

mortelli added 3 commits November 19, 2019 16:08

api: minor changes to RNS resolution tests

879c7a1

api: add tld function

80f1778

vendor: make vendor results

8d589ff

santicomp2014 requested review from janos and acud November 19, 2019 20:42

mortelli added 2 commits November 20, 2019 09:35

Merge remote-tracking branch 'origin/master' into rns-resolution

73211e2

Merge remote-tracking branch 'origin/master' into rns-resolution

9582393

janos approved these changes Nov 21, 2019

View reviewed changes

mortelli added 3 commits November 25, 2019 09:14

Merge remote-tracking branch 'origin/master' into rns-resolution

ef34aa2

Merge remote-tracking branch 'origin/master' into rns-resolution

18b67dd

Merge remote-tracking branch 'origin/master' into rns-resolution

095063f

janos approved these changes Nov 27, 2019

View reviewed changes

santicomp2014 mentioned this pull request Nov 27, 2019

Unify resolvers into one cli flag #1991

Open

Merge branch 'master' into rns-resolution

8beb6fb

Eknir reviewed Nov 27, 2019

View reviewed changes

zelig approved these changes Nov 28, 2019

View reviewed changes

skylenet self-requested a review November 28, 2019 08:11

skylenet approved these changes Nov 28, 2019

View reviewed changes

janos suggested changes Nov 28, 2019

View reviewed changes

api,swarm: corrected comments, also refactored parseResolverAPIAddress

a8b0897

acud added the do-not-merge label Nov 29, 2019

acud added this to the 0.5.5 milestone Nov 29, 2019

acud removed the do-not-merge label Nov 29, 2019

acud approved these changes Nov 29, 2019

View reviewed changes

janos approved these changes Dec 2, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into rns-resolution

eb645ff

mortelli merged commit fd3b095 into master Dec 2, 2019

mortelli deleted the rns-resolution branch December 2, 2019 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swarm, api, main: RNS resolution #1939

swarm, api, main: RNS resolution #1939

mortelli commented Nov 8, 2019 •

edited

Loading

janos left a comment

zelig commented Nov 27, 2019

janos left a comment

Eknir left a comment

Eknir Nov 27, 2019

santicomp2014 Nov 28, 2019

Eknir Nov 27, 2019

santicomp2014 Nov 28, 2019

Eknir Nov 27, 2019

santicomp2014 Nov 28, 2019

Eknir Nov 27, 2019

santicomp2014 Nov 28, 2019

Eknir Nov 27, 2019

santicomp2014 Nov 28, 2019

Eknir Nov 27, 2019

santicomp2014 Nov 28, 2019

Eknir Nov 27, 2019

santicomp2014 Nov 28, 2019

Eknir Nov 27, 2019

santicomp2014 Nov 28, 2019

skylenet commented Nov 28, 2019

janos left a comment

		@@ -225,6 +233,20 @@ func (a *API) Store(ctx context.Context, data io.Reader, size int64, toEncrypt b
		// Resolve a name into a content-addressed hash
		// where address could be an ENS name, or a content addressed hash

swarm, api, main: RNS resolution #1939

swarm, api, main: RNS resolution #1939

Conversation

mortelli commented Nov 8, 2019 • edited Loading

How it works

Vendoring

janos left a comment

Choose a reason for hiding this comment

zelig commented Nov 27, 2019

janos left a comment

Choose a reason for hiding this comment

Eknir left a 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

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

skylenet commented Nov 28, 2019

janos left a comment

Choose a reason for hiding this comment

mortelli commented Nov 8, 2019 •

edited

Loading