Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upload with postage stamps #212

Closed
Eknir opened this issue Mar 16, 2021 · 6 comments
Closed

Upload with postage stamps #212

Eknir opened this issue Mar 16, 2021 · 6 comments
Labels
kind:enhancement A net-new feature or an improvement to an existing feature need:analysis Needs further analysis before proceeding type:issue

Comments

@Eknir
Copy link

Eknir commented Mar 16, 2021

As an uploader of content, I want to upload my content without being bothered by the details on purchasing stamps and attaching them, such that uploading content is not much more complicated that what it used to be before postage stamps were required.

Obvious integrations are:

  • swarm-cli
  • gateway
  • bee-status

This issue is a first step, the scope should be minimal and ideally, it should be released at the same time as when the postage stamps are released (or shortly thereafter).

@agazso agazso added kind:enhancement A net-new feature or an improvement to an existing feature need:analysis Needs further analysis before proceeding labels Mar 16, 2021
@AuHau
Copy link
Contributor

AuHau commented May 4, 2021

So I have started working on this as part of the #290 but I was expecting that stamps will be optional and hence choose some way how to design API around that. Now it seems that stamps will be mandatory and then it is a question if how to design the API around that. I see two ways.

Mandatory parameter

Having all the upload methods like bee.uploadData(), bee.uploadFiles() required parameter batchId that needs to be passed when uploading data. This is somewhat not great DX as the user's of bee-js has to do the stamps management across their whole app.

Example:

uploadData(data: string | Uint8Array, stamp: string | Stamp, options?: UploadOptions): Promise<Reference>

Instance's default batch

To my understanding, it is not really expected for most of the use cases to have a lot of (small) different batches as it requires on-chain transactions that are expensive, so most probably the users will create a batch that they will be employing for a longer time. @Eknir is this a valid assumption?
If so, then I would suggest a similar design like with signer, where default signer can be specified for the Bee class instance and can be specified in options where it is needed if users needs to override the default.

Example:

class Bee{
//...
	constructor(url: string, {batchId, ...}: BeeOptions) {
		//...
	}

//...

uploadData(data: string | Uint8Array, options?: UploadOptions): Promise<Reference>
}

Where options support batchId as well and at least one has to be configured.

@Eknir @agazso what are your thoughts?

@agazso
Copy link
Member

agazso commented May 5, 2021

I am still not sure if the postage stamps are mandatory or optional. @Eknir or @acud can you please share more information about this?

If it's optional I would make it part of the options. If it's mandatory it's possible to implement in the two ways that is outlined. I am not a huge fan of the way we did the signer, because if it's optional in both the constructor and in the upload then we can no longer rely on the type system to enforce correct invocations and we have to do runtime checks instead. However since we already have this pattern with the signer it may be more consistent to do the same.

@acud
Copy link
Member

acud commented May 6, 2021

We went through this on the public demo session - right now postage stamps are mandatory, this is also reflected in the API design - if you try to upload something without a batch ID in the header, the upload will fail on a 400 Bad Request.

@AuHau
Copy link
Contributor

AuHau commented May 6, 2021

...no longer rely on the type system to enforce correct invocations and we have to do runtime checks instead.

Well, I mean TypeScript is not giving you any guarantees. I actually think this is the "dark side" of TS when a lot of people start to "feel safe" because they have types check and hence are not doing runtimes checks and then when some JS dev uses your library he is surprised what it can do with it, usually leading with very errors that do not make sense. So I don't see it as a negative thing.

We went through this on the public demo session - right now postage stamps are mandatory, this is also reflected in the API design - if you try to upload something without a batch ID in the header, the upload will fail on a 400 Bad Request.

Thanks for the input. Well, I will move forward then with the "signer approach".

@agazso
Copy link
Member

agazso commented May 7, 2021

Well, I mean TypeScript is not giving you any guarantees. I actually think this is the "dark side" of TS when a lot of people start to "feel safe" because they have types check and hence are not doing runtimes checks and then when some JS dev uses your library he is surprised what it can do with it, usually leading with very errors that do not make sense. So I don't see it as a negative thing.

This is not really what I meant (and we are doing input validation anyhow). What I meant is that by making a mandatory argument optional and also optional in the constructor it is no longer possible to evaluate and therefore understand the code with an upload call without a postage stamp argument if the Bee instance creation is not in the same scope.

For example, imagine if you only see this in the code somewhere:

// ...
await bee.uploadData(data)
// ...

It may be a valid or invalid call, depending on how the bee object was instantiated. It may also be valid (in the sense that there is a batchId provided in the constructor) but incorrect in the context when using the wrong bee instance. But because the mandatory argument is decoupled from the actual call you have to verify the instance creation calls and all the different ways the bee object can be passed until it arrives to the actual invocation and that may be not always trivial.

I feel that this style makes the code slightly easier to write but harder to read ultimately. So this may be better for hackathon style of proof-of-concept projects but harder to maintain on the long term.

@AuHau
Copy link
Contributor

AuHau commented May 19, 2021

This was resolved in #290

@AuHau AuHau closed this as completed May 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:enhancement A net-new feature or an improvement to an existing feature need:analysis Needs further analysis before proceeding type:issue
Projects
None yet
Development

No branches or pull requests

4 participants