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

book: build sdk example #1123

Merged
merged 2 commits into from
Dec 24, 2024
Merged

Conversation

alv-around
Copy link
Contributor

Last week, I corrected some outdated documentation on the sdk usage (#1120). However, the two underlying problems were not address, which are:

  1. There are currently no checkguards done on the sdk interface.

  2. The code snippets in the documentation are hard-coded, which can lead to inconsistencies between the documentation and the actual code (as happened in book: sdk usage correction #1120)

In this PR, I tackle both problem by:

a. created a bin crate (sdk.rs) in the examples folder. This way, the compiler will run it's check on the code, and if anything if the interface changes this will trigger a compiler error on the example. This has also the advantage that the code is never run, against running expensive e2e tests.

I also rename the guest-example directory from example to guest to avoid any confusion with the generic cargo directory examples.

b. I linked in book the code snippets to the compiled code in a, to ensure that the code snippets are automatically updated so no extra effort is incurred in updating the docs.

@alv-around alv-around reopened this Dec 23, 2024
@jonathanpwang jonathanpwang self-requested a review December 23, 2024 16:06
Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

LGTM! The idea to use snippets is really nice and an overall improvement.

Now that you've added the bin, should we change the target_path etc to the guest package so that if we wanted to, cargo run --example sdk would actually run?

I'll approve once I generate the docs locally to double check.

Copy link
Contributor

@jonathanpwang jonathanpwang left a comment

Choose a reason for hiding this comment

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

LGTM

@jonathanpwang
Copy link
Contributor

Please fix lint

@alv-around
Copy link
Contributor Author

Please fix lint

Sorry, that went completely under my radar. Is now fixed :)

@alv-around
Copy link
Contributor Author

Now that you've added the bin, should we change the target_path etc to the guest package so that if we wanted to, cargo run --example sdk would actually run?

I purposely wrote examples/sdk.rs to not run out the box, because the binary executes very expensive operations. This way, I wanted to discourage accidental triggers of this binary.

I could have shadowed the variable target_path after line 59:
https://github.com/alv-around/openvm/blob/6ef5dab4ed7bdfdb5d8e1a268dac2f05b72e57e2/crates/sdk/examples/sdk.rs#L59
With:
https://github.com/alv-around/openvm/blob/6ef5dab4ed7bdfdb5d8e1a268dac2f05b72e57e2/crates/sdk/examples/sdk.rs#L47-L51

This way it would run out of the box, however this would have made the imports in the book a bit more convoluted. And I am not sure whether that make things better.

I think like this we have get the best of both worlds:

  1. we tell the compiler: "this is how we want the users to use our product, please run your checks". But at the same time we don't run any expensive computation.
  2. we export in a simple manner and straight-forward manner the documentation.

But am happy to change anything you consider most appropriate :)

@jonathanpwang
Copy link
Contributor

^This makes sense. I think we can make a separate example repo with a runnable version (and without the heavier computation part) to complement.

@jonathanpwang jonathanpwang changed the title Build sdk example book: build sdk example Dec 24, 2024
@jonathanpwang jonathanpwang merged commit 74fac04 into openvm-org:main Dec 24, 2024
4 checks passed
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