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

Protocol based on serde #2161

Open
wants to merge 60 commits into
base: master
Choose a base branch
from
Open

Protocol based on serde #2161

wants to merge 60 commits into from

Conversation

DmitryAstafyev
Copy link
Collaborator

Implementation of protocol for communication between rust and nodejs based on serde + bincode.

@DmitryAstafyev DmitryAstafyev marked this pull request as draft November 28, 2024 07:39
@DmitryAstafyev DmitryAstafyev force-pushed the binary_protocol branch 2 times, most recently from f8aa677 to e005959 Compare December 6, 2024 23:54
@DmitryAstafyev DmitryAstafyev marked this pull request as ready for review December 23, 2024 11:32
AmmarAbouZor
AmmarAbouZor requested changes Jan 2, 2025
View reviewed changes
Copy link
Member

@AmmarAbouZor AmmarAbouZor left a comment

Choose a reason for hiding this comment

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

This looks great 👏

I've found minor issues and added some suggestions.

I'll provide the needed changes on the build CLI tool once the other PR is merged and master is merged here.

There is another point about Macros Hygiene that can be adjusted (optionally since the macro are for internal use only). But if we want to make the macro hygiene we may need to use full paths on every type inside the macros up to $crate:: for crate types or ::std:: for rust standard library types

application/apps/indexer/stypes/Cargo.toml Show resolved Hide resolved
application/apps/indexer/stypes/Cargo.toml Outdated Show resolved Hide resolved
application/apps/indexer/stypes/bindings/dlt.ts Show resolved Hide resolved
application/apps/indexer/stypes/src/command/folders/extending.rs Outdated Show resolved Hide resolved
application/apps/indexer/stypes/src/operations/extending.rs Outdated Show resolved Hide resolved
application/apps/indexer/stypes/src/operations/extending.rs Outdated Show resolved Hide resolved
application/apps/indexer/stypes/src/progress/extending.rs Outdated Show resolved Hide resolved
application/apps/protocol/src/err.rs Show resolved Hide resolved
application/apps/protocol/test.sh
cd ../../rustcore/ts-bindings
rm -rf ./node_modules
rm -rf ./spec/build
rake bindings:test:protocol
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to replace this rake call with a function on the build CLI tool before merging or directly after that, to avoid the dependency on Ruby tooling

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, sure... will do it as soon as build cli will be updated

@DmitryAstafyev
Introduce stypes crate and related
0a23067
@DmitryAstafyev
Rearrange protocol wasm module
84dd041
@DmitryAstafyev
Update indexer code (switch to stypes)
1003a08
@DmitryAstafyev
Cleaun up clippy warnings
6de96d9
@DmitryAstafyev
Resolve clippy issues
a62c10e
@DmitryAstafyev
Rearrange modules in the session
5eb8f91
@DmitryAstafyev
Add feature "nodejs" to stypes crate
baf1de7
@DmitryAstafyev
Update rs-bindings with feature "nodejs" of protocol
172affb
@DmitryAstafyev
Get rid of JSON in the scope of SearchMapUpdate event
e107c9c
@DmitryAstafyev
Remove factory and config from base
61def86
@DmitryAstafyev
Add generic CommandOutput into protocol
e4063f5
@DmitryAstafyev
Update protocol wasm module
3c00fac
@DmitryAstafyev
Update wasm module
07dbc8a
@DmitryAstafyev
Add proptest strategies
0fc6dae
@DmitryAstafyev
Writing random message examples for testing
9cb0d4d
@DmitryAstafyev
Reading generated tests for protocol
65d0f78
@DmitryAstafyev
Test protocol on both sides
7d72842
@DmitryAstafyev
Update lock files after rebasing
f2edc33
@DmitryAstafyev
Add protocol test runner
ba11fb8
@DmitryAstafyev
Finalize crate stypes
2b60366
@DmitryAstafyev
Change types on nodejs/client levels
a2e2351
@DmitryAstafyev
Avoid BigInt usage on JS level
a1705cc
@DmitryAstafyev
Include bindings types into platform
e97dda0
@DmitryAstafyev
Resolve types conflicts (ts-binding to client/holder)
8ae7315
@DmitryAstafyev
Fix tests and avoid observe proxy obj
435bdd5
@DmitryAstafyev
Update protocol module docs
53225b7
@DmitryAstafyev
Update docs for tools/extend
cf7669c
@DmitryAstafyev
Replace some types on platform level with bindings
4764e17
@DmitryAstafyev
Update tests and types
e5799f7
@DmitryAstafyev
Update stype (added envvars related types)
8bcf6dc
@DmitryAstafyev
Update TS types
eb4e5e2
@DmitryAstafyev
Update types (Dlt stat info)
9aab989
@DmitryAstafyev
Update types on holder/client scope
f470257
@DmitryAstafyev
Update build scripts (rake + cli)
7d98827
@DmitryAstafyev
Add missed tests for recently added types
66c63c3
@DmitryAstafyev
Update documentation
70fc492
@DmitryAstafyev
Fix chart's issue (invalid map data)
420588e
@DmitryAstafyev
Prevent running stypes tests in regular way
0d40177
@DmitryAstafyev
Update CLI sys tests
bdbc729
@DmitryAstafyev
Build before run test in CI (temporary solution)
25f3a19
@DmitryAstafyev
Resolve review notes and rebase
6fc4a2a
AmmarAbouZor
AmmarAbouZor approved these changes Jan 3, 2025
View reviewed changes
Copy link
Member

@AmmarAbouZor AmmarAbouZor left a comment

Choose a reason for hiding this comment

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

This looks amazing, great job! 💯

I'll provide the needed implementation on the build CLI tool and add them to this PR ASAP

@AmmarAbouZor
Build CLI: Include Protocol in test dependencies
96d1c30 
* Adjust dependencies resolving to include protocol build on test jobs.
* Add unit test to ensure involved targets are built on bindings tests.
* Adjust integration tests accordingly.
@AmmarAbouZor
Copy link
Member

  • Changes on the Build CLI to include building protocol on test commands are implemented
  • Unit tests are extended to ensure needed build jobs are included on test commands
  • Integration tests are adjusted to ensure that protocol is built and cleaned properly
  • Temp fix in CI check is removed
  • Generated ts files are included as well since they will be rewritten on every build run and the automatic formatting and comments will be rewritten

CI is failing now because it still using the previous Tool version and not using the latest one. link
@itsmesamster Could you please have a look on it?

@AmmarAbouZor
CI: Remove temp fix
4d9775f 
Remove temp fix from GitHub action after fixing the main issue in
build CLI tool
@AmmarAbouZor
BinCode: Exclude Generating files on normal tests
6f05b22 
* Generate typescript files only on running tests with feature
  `test_and_gen` activated.
@AmmarAbouZor
BinCode: Ignore prop tests by default.
69f75b4 
The same feature `test_and_gen` is used for both generating types and
running prop tests. By marking prop tests as ignored we are able to
generate the files without running the extensive prop tests
@AmmarAbouZor
Copy link
Member

@DmitryAstafyev I've fixed generating typescript files on each test call by adding the feature test_and_gen to the conditional attributes, changing cfg_attr(test,...) to cfg_attr( all(test, feature = "test_and_gen"), on each type

Then I ran into an issue that the prop tests will be triggered each time we want to generate new types, so I marked them is ignored to be ran explicitly with the --ignored flag on cargo test. Alternatively, we can introduce another feature prop_tests and run the tests on it.

@AmmarAbouZor AmmarAbouZor mentioned this pull request Jan 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AmmarAbouZor AmmarAbouZor AmmarAbouZor approved these changes

@marcmo marcmo Awaiting requested review from marcmo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DmitryAstafyev @AmmarAbouZor