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

PE-4318: implement stream functions #52

Merged
merged 33 commits into from
Oct 26, 2023
Merged

PE-4318: implement stream functions #52

merged 33 commits into from
Oct 26, 2023

Conversation

karlprieb
Copy link
Collaborator

No description provided.

@karlprieb karlprieb marked this pull request as draft August 23, 2023 15:40
@karlprieb karlprieb force-pushed the PE-4318 branch 2 times, most recently from 21142d6 to 9f263f5 Compare August 25, 2023 20:45
@karlprieb karlprieb force-pushed the PE-4318 branch 2 times, most recently from 6bc1878 to 1fd8170 Compare August 30, 2023 17:36
input: signatureData,
signature: signature,
modulus: decodeBytesToBigInt(owner),
publicExponent: BigInt.from(65537));

Choose a reason for hiding this comment

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

why 65537?

Copy link

Choose a reason for hiding this comment

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

It seems to be the Largest known Fermat prime. We could give it a name if we make a constant for this value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@dtfiedler
Copy link

there's likely similar patterns for handling streams and packaging bundles relevant to the turbo-sdk ardriveapp/turbo-sdk#9

@karlprieb karlprieb marked this pull request as ready for review September 18, 2023 12:55
Copy link

@matibat matibat left a comment

Choose a reason for hiding this comment

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

Reviewed half of the code. I'll continue tomorrow. Looking great so far. 🚀

.gitignore Show resolved Hide resolved
@@ -0,0 +1,20 @@
{
description = "A basic shell for arweave-dart";
inputs.nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
Copy link

Choose a reason for hiding this comment

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

Do we need unstable packages?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's only for nix env (which I'm the only on using right now).
nixpkgs has 3 types of releases, the master branch which is unstable, unstable branch which is like a rolling release RC and tagged branches which are stable. Stable tags are updated at each 6˜12 months.
Flutter and dart v3 are not on tagged branches yet.
unstable doesn't mean it's really unstable, just that it was not tagged yet.

Copy link

Choose a reason for hiding this comment

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

Don't we want to ignore this file?

Copy link

Choose a reason for hiding this comment

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

Perhaps the whole folder, I'd say

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so. It's expected for users to copy and use this js file if they want to support web environments

Copy link

Choose a reason for hiding this comment

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

I mean, this is generated code and not source files. We usually git-ignore the generated code because we do generate it on build time.

But if we're aware of that and this is intended then I'm good with it.

js/sha384/package.json Outdated Show resolved Hide resolved
js/sha384/src/index.ts Show resolved Hide resolved
Comment on lines +120 to +121
final tagsBytes = decodeBytesToLong(await reader.readBytes(8));
byteIndex += 8;
Copy link

Choose a reason for hiding this comment

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

This one seems to be an unused variable. Is it perhaps a duplicate of what's below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's unused, but not duplicated. The first 8 bytes determine the bytes of the tags and we don't use it for anything right now, but we should read them from the stream as we follow the spec order of bytes

Copy link

Choose a reason for hiding this comment

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

Ya, but isn't it the same as what's below? If not then let's consider using another name to differentiate both.

  // get tags bytes length
  final tagsBytesLenthBytes = await reader.readBytes(8);
  final tagsBytesLength = decodeBytesToLong(tagsBytesLenthBytes);
  byteIndex += 8;

Copy link

Choose a reason for hiding this comment

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

We just confirmed with @thiagocarvalhodev that the first one represents the amount of tags there are, and the second one represents the total size in bytes for the whole tags altogether.

We could rename them to make obvious the difference.

input: signatureData,
signature: signature,
modulus: decodeBytesToBigInt(owner),
publicExponent: BigInt.from(65537));
Copy link

Choose a reason for hiding this comment

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

It seems to be the Largest known Fermat prime. We could give it a name if we make a constant for this value.

lib/src/streams/data_models.dart Show resolved Hide resolved
}
}

Future<Uint8List> deepHashChunks(dynamic chunks, Uint8List acc) async {
Copy link

Choose a reason for hiding this comment

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

Should we type chunks? It seems to be a list-like object.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deepHashChunks can be either a Stream<Uint8List>, Uint8List, a List<List> or List<Stream<Uint8List>>. As Dart doesn't have union types, using dynamic was the best option that I found :(

Do you have any suggestion?

Copy link

Choose a reason for hiding this comment

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

I was thinking of making the type be Iterable<dynamic> but I see Stream implements neither List nor Iterable.

What we can do is what you did for deepHashStream: to check if data is Stream<Uint8List> or data is List<List> || data is List<Stream<Uint8List>>, and to throw otherwise.

Having the method as it is (accessing properties we assume it has) is dangerous. 😬

lib/src/streams/sha384_web.dart Outdated Show resolved Hide resolved
Copy link

@matibat matibat left a comment

Choose a reason for hiding this comment

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

Finished reviewing :)

input: signatureData,
signature: signature,
modulus: decodeBytesToBigInt(owner),
publicExponent: BigInt.from(65537));
Copy link

Choose a reason for hiding this comment

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

We're using this number in many places, let's make a constant for it. Or perhaps to factor out this code into it's own method: bool isSignatureValid(signatureData, signature, owner)

lib/src/streams/utils.dart Show resolved Hide resolved
pubspec.lock Outdated Show resolved Hide resolved
test/data_bundle_test.dart Show resolved Hide resolved
Comment on lines +33 to +34
group('[streams][data_item]', () {
test('create and sign data item', () async {
Copy link

Choose a reason for hiding this comment

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

Nit: we usually set the top-level group's description as the name of the component we're testing so you can read: "Component X behaves like THIS with the input A" - E.g. "createDataItemTaskEither method" "instantiates a stream that creates and sign a data item"

@karlprieb karlprieb changed the title PE-4318: implement stream functions [WIP] PE-4318: implement stream functions Oct 6, 2023
karlprieb and others added 8 commits October 6, 2023 10:51
- use the url to the git
- implements the abort upload functionality. It adds to new classes: `ChunkUploader` and `UploadAborter`.
The API for the method for upload also changed.
…-stop-file-upload-when-using-a-rs-only

PE-4812: add option to cancel an ongoing upload
- uses the fetch_client fork with the cancel option
- bumps the package version
@thiagocarvalhodev thiagocarvalhodev self-assigned this Oct 26, 2023
Copy link

@matibat matibat left a comment

Choose a reason for hiding this comment

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

Comment on lines +153 to +155
typedef BundledDataItemResult
= Future<TaskEither<StreamTransactionError, DataItemResult>>;
BundledDataItemResult createBundledDataItemTaskEither({
Copy link

Choose a reason for hiding this comment

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

It seems createDataBundleTaskEither is returning DataBundleResult instead of DataItemResult.

Comment on lines +242 to +244
// Will be refactored to use TaskEither

// Future<List<Map<String, dynamic>>> processBundle(
Copy link

Choose a reason for hiding this comment

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

Something to do here? Can be cleaned up?

Comment on lines +120 to +121
final tagsBytes = decodeBytesToLong(await reader.readBytes(8));
byteIndex += 8;
Copy link

Choose a reason for hiding this comment

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

We just confirmed with @thiagocarvalhodev that the first one represents the amount of tags there are, and the second one represents the total size in bytes for the whole tags altogether.

We could rename them to make obvious the difference.


Client getClient() => FetchClient(
mode: RequestMode.cors,
// streamRequests: true,
Copy link

Choose a reason for hiding this comment

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

Commented code.

@thiagocarvalhodev thiagocarvalhodev merged commit a61f9d7 into master Oct 26, 2023
3 checks passed
@thiagocarvalhodev thiagocarvalhodev deleted the PE-4318 branch October 26, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants