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

feat: add re exports from base used in public api #394

Merged
merged 5 commits into from
Jun 30, 2024

Conversation

mFragaBA
Copy link
Contributor

@mFragaBA mFragaBA commented Jun 24, 2024

closes #280

Tested with:

  • branch mFragaBA-workspace-refactor which refactors the single crate structure into a miden-cli, miden-client and tests subcrates. This made it easier to test the usage of the client library as a dependency of the integration tests. I added it as a dependency (and didn't add miden-objects nor miden-tx) and added missing re-exports if any until it compiled.
  • wasm fork of the miden-client.

I think testing against real use cases instead of just looking at the api will avoid some missing re-exports problems. We can still iterate on this as users of the crate can decide when they remove the dependency and just use the re-exports instead.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@mFragaBA mFragaBA marked this pull request as ready for review June 24, 2024 20:14
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I left some comments inline.

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you! I would move Felt, StarkField, and Word into the root, but other than that, we can merge.

@mFragaBA mFragaBA force-pushed the mFragaBA-add-re-exports-from-base-used-in-public-api branch 2 times, most recently from cd9f09d to 4945a84 Compare June 27, 2024 14:34
In order to test this, I replicated the changes from this commit into `mFragaBA-workspace-refactor`, and tried making the integration tests compile without using miden-objects nor miden-tx as a dependency.
added more re-exports based on what I saw being imported in the [wasm fork](https://github.com/demox-labs/miden-client/).
@mFragaBA mFragaBA force-pushed the mFragaBA-add-re-exports-from-base-used-in-public-api branch from 4945a84 to a16225c Compare June 28, 2024 20:48
Copy link
Collaborator

@igamigo igamigo left a comment

Choose a reason for hiding this comment

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

LGTM!

@bobbinth
Copy link
Contributor

I think this is the next PR that should be merged. And then #407 could follow shortly thereafter. Or, if it is ready we could merge #402 after this one and then merge #407.

@igamigo igamigo merged commit 1d9655b into next Jun 30, 2024
7 checks passed
@igamigo igamigo deleted the mFragaBA-add-re-exports-from-base-used-in-public-api branch June 30, 2024 13:06
bobbinth pushed a commit that referenced this pull request Jul 5, 2024
* refactor: add refactors from miden-objects and miden-tx

* feat: add miden base re-exports on library crate

In order to test this, I replicated the changes from this commit into `mFragaBA-workspace-refactor`, and tried making the integration tests compile without using miden-objects nor miden-tx as a dependency.

* feat: add more miden base re-exports on library crate

added more re-exports based on what I saw being imported in the [wasm fork](https://github.com/demox-labs/miden-client/).

* refactor: flatten re-exports

* address comments from review
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.

3 participants