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

chore(store)!: move protocol implementation opinions to @waku/sdk #1913

Merged
merged 15 commits into from
Apr 1, 2024

Conversation

danisharora099
Copy link
Collaborator

@danisharora099 danisharora099 commented Mar 13, 2024

Problem

#1886

Solution

Moves away the abstraction of using multiple peers, along with the offered APIs to make queries for store by:

  • creating a new class StoreSDK (and renaming protocol implementation to StoreCore
    • StoreCore contains one API that returns an AsyncGenerator: *queryPerPage
    • StoreSDK offers different APIs to query, that uses queryPerPage
    • Increases readability and modularity

Notes

Contribution checklist:

  • covered by unit tests;
  • covered by e2e test;
  • add ! in title if breaks public API;

Copy link

github-actions bot commented Mar 13, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
Waku node 185.75 KB (+0.26% 🔺) 3.8 s (+0.26% 🔺) 3.1 s (+84.81% 🔺) 6.8 s
Waku Simple Light Node 185.45 KB (-0.03% 🔽) 3.8 s (-0.03% 🔽) 3.3 s (+4.51% 🔺) 7 s
ECIES encryption 22.88 KB (0%) 458 ms (0%) 453 ms (-33.82% 🔽) 911 ms
Symmetric encryption 22.42 KB (0%) 449 ms (0%) 592 ms (-15.19% 🔽) 1.1 s
DNS discovery 73.67 KB (0%) 1.5 s (0%) 2.2 s (+20.9% 🔺) 3.7 s
Peer Exchange discovery 75.37 KB (0%) 1.6 s (0%) 1.7 s (-27.37% 🔽) 3.2 s
Local Peer Cache Discovery 68.99 KB (0%) 1.4 s (0%) 1.4 s (-17.48% 🔽) 2.8 s
Privacy preserving protocols 39.97 KB (0%) 800 ms (0%) 1.4 s (-19.7% 🔽) 2.2 s
Waku Filter 20.11 KB (-0.03% 🔽) 403 ms (-0.03% 🔽) 515 ms (+22.71% 🔺) 917 ms
Waku LightPush 115.41 KB (-0.11% 🔽) 2.4 s (-0.11% 🔽) 2.2 s (-21.87% 🔽) 4.5 s
History retrieval protocols 116.05 KB (+500.75% 🔺) 2.4 s (+500.75% 🔺) 2.5 s (+713.01% 🔺) 4.8 s
Deterministic Message Hashing 4.96 KB (0%) 100 ms (0%) 48 ms (+39.99% 🔺) 147 ms

]
],
"dependencies": {
"@waku/proto": "^0.0.6"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we should add it as dependency to @waku/interfaces

Copy link
Collaborator Author

@danisharora099 danisharora099 Mar 22, 2024

Choose a reason for hiding this comment

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

why is that? 🤔
as i see it, it does not bring any breaking changes, in fact reduces redundant type declarations that can be imported from proto such as done for Cursor in this PR

Copy link
Collaborator

Choose a reason for hiding this comment

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

@waku/interface name implies to be interfaces only and to have little to no runtime footprint. as I mentioned somewhere in your PRs - right now we abuse enums and use them as const (note: this is not necessarily bad thing with enums)

as for @waku/proto - it has enormous runtime footprint and has nothing to do with ts types directly

we should think of work around here:

  • defining type in @waku/interfaces and using in @waku/proto instead
  • duplicating type in @waku/interfaces and using it from here instead of @waku/proto

maybe you think of something else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

defining type in @waku/interfaces and using in @waku/proto instead

this seems challenging because proto ts files are generated from .proto definitions, and we shouldn't be manually updating those

duplicating type in @waku/interfaces and using it from here instead of @waku/proto

this is what we have been doing so far

Copy link
Collaborator

Choose a reason for hiding this comment

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

this is what we have been doing so far

I probably missed it. Could you, please, point me where?

I checked build artifact and it doesn't contain any footprint of proto, that means it is smart enough to understand there is only type import

as a precaution I propose to make it devDependency (whole @waku/interfaces is kinda a dev dependnecy anyway) and change import { proto_store as proto } from "@waku/proto"; to something like import type { proto_store as proto } from "@waku/proto"; in store.ts

This way we ensure it is not increasing size and it doesn't leak anything else apart from types

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 probably missed it. Could you, please, point me where?

For example: Cursor:
@waku/proto definition:

export interface Index {
digest: Uint8Array
receiverTime: bigint
senderTime: bigint
pubsubTopic: string
}

@waku/interfaces redefinition:
export interface Cursor {
digest: Uint8Array;
receiverTime: bigint;
senderTime: bigint;
pubsubTopic: string;
}

as a precaution I propose to make it devDependency (whole @waku/interfaces is kinda a dev dependnecy anyway) and change import { proto_store as proto } from "@waku/proto"; to something like import type { proto_store as proto } from "@waku/proto"; in store.ts

This way we ensure it is not increasing size and it doesn't leak anything else apart from types

I agree with this, approach wise

Copy link
Collaborator

@weboko weboko left a comment

Choose a reason for hiding this comment

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

let's discuss

packages/sdk/package.json Outdated Show resolved Hide resolved
@danisharora099 danisharora099 requested review from weboko and a team March 28, 2024 09:06
Copy link
Collaborator

@weboko weboko left a comment

Choose a reason for hiding this comment

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

thank you for addressing comments!

@danisharora099 danisharora099 merged commit bf42c8f into master Apr 1, 2024
8 of 10 checks passed
@danisharora099 danisharora099 deleted the chore/refactor-store branch April 1, 2024 11:17
@weboko weboko mentioned this pull request Apr 9, 2024
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.

2 participants