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

Add notary client crate #489

Merged
merged 15 commits into from
Jun 18, 2024
Merged

Add notary client crate #489

merged 15 commits into from
Jun 18, 2024

Conversation

yuroitaki
Copy link
Member

@yuroitaki yuroitaki commented May 24, 2024

Addressed #443, where

  • Examples and notary server integration test now use this notary client crate
  • Added a test in notary integration test to test auth module
  • Some unused dependencies are removed

Another PR will follow to migrate this notary client crate to the notary server folder (the migration touches many parts of the codebase (e.g. github action, devops deployment scripts, notary server dockerfile), hence a separate PR is better to reduce the review scope)

Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

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

I've not reviewed exhaustively, but we can start with this discussion

tlsn/tlsn-notary-client/src/lib.rs Show resolved Hide resolved
tlsn/tlsn-notary-client/src/error.rs Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Show resolved Hide resolved
tlsn/examples/twitter/twitter_dm.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
@yuroitaki yuroitaki requested a review from sinui0 May 30, 2024 03:47
Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

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

Getting there!

tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/examples/twitter/twitter_dm.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Show resolved Hide resolved
@yuroitaki yuroitaki requested a review from sinui0 June 5, 2024 10:54
@themighty1
Copy link
Member

@yuroitaki, after you guys finalize all the details, pls ping me for a review, thanks.

Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

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

Looking good! A few more nits

tlsn/tlsn-notary-client/Cargo.toml Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
@yuroitaki yuroitaki requested a review from sinui0 June 12, 2024 04:01
tlsn/tlsn-notary-client/src/error.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
@yuroitaki yuroitaki requested a review from sinui0 June 12, 2024 11:41
Copy link
Member

@sinui0 sinui0 left a comment

Choose a reason for hiding this comment

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

Gw, just about there!

tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Show resolved Hide resolved
Copy link
Member

@themighty1 themighty1 left a comment

Choose a reason for hiding this comment

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

good work! I fixed the method name comments Create->Creates but I didn't fix all the instances of a missing final period that we put at the end of both doc comments and regular comments since there were too many of them.

i think Ideally we should add punctuation at least to the new code that we add (regardless if the surrounding code in unpunctuated).

notary-server/tests/integration_test.rs Outdated Show resolved Hide resolved
tlsn/examples/discord/discord_dm.rs Outdated Show resolved Hide resolved
tlsn/examples/src/lib.rs Outdated Show resolved Hide resolved
tlsn/examples/twitter/twitter_dm.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/client.rs Outdated Show resolved Hide resolved
tlsn/tlsn-notary-client/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@themighty1 themighty1 left a comment

Choose a reason for hiding this comment

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

all looks good, thanks.

@yuroitaki yuroitaki merged commit bff2a53 into dev Jun 18, 2024
15 checks passed
@yuroitaki yuroitaki deleted the feature/notary-client branch June 19, 2024 09:30
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.

None yet

3 participants