SkyDB integration - GetEntry, SetEntry, GetJSON and SetJSON by ilkamo · Pull Request #29 · NebulousLabs/go-skynet

ilkamo · 2020-12-17T21:51:55Z

Implemented GetEntry, SetEntry, GetJSON, SetJSON.
Added different test cases.

More tests and SetEntryOptions, GetEntryOptions, etc... with timeout will be added in a separate PR.

mrcnski · 2020-12-18T14:30:30Z

Awesome, thank you for this! I'll take a look.

mrcnski · 2021-01-13T14:05:31Z

crypto.go

+}
+
+// hashRegistryEntry hashes the given registry entry.
+func hashRegistryEntry(s RegistryEntry) []byte {


Can you please add a test for this function? We have one in skynet-js you can copy.

I would use the encoding package encoding.Marshl That would eliminate the need for the majority of this file I believe.

mrcnski · 2021-01-13T14:11:41Z

registry.go

+)
+
+// GetEntry gets the registry entry corresponding to the publicKey and dataKey.
+func (sc *SkynetClient) GetEntry(


API methods should take options, see e.g. (sc *SkynetClient) Download.

mrcnski · 2021-01-13T14:15:54Z

registry.go

+func (sc *SkynetClient) GetEntry(
+	publicKey string,
+	dataKey string,
+) (r RegistryEntryResponse, err error) {


Why not return the same object as in skynet-js? RegistryEntryResponse is slightly different from the SignedRegistryEntry returned in skynet-js.

mrcnski · 2021-01-13T15:07:06Z

registry.go

+	}
+
+	options := sc.Options
+	options.customContentType = "application/json"


Why this line?

mrcnski · 2021-01-13T15:07:47Z

tests/skydb_integration_test.go

@@ -0,0 +1,278 @@
+package tests


Can you please also add tests against the registry? (e.g. using the registry methods directly.) We have some in skynet-js you can look at.

Also I think we should have at least one end-to-end test that sets a random registry entry on siasky.net, queries the entry, and makes sure we get the same data back.

mrcnski · 2021-01-13T15:10:40Z

skydb.go

+) (io.ReadCloser, error) {
+	entry, err := sc.GetEntry(publicKey, dataKey)
+	if err != nil {
+		return nil, errors.AddContext(err, "could not get entry")


This shouldn't return an error if the entry wasn't found. In skynet-js we return null, I guess this should return nil.

mrcnski · 2021-01-13T15:11:03Z

skydb.go

+func (sc *SkynetClient) GetJSON(
+	publicKey string,
+	dataKey string,
+) (io.ReadCloser, error) {


This should return JSON instead of a ReadCloser.

mrcnski · 2021-01-13T15:12:25Z

registry.go

+		},
+	)
+	if err != nil {
+		return r, errors.AddContext(err, "could not execute request")


Should return null instead of an error on 404 (entry not found) - see skynet-js.

mrcnski · 2021-01-13T15:14:13Z

skydb.go

+		}
+	}
+
+	tempFile, err := createTempFileFromJson(dataKey, json)


I don't think it should be necessary to create an actual physical file in order to upload data. We can just upload the streamed data directly. I believe there is a generic upload function in upload.go that does this, but if you need help don't hesitate to ask.

mrcnski · 2021-01-13T15:15:35Z

skydb.go

+func (sc *SkynetClient) SetJSON(
+	privateKey string,
+	dataKey string,
+	json io.Reader,


This should take a json object instead of a Reader.

mrcnski · 2021-01-13T15:19:47Z

tests/skydb_integration_test.go

+	}
+
+	opts := skynet.DefaultDownloadOptions
+	urlpath := strings.TrimRight(opts.EndpointPath, "/") + "/" + skylink


nit: I believe we have a EnsureSuffix helper in Sia that may be useful here.

modules.EnsureSuffix 👍

mrcnski

Looks good overall, some parts need to be modified to be closer to the Javascript API, and to be more usable. I also think we need to verify this code with an end-to-end test.

ilkamo · 2021-01-18T18:38:38Z

@m-cat thanks for the review. I will work on the proposed changes as soon as I have some free time.

MSevey

Sorry about the delay on this, I missed the notification somehow.

MSevey · 2021-02-12T16:10:44Z

crypto.go

+}
+
+// hashDataKey hashes the given data key.
+func hashDataKey(dataKey string) []byte {


We have a crypto.HashObject method you can just use as oppose to implementing this function. Just need to import gitlab.com/NebulousLabs/Sia/crypto

MSevey · 2021-02-12T16:13:37Z

crypto.go

+)
+
+// encodeNumber converts the given number into a byte array.
+func encodeNumber(number uint64) []byte {


We have an encoding package that you can use instead of creating new methods.

gitlab.com/NebulousLabs/encoding

You can use encoding.Marshal

MSevey · 2021-02-12T16:15:27Z

crypto.go

+}
+
+// hashRegistryEntry hashes the given registry entry.
+func hashRegistryEntry(s RegistryEntry) []byte {


I would use the encoding package encoding.Marshl That would eliminate the need for the majority of this file I believe.

MSevey · 2021-02-12T16:15:29Z

crypto.go

+}
+
+// hashAll takes all given arguments and hashes them.
+func hashAll(args ...[]byte) []byte {


crypto.HashAll 😄

MSevey · 2021-02-12T16:15:36Z

crypto.go

+}
+
+// encodeString converts the given string into a byte array.
+func encodeString(toEncode string) []byte {


This function is encoding the length of the string, not the string itself. Is that intentional?

MSevey · 2021-02-12T16:24:29Z

registry.go

+
+	publicKeyBytes, err := hex.DecodeString(publicKey)
+	if err != nil {
+		return false, errors.New("could not decode publicKey")


same, ignoring original error.

MSevey · 2021-02-12T16:25:11Z

registry.go

+) ([]byte, error) {
+	privateKeyBytes, err := hex.DecodeString(privateKey)
+	if err != nil {
+		return nil, errors.New("could not decode privateKey")


same, ignoring original error

MSevey · 2021-02-12T16:25:58Z

skydb.go

+
+	skylink, err := hex.DecodeString(entry.Data)
+	if err != nil {
+		return nil, errors.New("could not decode data")


same, ignoring original error

MSevey · 2021-02-12T16:28:58Z

skydb.go

+	privateKey string,
+	dataKey string,
+	json io.Reader,
+	revision *uint64,


Is there a reason we are using a pointer here?

MSevey · 2021-02-12T16:31:25Z

tests/skydb_integration_test.go

+	}
+
+	opts := skynet.DefaultDownloadOptions
+	urlpath := strings.TrimRight(opts.EndpointPath, "/") + "/" + skylink


modules.EnsureSuffix 👍

ilkamo added 17 commits December 14, 2020 00:02

wip - added getEntry and getJson

f40a6b6

removed unused error

b0c4ea8

wip

6e0920b

working setEntry, getEntry

dc04295

working SetJson and GetJson

0f8fcf4

code refactoring

cd61dd7

using TeeReader

1317764

fix lint, comments and tests

41f6838

more comments

81355ac

publicKeyFromPrivateKey test and fix

73c6382

added tests

69221a4

added math.MaxUint64

794bc4a

renaming

7beaa6a

added tests

c47181d

fix linter

9106d5a

removed ugly support func

a392b9a

using consts

b10cd58

mrcnski requested review from mrcnski, peterjan and ro-tex December 18, 2020 14:29

mrcnski reviewed Jan 13, 2021

View reviewed changes

registry.go

}

options := sc.Options

options.customContentType = "application/json"

Copy link

Contributor

mrcnski Jan 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this line?

mrcnski reviewed Jan 13, 2021

View reviewed changes

mrcnski suggested changes Jan 13, 2021

View reviewed changes

mrcnski requested a review from MSevey January 13, 2021 15:25

MSevey suggested changes Feb 12, 2021

View reviewed changes

MSevey mentioned this pull request Mar 22, 2021

SkyDB integration - GetEntry, SetEntry, GetJSON and SetJSON SkynetLabs/go-skynet#3

Open

Conversation

ilkamo commented Dec 17, 2020

Uh oh!

mrcnski commented Dec 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrcnski Jan 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrcnski left a comment

Choose a reason for hiding this comment

Uh oh!

ilkamo commented Jan 18, 2021

Uh oh!

MSevey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mrcnski Jan 13, 2021 •

edited

Loading