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.

Sign up for GitHub

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

Jump to bottom

feat(PE-4472): add uploadFile, uploadSignedDataItem implementations for node and web #9

Merged
merged 30 commits into from
Sep 7, 2023
Merged

feat(PE-4472): add uploadFile, uploadSignedDataItem implementations for node and web #9

merged 30 commits into from
Sep 7, 2023

Conversation

dtfiedler
Copy link
Collaborator

@dtfiedler dtfiedler commented Aug 28, 2023 •
edited
Loading

TODO:

  • consolidate the GET/POST requests to turbo into a TurboHTTPService class that abstracts away axios
  • add AbortController to uploads
  • implement uploadSignedDatatItem for web - there is no easy way to do this with ReadableStreams - so i may table the implementation for a separate PR (feel free to provide any thoughts, ChatGPT is a bit volatile in it's recs)
  • handle signing ReadableStreams with something like streamSigner so they don't have to be loaded in memory - will plan to implement in a separate PR
  • additional tests
  • rename public and private static functions to unauthenticated and authenticated

Most Recent Updates:

  • abstracted privateKey to a TurboAreaveWallet class - right now it wraps arbundles ArweaveSigner depending on the environment (web vs. node) - we can move towards implementing ourselves if we want to avoid their stuff
  • leveraged some inheritance for Authenticated classes - this avoids redundancies and is hopefully more approachable for devs, but does blend composition vs. inheritance a bit
  • created TurboHTTPService that abstracts away axios configuration and handles AbortControllers passed to it. We may eventually want to move away from axios so we could consider changing the CancelledError to some custom error

Build Configuration

  • @ardrive/turbo-sdk/node includes the exports for NodeJS environments
  • @ardrive/turbo-sdk/web includes the exports for Browser environments

@dtfiedler dtfiedler changed the title feat: add uploadFiles implementation for node and web feat(PE-4472): add uploadFiles implementation for node and web Aug 28, 2023
@dtfiedler dtfiedler force-pushed the PE-4472-uploads branch 2 times, most recently from 218b5b8 to 42c2ed9 Compare August 28, 2023 16:33
fedellen
fedellen reviewed Aug 31, 2023
View reviewed changes
examples/node/index.mjs
* Alternatively instantiate your own clients independently.
*/
const paymentService = new TurboUnauthenticatedPaymentService({
url: 'https://payment.ardrive.dev',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Writing down some aside thoughts on exposing our ArDrive Turbo "testnet"

This public test API (payment.ardrive.dev / upload.ardrive.dev):

  • will accept Stripe Test Cards to reward Credits
  • sends data to a live production gateway (arweave.net)
  • posts optical bridge GQL info to prod gateway (arweave.net)
  • has an optimistic data cache with dev gateway (arweave.dev)
  • also accepts free data under 500 KiB

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel our testnet could offer developers great advantages during building. But our test data should expire and never make it to a live arweave node

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

agreed - we may need to consider a separate public test environment that is low-cost for us but dev friendly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also a good point to provide some testing utils and constants to help devs write unit tests with the SDK (e.g. test card numbers)

@fedellen fedellen self-requested a review August 31, 2023 15:35
fedellen
fedellen reviewed Aug 31, 2023
View reviewed changes
examples/node/index.mjs Outdated Show resolved Hide resolved
fedellen
fedellen reviewed Aug 31, 2023
View reviewed changes
examples/node/index.mjs Outdated Show resolved Hide resolved
fedellen
fedellen reviewed Aug 31, 2023
View reviewed changes
tests/turbo.test.ts Outdated Show resolved Hide resolved
fedellen
fedellen reviewed Aug 31, 2023
View reviewed changes
src/types/turbo.ts Outdated Show resolved Hide resolved
fedellen
fedellen reviewed Aug 31, 2023
View reviewed changes
src/utils/readableStream.ts
Comment on lines +17 to +21
export async function readableStreamToBuffer({
stream,
}: {
stream: ReadableStream;
}): Promise<Buffer> {
Copy link
Collaborator

@fedellen fedellen Aug 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Doesn't streamToBuffer gets a performance boost by including the content length?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, a big boost.

fedellen
fedellen reviewed Aug 31, 2023
View reviewed changes
src/web/signer.ts Outdated
Comment on lines 46 to 55
async (streamGenerator: () => ReadableStream) => {
// Convert the readable stream to a Blob
const buffer = await readableStreamToBuffer({
stream: streamGenerator(),
});
const arrayBuffer = Uint8Array.from(buffer);
// convert the blob to a Uint8Array
const dataItem = createData(arrayBuffer, signer);
await dataItem.sign(signer);
return dataItem.getRaw();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to convert from readable stream to buffer on the web path?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like if we got all the polyfills setup right for arbundles and the client sends a readable stream generator, we should be able to use the streamSigner like the node path

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

streamSigner only supports Readables at the moment - we might be able to create our own implementation that handles ReadableStreams. right now ReadableStreams get loaded into memory (buffer) - so not optimal

@fedellen
Copy link
Collaborator

Looking good, left some thoughts

@dtfiedler dtfiedler force-pushed the PE-4472-uploads branch 2 times, most recently from bd67dc5 to 8a79000 Compare August 31, 2023 21:49
@dtfiedler dtfiedler mentioned this pull request Sep 1, 2023
Merged
dtfiedler added 2 commits September 1, 2023 11:53
feat: remove TurboDataItemVerifier
fee5675 
We actually do not need to worry about validating the signatures of signed data items sent to the SDK. They will just be forwarded to the Upload Service and it will be responsible for validating the signatures of those data items. Makes it much easier on the SDK side and avoids having to worry about verifying sigs in web vs. node environments.
chore: remove unused 'bundle' flag on uploadFiles, update tests to pr…
8262c90 
…ovide multiple data items
arielmelendez
arielmelendez reviewed Sep 1, 2023
View reviewed changes
src/common/payment.ts Outdated
constructor({
url = 'https://payment.ardrive.dev',
retryConfig,
privateKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider abstracting this out into a Signer-like interface for which we could provide some basic implementations. But I just see it as something we'll almost certainly have to excise out in a later version as we begin to abstract for other chains or wallet connection types (e.g. ArConnect).

chore: some cleanup items
538f2f8 
- change Public/Private interfaces to Unauthenticated<Service>Interface
- update tests for uploading signedDataItems
- add TODOs for creating ReadableStreams
- cleanup examples
chore: use inheritance for private classes
16ff144 
While we generally favor composition over inheritance, this pattern is easy to comprehend and avoids redunandancy for simp
e HTTP requests that don't have unique implementations.
@dtfiedler dtfiedler changed the title feat(PE-4472): add uploadFiles implementation for node and web feat(PE-4472): add uploadFile, uploadSignedDataItem implementations for node and web Sep 5, 2023
@dtfiedler dtfiedler marked this pull request as ready for review September 5, 2023 22:20
@dtfiedler dtfiedler requested a review from a team as a code owner September 5, 2023 22:20
dtfiedler added 4 commits September 5, 2023 18:28
chore: update build outputs to avoid SubtleCrypto errors
f0a19e0 
We separate the builds into differnet tsconfig.json files so specific deps required in web do not get pulled into NodeJS environments and vice versa
chore(README): add a skeleton README with placeholders for API specs
3acba94
chore: pull up TurboBaseFactory to common
96208ea 
Implement private in enviornment specific TurboFactory, update examples in README
chore: cleanup comments, fix lib package outputs
b56216e
dtfiedler
dtfiedler commented Sep 6, 2023
View reviewed changes
package.json
Comment on lines +29 to 38
"./node": {
"import": "./lib/node/index.js",
"require": "./lib/node/index.js",
"types": "./lib/types/index.d.ts"
},
"./web": {
"import": "./lib/web/index.js",
"require": "./lib/web/index.js",
"types": "./lib/types/index.d.ts",
"browser": "./bundles/web.bundle.min.js"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these are intentional - and we may want to remove .

for node environments - the only exports available to clients are in lib/node/index.js
for web - the only exports available to clients are from lib/web/index.js

other utilities/common exports are internal and not able to be imported by consuming clients - this should make it very hard to use the SDK incorrectly

@dtfiedler
Copy link
Collaborator Author

Two additional follow up comments:

  • ensure we're not managing AbortController timeouts internally
  • add keepAlive configuration to keep axios connection warm

@dtfiedler dtfiedler merged commit bf2a054 into main Sep 7, 2023
8 checks passed
@dtfiedler dtfiedler deleted the PE-4472-uploads branch September 7, 2023 21:52
@ardrive-bot
Copy link

🎉 This PR is included in version 1.0.0-alpha.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@ardrive-bot
Copy link

🎉 This PR is included in version 1.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedellen fedellen fedellen approved these changes

@arielmelendez arielmelendez Awaiting requested review from arielmelendez

Assignees
No one assigned
Labels
released on @alpha released on @beta
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dtfiedler @fedellen @ardrive-bot @arielmelendez