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

refactor: library api reorganization #367

Merged
merged 106 commits into from
Jun 17, 2024
Merged

Conversation

mFragaBA
Copy link
Contributor

@mFragaBA mFragaBA commented May 28, 2024

closes #362.
closes #284.

I tried to have each commit as self contained as possible:

  • b84c845 refactors the re-exports for the library crate to have a more flattened structure (the code structure stays as is, only exports change)
  • 9251fb1 makes StoreConfig an associated type of Store. This allows the consumers of the library to re-use the ClientConfig struct when providing their own store implementation. Not only that but we expect it to satisfy a few trait bounds (usually they can be derived) such as Debug, Default, Eq, PartialEq, Serialize, DeserializeOwned.
  • 9587e23 and dde95bb adds features to enable the sqlite store and the tonic rpc client respectively
  • 5eebbc5 is pretty self descriptive. The reasoning is that concurrent implies std in base so we should add it here as well to be explicit about it.
  • 503a1c9 we remove the feature flags uuid and test_utils. This comes at a bit of a cost. Unless we're on unit tests of the client, we can't use anything under #[cfg(test)] because in those cases (like the CLI and integration tests) the client gets compiled but not in test mode. I couldn't find a built in way to do so and the workaround is what we had with test_utils which is to define a custom feature flag, but the problem is that it cannot be hidden. In order to remove uuid and test_utils I also had to remove some tests from src/cli/..., but considering we're adding very similar tests on feat: add CLI integration tests using assert_cmd #353 it's not a big deal.
  • In f87000b we wanted to stop exporting CliConfig from miden_client. The way to do so was by inverting the dependency between it and the ClientConfig struct. So we go from "A ClientConfig can have a CliConfig defined" to "All CliConfig has a ClientConfig defined"
  • Lastly, in ff56c79 was something mentioned in comments where we could stop exporting rusqlite and rusqlite_migration as features by prepending them with a dep: on Cargo.toml

Discussion

  • I'm thinking that in order to split dependencies that are CLI only we'll probably need to change this into a workspace and have a separate crate for the CLI. That way we can, for example only require figment for the CLI. I think that's a second refactor we could leave for a separate PR, but we can tackle it here.

@mFragaBA mFragaBA force-pushed the mFragaBA-library-api-reorganization branch 4 times, most recently from 90bc912 to 40b6cf4 Compare May 29, 2024 21:00
@mFragaBA mFragaBA marked this pull request as ready for review May 29, 2024 22:05
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@mFragaBA mFragaBA force-pushed the mFragaBA-library-api-reorganization branch from 82bbe7f to 6c323cb Compare May 30, 2024 16:26
@tomyrd
Copy link
Collaborator

tomyrd commented May 31, 2024

The changes in miden_tx broke some of the imports here, I already commited some fixes to the parent branch so we only have to merge them.

@mFragaBA mFragaBA force-pushed the mFragaBA-library-api-reorganization branch from 6c323cb to ff56c79 Compare May 31, 2024 21:40
@mFragaBA
Copy link
Contributor Author

The changes in miden_tx broke some of the imports here, I already commited some fixes to the parent branch so we only have to merge them.

done! Rebased and pushed

@mFragaBA mFragaBA force-pushed the mFragaBA-next-0.4 branch from 1f96117 to 52d1ac7 Compare May 31, 2024 21:49
@tomyrd tomyrd force-pushed the mFragaBA-next-0.4 branch from 52d1ac7 to 1f96117 Compare May 31, 2024 21:50
mFragaBA and others added 8 commits May 31, 2024 18:51
The fixes can be separated into 2:

- compilation errors (most of them were because the old `Account::new` became `Account::from_parts` and the addition of `OutputNote::Partial` which needed to be handled in some pattern matchings.
- start storing the partial output notes (currently being discarded), this also came with small refactors.
@mFragaBA mFragaBA force-pushed the mFragaBA-next-0.4 branch from 1f96117 to 5a4091c Compare May 31, 2024 21:54
@mFragaBA mFragaBA force-pushed the mFragaBA-library-api-reorganization branch from 8e2e258 to 95c1f97 Compare May 31, 2024 21:57
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. Also, a few comments about the errors module:

  • IdPrefixFetchError seems like CLI-specific error. I think we should remove it from the public library interface.
  • InvalidNoteInputsError seems like an internal-only error which doesn't appear in the public interface. If so, we should hide it (e.g., use pub(crate)).
  • Should we rename NodeRpcClientError into just RpcError? I think having words Node and Client there is making things more confusing.
  • We should probably rename ScreenerError into NoteScreenerError.

In general, I'm wondering if we need this module at all or maybe we should just move (or just re-export) error types into relevant modules (e.g., RPC error would be in the rpc module).

src/store/mod.rs Outdated Show resolved Hide resolved
src/config.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
Cargo.toml Outdated Show resolved Hide resolved
@mFragaBA
Copy link
Contributor Author

mFragaBA commented Jun 3, 2024

  • InvalidNoteInputsError seems like an internal-only error which doesn't appear in the public interface. If so, we should hide it (e.g., use pub(crate)).

While it does not appear in the public interface directly, it is used in NoteScreenerError which is part of the public api. Making InvalidNoteInputsError results in the following warning:

 1  warning: type `InvalidNoteInputsError` is more private than the item `NoteScreenerE
 rror::InvalidNoteInputsError::0`
    --> src/errors.rs:398:28
     |
 398 |     InvalidNoteInputsError(InvalidNoteInputsError),
     |                            ^^^^^^^^^^^^^^^^^^^^^^ field `NoteScreenerError::Inva
 lidNoteInputsError::0` is reachable at visibility `pub`
     |
 note: but type `InvalidNoteInputsError` is only usable at visibility `pub(crate)`
    --> src/errors.rs:428:1
     |
 428 | pub(crate) enum InvalidNoteInputsError {
     | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = note: `#[warn(private_interfaces)]` on by default

@bobbinth
Copy link
Contributor

bobbinth commented Jun 3, 2024

While it does not appear in the public interface directly, it is used in NoteScreenerError which is part of the public api. Making InvalidNoteInputsError results in the following warning:

Ah - I see! Let's keep it as is then.

mFragaBA added 4 commits June 3, 2024 15:54
This allows implementors of other kinds of store to specify their own configuration, provided they implement `Default` and serialization/deserialization traits.
Base automatically changed from mFragaBA-next-0.4 to next June 14, 2024 21:37
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml 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 left a few more comments/questions inline (in addition to the ones above). But overall, I think this is good to be merged.

src/main.rs Outdated Show resolved Hide resolved
tests/README.md Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile 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 left one small comment regarding the config file - but other than that, I think this PR can be merged.

@igamigo igamigo merged commit afd0844 into next Jun 17, 2024
7 checks passed
@igamigo igamigo deleted the mFragaBA-library-api-reorganization branch June 17, 2024 21:01
@igamigo igamigo mentioned this pull request Jun 17, 2024
bobbinth added a commit that referenced this pull request Jul 5, 2024
* feat: point to miden base's next and fix errors

The fixes can be separated into 2:

- compilation errors (most of them were because the old `Account::new` became `Account::from_parts` and the addition of `OutputNote::Partial` which needed to be handled in some pattern matchings.
- start storing the partial output notes (currently being discarded), this also came with small refactors.

* fix: point node to custom branch on integration tests also

* fix: filter out partial notes

* fix: only store in scripts table if output note record has a script

* fix: add new storage type to node config file from #365

* fix: pull from custom branch instead of main

* Update the `TransactionAuthenticator` imports

* Remove duplicate `get_falcon_signature`

* fix: update note script root hashes

* refactor: flatten refactors

* refactor: make `StoreConfig` an associated type of the `Store` trait.

This allows implementors of other kinds of store to specify their own configuration, provided they implement `Default` and serialization/deserialization traits.

* refactor: put sqlite store behind feature flag

* refactor: put tonic rpc client behind a feature flag

* refactor: make concurrent feature imply std

* refactor: remove tests in cli module files

- we remove some tests that got duplicated during the refactor of the CLI
- the remaining tests will be moved into the integration tests added in #353

* refactor: invert dependency between ClientConfig and CliConfig and move CliConfig to CLI binary.

* refactor: stop exposing optional dependencies as features

* fix: fix compilation errors after rebase

* fix CHANGELOG

* address some review comments

* refactor: rename error types

* refactor: remove old client config struct

* feat: point to miden base's next and fix errors

The fixes can be separated into 2:

- compilation errors (most of them were because the old `Account::new` became `Account::from_parts` and the addition of `OutputNote::Partial` which needed to be handled in some pattern matchings.
- start storing the partial output notes (currently being discarded), this also came with small refactors.

* fix: point node to custom branch on integration tests also

* fix: filter out partial notes

* fix: only store in scripts table if output note record has a script

* fix: add new storage type to node config file from #365

* fix: pull from custom branch instead of main

* Update the `TransactionAuthenticator` imports

* Remove duplicate `get_falcon_signature`

* fix: update note script root hashes

* Use `&mut rng` for note creation in transactions

* Fix usage of new `InputNote`

* fix: fix after rebase

* feat: point to miden base's next and fix errors

The fixes can be separated into 2:

- compilation errors (most of them were because the old `Account::new` became `Account::from_parts` and the addition of `OutputNote::Partial` which needed to be handled in some pattern matchings.
- start storing the partial output notes (currently being discarded), this also came with small refactors.

* fix: filter out partial notes

* fix: only store in scripts table if output note record has a script

* fix: add new storage type to node config file from #365

* Update the `TransactionAuthenticator` imports

* Remove duplicate `get_falcon_signature`

* fix: update note script root hashes

* Use `&mut rng` for note creation in transactions

* Fix usage of new `InputNote`

* fix: fix after rebase

* cargo: point to node's next branch

* Change node ref

* test: avoid reruns on genesis cli tests

test will fail on retry anyways since the genesis account stays is the same every time.

* add looser sleep times to ensure blocks are included

* test: add helper to wait until notes get committed/consumed

* test: add helper to wait until notes get committed/consumed

* deps: point to miden-node's next branch and fix compilation errors

* Test transaction ordering

* make: remove dependency for node to avoid duplication on CI

* Rollback node branch

* fix: fix breaking change from base

* fix: update swap note script root

* feat: point to miden base's next and fix errors

The fixes can be separated into 2:

- compilation errors (most of them were because the old `Account::new` became `Account::from_parts` and the addition of `OutputNote::Partial` which needed to be handled in some pattern matchings.
- start storing the partial output notes (currently being discarded), this also came with small refactors.

* fix: filter out partial notes

* fix: only store in scripts table if output note record has a script

* fix: add new storage type to node config file from #365

* Update the `TransactionAuthenticator` imports

* Remove duplicate `get_falcon_signature`

* fix: update note script root hashes

* Use `&mut rng` for note creation in transactions

* Fix usage of new `InputNote`

* fix: fix after rebase

* cargo: point to node's next branch

* Change node ref

* test: avoid reruns on genesis cli tests

test will fail on retry anyways since the genesis account stays is the same every time.

* add looser sleep times to ensure blocks are included

* test: add helper to wait until notes get committed/consumed

* deps: point to miden-node's next branch and fix compilation errors

* Test transaction ordering

* Rollback node branch

* make: remove dependency for node to avoid duplication on CI

* fix: fix breaking change from base

* fix: update swap note script root

* fix: add new aux field to updated note creation functions from base

* chore: increment crate version to v0.3.1

* Handle aux parameter

* MASM fix

* Correct script root

* Update CHANGELOG.md for WASM changes

* Aux param on custom note

* Remove wasm feature in favor of being compatible by default

* Clean up testing targets

* Clean up lint targets

* Fix typo

* Undo async call in CLI

* Update Cargo.toml

* Undo async call in CLI

* Cargo fmt

* Remove getrandom

* Re-add getrandom, but under asyhnc

* Reviews

---------

Co-authored-by: tomyrd <[email protected]>
Co-authored-by: Ignacio Amigo <[email protected]>
Co-authored-by: Bobbin Threadbare <[email protected]>
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.

4 participants