api: add postage batch id header #886

acud · 2020-10-30T09:15:43Z

Adds a postage batch ID header to the API

related to #872

zelig

i do not see any tests
the post package should be removed, see pr postage: new pkg for postage stamps, uploader stamping #890
batch id should be inherited through the context, not added to every method

zelig · 2020-11-03T12:46:29Z

pkg/post/stamper.go

@@ -0,0 +1,12 @@
+package post


this file is not needed and should be removed

acud · 2020-11-03T13:27:38Z

batch id should be inherited through the context, not added to every method

context attached values are optional by definition. if i understand correctly, batch id is not optional. correct me if i'm wrong

acud · 2020-11-03T14:42:43Z

i do not see any tests

I don't see what to test here (I kid you not). Let's add tests to this package once the stamper is wired into the pipeline, this way we can mock it and assert it was called with the correct value

janos

This PR needs tests for at least that the http header is handled as expected, like we have for other headers.

janos · 2020-11-03T15:41:06Z

pkg/api/api.go

@@ -47,8 +49,15 @@ const (
 var (
 	errInvalidNameOrAddress = errors.New("invalid name or bzz address")
 	errNoResolver           = errors.New("no resolver connected")
+	errInvalidPostageBatch  = errors.New("invalid postage batch id")
+
+	fallbackPostageBatch []byte


This is a mutable shared variable between multiple goroutines and we cannot be sure that it will not be mutated, as it is passed to other components. I assume that it is an optimization to avoid allocations, but, I think that this optimization is not needed, and it is better to be safe.

pkg/api/api.go

janos

Thanks for addressing my comments. 👍

zelig

i do not understand fallback to all zeros batch id otherwise ok

zbiljic · 2020-11-08T21:32:35Z

pkg/api/api.go

+		if len(h) != 64 {
+			return nil, errInvalidPostageBatch
+		}
+		return hex.DecodeString(h)


If value in the header is not hexadecimal it will return some internal error. This might have been handled, and returned errInvalidPostageBatch again.

makes sense 👍 thanks

postage: new pkg for postage stamps, uploader stamping (#890) * postage: new pkg for postage stamps, uploader stamping * postage: amount->value, blockNumber big.Int-> uint64, stamp only has batch ID, not Batch * postage: fix godoc and copyright * postage, swarm: - swarm.Stamp as an interface - add postage/testing for mock Stamps - fix Stamp MarshalBinary to allow nil batch id and signature - add StampSize const * postage: heed review feedback Co-authored-by: acud <12988138+acud@users.noreply.github.com>

postage: new pkg for postage stamps, uploader stamping (#890) * postage: new pkg for postage stamps, uploader stamping * postage: amount->value, blockNumber big.Int-> uint64, stamp only has batch ID, not Batch * postage: fix godoc and copyright * postage, swarm: - swarm.Stamp as an interface - add postage/testing for mock Stamps - fix Stamp MarshalBinary to allow nil batch id and signature - add StampSize const * postage: heed review feedback Co-authored-by: acud <12988138+acud@users.noreply.github.com> Storage incentives: add stamper putter to api Postage BatchStore and BatchService (#1070) Co-authored-by: zelig <viktor.tron@gmail.com> add normalisedBalance to updater interface (#1108) make value the normalised balance (#1111) postage: add event listener (#1099) Wire up postage stamp syncing (#1114) localstore, shed: persist stamps (#1116) add --postage to beeinfra.sh setup pullsync, pushsync: add postage stamps (#1117) postage: add create endpoint (#1142) retrieve erc20 address from postage contract (#1169) postage: check balance before attempting stamp creation (#1177) postage: fix bucket depth (#1178) api: use hex encoding in postage api (#1179) increase page size (#1182) postage: handle bucket depth error in api (#1183) localstore: attach stamp to outgoing chunk (#1192) update postage stamp contract addresses for new token (#1208) batchstore: reserve (#1262) * postage/batchstore: reserve logic Co-authored-by: acud <12988138+acud@users.noreply.github.com> stamp support in storage and protocols (#1321) api: endpoints for stamp issuers (#1535) retrieval: add stamps (#1552) localstore reserve logic (#1322) Co-authored-by: acud <12988138+acud@users.noreply.github.com>

acud added the in progress ongoing development , hold out with review label Oct 30, 2020

acud self-assigned this Oct 30, 2020

bee-runner bot added the pull-request label Oct 30, 2020

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

acud requested review from zbiljic, janos and zelig November 3, 2020 11:20

zelig reviewed Nov 3, 2020

View reviewed changes

pkg/post/stamper.go Outdated

@@ -0,0 +1,12 @@

package post

Copy link

Member

zelig Nov 3, 2020

Choose a reason for hiding this comment

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

this file is not needed and should be removed

janos reviewed Nov 3, 2020

View reviewed changes

acud removed the ready for review The PR is ready to be reviewed label Nov 5, 2020

janos reviewed Nov 5, 2020

View reviewed changes

pkg/api/api.go Outdated Show resolved Hide resolved

acud requested review from janos and zelig November 5, 2020 10:44

acud added the ready for review The PR is ready to be reviewed label Nov 5, 2020

janos approved these changes Nov 5, 2020

View reviewed changes

zelig approved these changes Nov 8, 2020

View reviewed changes

zbiljic approved these changes Nov 8, 2020

View reviewed changes

api: accept postage id header and propagate it to pipeline

926989f

acud force-pushed the http-postage-header branch from 7d2ab49 to 926989f Compare November 9, 2020 11:21

acud changed the base branch from master to storage-incentives November 10, 2020 14:13

acud merged commit 23b818d into storage-incentives Nov 10, 2020

acud deleted the http-postage-header branch November 10, 2020 14:13

acud added a commit that referenced this pull request Dec 2, 2020

api: accept postage id header and propagate it to pipeline (#886)

d678be1

acud added a commit that referenced this pull request Dec 7, 2020

api: accept postage id header and propagate it to pipeline (#886)

f64b294

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

api: add postage batch id header #886

api: add postage batch id header #886

acud commented Oct 30, 2020

zelig left a comment

zelig Nov 3, 2020

acud commented Nov 3, 2020

acud commented Nov 3, 2020

janos left a comment

janos Nov 3, 2020

janos left a comment

zelig left a comment

zbiljic Nov 8, 2020

acud Nov 9, 2020

api: add postage batch id header #886

api: add postage batch id header #886

Conversation

acud commented Oct 30, 2020

zelig left a comment

Choose a reason for hiding this comment

zelig Nov 3, 2020

Choose a reason for hiding this comment

acud commented Nov 3, 2020

acud commented Nov 3, 2020

janos left a comment

Choose a reason for hiding this comment

janos Nov 3, 2020

Choose a reason for hiding this comment

janos left a comment

Choose a reason for hiding this comment

zelig left a comment

Choose a reason for hiding this comment

zbiljic Nov 8, 2020

Choose a reason for hiding this comment

acud Nov 9, 2020

Choose a reason for hiding this comment