api, manifest: abstract saver/loader functionality #939

acud · 2020-11-11T11:44:02Z

this is needed so we don't need to pass crypto.Signer and batch IDs to the manifest package as part of the storage incentives effort

needed for #873

zbiljic · 2020-11-11T15:38:50Z

pkg/manifest/manifest.go

@@ -36,7 +35,7 @@ type Interface interface {
 	// HasPrefix tests whether the specified prefix path exists.
 	HasPrefix(string) (bool, error)
 	// Store stores the manifest, returning the resulting address.
-	Store(context.Context, storage.ModePut) (swarm.Address, error)
+	Store() (swarm.Address, error)


I think the context.Context should be left here.

zbiljic · 2020-11-11T17:05:26Z

pkg/file/io.go

+
+type Loader interface {
+	// Load a reference in byte slice representation and return all content associated with the reference.
+	Load([]byte) ([]byte, error)


Since this is in Bee project, I think that Load/Save functions should return swarm.Address directly.

And maybe allow passing context to both operations.

this interface was made to be compatible with manifest.Loader in the manifest package. have a closer look that i am injecting this interface directly into the mantaray dependency on load and save. if you want it as an address then we need to change the interface in mantaray first

I am fully aware this was the reason. My comment is that we should not force external interface into this project.

The interface in "manatary" project was created to be independent from some external project. That interface is how it is, and should not be changed.
I would rather we have another level of indirection instead of having something like this. Especially since some other parts of Bee project might want to use this package.

zelig

lets revisit this when this is merged ethersphere/manifest#16

pkg/api/dirs.go

pkg/api/dirs_test.go

pkg/api/dirs.go

pkg/manifest/manifest.go

janos · 2020-11-12T12:50:22Z

Compilation errors should be fixed https://github.com/ethersphere/bee/pull/939/checks?check_run_id=1390345361#step:7:9.

acud · 2020-11-12T13:02:22Z

@janos errors are intentional. waiting for viktor's PR to be merged to the manifest project

zelig · 2020-12-02T20:15:47Z

pkg/file/io.go

@@ -107,3 +107,18 @@ func SplitWriteAll(ctx context.Context, s Splitter, r io.Reader, l int64, toEncr
 	}
 	return addr, nil
 }
+


these interfaces are defined twice. we should use only one and have it in storage package maybe

these interfaces provide functionalities which are wrapping components in the file package. that's why I put them here. it belongs here more than in storage, which relates more to localstore and the likes

pkg/traversal/traversal_test.go

pkg/api/api.go

pkg/api/dirs.go

pkg/api/file.go

zelig

LGTM

zbiljic

Only things I think should be changed is LoadSaver function signatures, to use swarm.Address. That way package may be usable for other things in the future.

zbiljic · 2020-12-04T14:30:30Z

pkg/file/io.go

+
+type Loader interface {
+	// Load a reference in byte slice representation and return all content associated with the reference.
+	Load(context.Context, []byte) ([]byte, error)


I think since we are in the bee project, the Load and Save function should work directly with swarm.Address, instead of using []byte.

This may require having some proxy implementation for manifest, where we have implementation with byte slice, which translates to swarm.Address.

but then it means I need an extra abstraction that would use this one. I'm not sure why this is necessary. I only created this implementation to be compatible with the manta ray interfaces. I think that once other components need to use similar functionality we can generalize, but I'm not sure why to do so prematurely

Yeah, I just do not think application should cater to library completely.

It can be seen that references are already used, but the simple manifest implementation.

zbiljic · 2020-12-04T14:34:32Z

pkg/manifest/simple.go

 }

 // NewSimpleManifest creates a new simple manifest.
 func NewSimpleManifest(
-	encrypted bool,
-	storer storage.Storer,
+	ls file.LoadSaver,


Function signature may be collapsed now (it should be enough shorter).

bee-runner bot added the pull-request label Nov 11, 2020

acud force-pushed the manifest-saver branch from 45827df to 483cc2c Compare November 11, 2020 11:44

acud self-assigned this Nov 11, 2020

acud added the in progress ongoing development , hold out with review label Nov 11, 2020

acud force-pushed the manifest-saver branch from 13df3e7 to 5ca20ba Compare November 11, 2020 14:34

acud added ready for review The PR is ready to be reviewed and removed in progress ongoing development , hold out with review labels Nov 11, 2020

acud requested review from zbiljic, janos and zelig and removed request for zbiljic November 11, 2020 14:35

zbiljic suggested changes Nov 11, 2020

View reviewed changes

zelig requested changes Nov 12, 2020

View reviewed changes

acud added in progress ongoing development , hold out with review and removed ready for review The PR is ready to be reviewed labels Nov 12, 2020

acud force-pushed the manifest-saver branch from dfece68 to 66abed1 Compare November 19, 2020 15:14

acud force-pushed the manifest-saver branch from 66abed1 to f7dea45 Compare December 2, 2020 09:38

abstract stuff out of manifest package

14c545a

acud force-pushed the manifest-saver branch from 702bfde to 14c545a Compare December 2, 2020 13:49

acud requested review from zbiljic and zelig December 2, 2020 13:49

acud added ready for review The PR is ready to be reviewed and removed in progress ongoing development , hold out with review labels Dec 2, 2020

zelig reviewed Dec 2, 2020

View reviewed changes

acud added 2 commits December 3, 2020 10:47

move context to actual pipeline call

2f104f8

ls outside of switch

6b9eb3b

acud requested a review from zelig December 3, 2020 11:11

zelig approved these changes Dec 4, 2020

View reviewed changes

zbiljic suggested changes Dec 4, 2020

View reviewed changes

pr comment

87f35e1

zbiljic approved these changes Dec 4, 2020

View reviewed changes

acud merged commit 0b337f6 into master Dec 4, 2020

acud deleted the manifest-saver branch December 4, 2020 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api, manifest: abstract saver/loader functionality #939

api, manifest: abstract saver/loader functionality #939

acud commented Nov 11, 2020 •

edited

Loading

zbiljic Nov 11, 2020

zbiljic Nov 11, 2020

acud Nov 12, 2020

zbiljic Nov 12, 2020

zelig left a comment

janos commented Nov 12, 2020

acud commented Nov 12, 2020

zelig Dec 2, 2020

acud Dec 3, 2020 •

edited

Loading

zelig left a comment

zbiljic left a comment

zbiljic Dec 4, 2020

acud Dec 4, 2020

zbiljic Dec 4, 2020 •

edited

Loading

zbiljic Dec 4, 2020

api, manifest: abstract saver/loader functionality #939

api, manifest: abstract saver/loader functionality #939

Conversation

acud commented Nov 11, 2020 • 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

zelig left a comment

Choose a reason for hiding this comment

janos commented Nov 12, 2020

acud commented Nov 12, 2020

Choose a reason for hiding this comment

acud Dec 3, 2020 • edited Loading

Choose a reason for hiding this comment

zelig left a comment

Choose a reason for hiding this comment

zbiljic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zbiljic Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acud commented Nov 11, 2020 •

edited

Loading

acud Dec 3, 2020 •

edited

Loading

zbiljic Dec 4, 2020 •

edited

Loading