RFC: coreapi.Name,Key by magik6k · Pull Request #4477 · ipfs/kubo

magik6k · 2017-12-10T22:50:32Z

TODO:

Review
Docs
Tests

Note that most of the implementation code comes form respective commands, if this get's merged, I'll rewrite them to use this API.

whyrusleeping · 2017-12-14T21:26:34Z

@magik6k since these are going to be user facing APIs, lets go ahead and make this all godoc compliant

ghost · 2017-12-15T17:35:26Z

core/coreapi/coreapi.go

+		api,
+		nil,
+	}
+}


Is this multiline format what gofmt emitted?

ghost · 2017-12-16T17:47:39Z

core/coreapi/interface/interface.go

+type IpnsEntry struct {
+	Name  string
+	Value Path
+}


I wonder if we should make IpnsEntry an interface, in the spirit of future extensibility. Opinions on that?

On the topic of future extensibility, we should also consider adding TTL/expiry to the returned values.

ghost · 2017-12-16T17:48:31Z

core/coreapi/interface/interface.go

+	// WithKey is an option for Publish which specifies the key to use for
+	// publishing. Default value is "self" which is the node's own PeerID.
+	//
+	// You can use .Key API to list and generate more names and their respective keys.


You can use KeyAPI

ghost

This looks pretty good overall :) 🤖 Left a few comments and questions

ghost · 2017-12-16T17:54:24Z

core/coreapi/interface/interface.go

+
+	// WithNoCache is an option for Resolve which specifies when set to true
+	// disables the use of local name cache. Default value is false
+	WithNoCache(nocache bool) options.NameResolveOption


I have a hunch we shouldn't make this a negative, even if it's already a negative in the CLI/API -- opinions?

ghost · 2017-12-16T18:04:37Z

core/coreapi/interface/interface.go

+	// Supported algorithms:
+	// * rsa
+	// * ed25519
+	WithAlgorithm(algorithm string) options.KeyGenerateOption


Let's call this WithType(), following ipfs key generate --type=

We can have consts for the key types:

const ( RSAKey = "rsa" Ed25519Key = "ed25519" ) keyapi.Generate(ctx, "my-key", keyapi.WithType(coreopts.Ed25519Key))

ghost · 2017-12-16T18:09:21Z

core/coreapi/interface/interface.go

+	WithForce(force bool) options.KeyRenameOption
+
+	// List lists keys stored in keystore
+	List(ctx context.Context) (map[string]string, error) //TODO: better key type?


//TODO: better key type?

Let's make it a Key interface (for easier future extensibility)

type Key interface { Name() string Type() string // is the type exposed through the keystore? Path() Path // => /ipns/QmPeerID }

Also, any opinion on returning a map[string]Key or a []Key here?

ghost · 2017-12-16T18:11:54Z

core/coreapi/interface/interface.go

+type KeyAPI interface {
+	// Generate generates new key, stores it in the keystore under the specified
+	// name and returns a base58 encoded multihash of it's public key
+	Generate(ctx context.Context, name string, opts ...options.KeyGenerateOption) (string, error)


Should we return Paths instead of PeerID strings?

ghost · 2017-12-16T18:13:23Z

core/coreapi/interface/options/name.go

+
+func NamePublishOptions(opts ...NamePublishOption) (*NamePublishSettings, error) {
+	options := &NamePublishSettings{
+		ValidTime: 24 * time.Hour,


Should have DefaultValidTime = 24 * time.Hour for this

ghost · 2017-12-17T00:00:53Z

core/coreapi/key.go

+
+func (api *KeyAPI) Rename(ctx context.Context, oldName string, newName string, opts ...caopts.KeyRenameOption) (string, bool, error) {
+	options, err := caopts.KeyRenameOptions(opts...)
+	if newName == "self" {


This looks like it should be err != nil, typo?

ghost · 2017-12-17T00:11:30Z

core/coreapi/name.go

+
+	if n.Identity == "" {
+		return nil, errors.New("identity not loaded")
+	}


We don't use n.Identity, I think this check can be removed

ghost · 2017-12-17T00:19:03Z

core/coreapi/interface/options/name.go

+		settings.Key = key
+		return nil
+	}
+}


I noticed key can be both a key name and peerID (see keylookup()) -- cool! Let's make sure to document that :)

magik6k · 2017-12-22T15:47:09Z

Waiting for #4471 to be merged for test utils

vyzo

👍 looks really good to me

whyrusleeping · 2017-12-31T22:47:40Z

@magik6k merged the other PR, let me know when to look at this one again

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k · 2018-01-02T00:38:12Z

core/coreapi/name_test.go

+}
+
+func TestBasicPublishResolveTimeout(t *testing.T) {
+	t.Skip("ValidTime doesn't appear to work at this time resolution")


@whyrusleeping, I noticed that ValidTime doesn't seem to work at this time scale. Is this by design (should I drop this test), or is there a better way to do this (or is it a bug)?

hrm... thats tricky. It should be okay at 100ms... I'm not sure this isnt a bug. How does it fail?

magik6k · 2018-01-02T00:42:48Z

Added tests, fixed some bugs they caught, other than that one question and after test are reviewed this PR is RTM.

whyrusleeping · 2018-01-02T17:59:16Z

core/coreapi/interface/interface.go

+
+	// WithSize is an option for Generate which specifies the size of the key to
+	// generated. Default is 0
+	WithSize(size int) options.KeyGenerateOption


It feels a bit weird having these options be part of the interface... cc @lgierth

The thinking was that in the future some of these option funcs might depend on state of the KeyAPI object. But actually in these cases we can just pass it in as an argument, e.g. keyapi.Generate(ctx, "mykey", coreopts.WithType(coreopts.Ed25519Key))

Drawback: the likelyhood of name collisions increases: there's more "type" options than just the one in key/generate. We can prefix these funcs, e.g. coreopts.WithKeyType, or we can make a conversion similar to the api.KeyAPI() conversions: opts := api.Options(); keyopts := opts.KeyOptions()

Oh, so all the options are in the same package?

They are in one package, though are scoped per API, see https://github.com/ipfs/go-ipfs/pull/4477/files#diff-764a71d2f522b4136e7909befd573776R53 (or the tests).

For key API, type will always mean 'key type'. In the case where we create another function for which it would make sense to pass this option, it's possible to reuse that (with some more go types magic)

whyrusleeping · 2018-01-02T18:29:47Z

core/coreapi/name.go

+			return nil, err
+		}
+
+		if pid.Pretty() == k {


when could this ever not be the case?

We're iterating over all keys in the keystore and this is our return condition -- it's not a sanity assertion :)

Ah, i see now. Carry on

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k added the topic/core-api Topic core-api label Dec 10, 2017

magik6k requested a review from a user December 10, 2017 22:50

ghost assigned magik6k Dec 10, 2017

ghost added the status/in-progress In progress label Dec 10, 2017

magik6k mentioned this pull request Dec 14, 2017

coreapi: Basic object API implementation #4492

Merged

3 tasks

magik6k force-pushed the feat/coreapi/name branch from 9aae19d to a56eade Compare December 15, 2017 01:59

ghost reviewed Dec 15, 2017

View reviewed changes

core/coreapi/coreapi.go

api,

nil,

}

}

Copy link
Copy Markdown

ghost Dec 15, 2017

Choose a reason for hiding this comment

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

Is this multiline format what gofmt emitted?

magik6k force-pushed the feat/coreapi/name branch from a56eade to f6d74af Compare December 15, 2017 22:04

magik6k mentioned this pull request Dec 15, 2017

CoreAPI progress tracking issue #4498

Open

51 tasks

ghost reviewed Dec 16, 2017

View reviewed changes

magik6k force-pushed the feat/coreapi/name branch from f6d74af to 1d0e236 Compare December 17, 2017 00:10

ghost suggested changes Dec 17, 2017

View reviewed changes

ghost reviewed Dec 17, 2017

View reviewed changes

magik6k force-pushed the feat/coreapi/name branch 3 times, most recently from e280cd4 to b80a76e Compare December 20, 2017 12:15

magik6k added the status/blocked Unable to be worked further until needs are met label Dec 22, 2017

vyzo reviewed Dec 22, 2017

View reviewed changes

ghost approved these changes Dec 23, 2017

View reviewed changes

magik6k force-pushed the feat/coreapi/name branch from b80a76e to 1123587 Compare December 27, 2017 10:15

magik6k removed the status/blocked Unable to be worked further until needs are met label Dec 31, 2017

magik6k added 5 commits January 1, 2018 17:35

coreapi: Name API proposal

027f498

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: Keystore API proposal

1b5fbb0

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: name/key functional options

1c73d48

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: Documentation for Name/Key

587dc18

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

coreapi: name/key review suggestions

8df2d1a

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the feat/coreapi/name branch from 1123587 to 8df2d1a Compare January 1, 2018 16:38

coreapi: key tests

396c34b

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the feat/coreapi/name branch from 067ae52 to 396c34b Compare January 1, 2018 18:01

magik6k commented Jan 2, 2018

View reviewed changes

whyrusleeping reviewed Jan 2, 2018

View reviewed changes

coreapi: Name tests

2109cbc

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the feat/coreapi/name branch from 7f4f086 to 2109cbc Compare January 2, 2018 20:01

whyrusleeping approved these changes Jan 4, 2018

View reviewed changes

whyrusleeping merged commit 3af7206 into master Jan 4, 2018

whyrusleeping deleted the feat/coreapi/name branch January 4, 2018 23:08

ghost removed the status/in-progress In progress label Jan 4, 2018

Uh oh!

Conversation

magik6k commented Dec 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

whyrusleeping commented Dec 14, 2017

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!

ghost 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!

magik6k commented Dec 22, 2017

Uh oh!

vyzo left a comment • 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!

whyrusleeping commented Dec 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

magik6k commented Jan 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost Jan 2, 2018 • edited by ghost 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!

magik6k Jan 2, 2018 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

magik6k commented Dec 10, 2017 •

edited

Loading

vyzo left a comment •

edited

Loading

ghost Jan 2, 2018 •

edited by ghost

Loading

magik6k Jan 2, 2018 •

edited

Loading