-
Notifications
You must be signed in to change notification settings - Fork 0
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
Fix cyclic dependency between test-utils
and hyperdrive-math
crates
#92
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
47c6ed5
to
fe6d296
Compare
hyperdrive-math can depend on test-utils, but test-utils shouldn't depend on hyperdrive-math.
adcbf9f
to
3eb9046
Compare
test-utils
and hyperdrive-math
crates
moves all the methods for Agent that require hyeprdrive-math things to hyperdrive-math/test_utils/agent.rs
3eb9046
to
3c49c8c
Compare
3c49c8c
to
fb64273
Compare
in order to use the cfg(test) code, this needs to be internal to the crate
fb64273
to
582ebc5
Compare
need access to the members, might as well make them public
ryangoree
approved these changes
May 13, 2024
dpaiton
reviewed
May 13, 2024
dpaiton
reviewed
May 13, 2024
dpaiton
reviewed
May 13, 2024
dpaiton
reviewed
May 13, 2024
dpaiton
reviewed
May 13, 2024
dpaiton
approved these changes
May 13, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor confusion/questions but in general this is a great improvement! Thanks
d2bdc5b
to
c2edaf5
Compare
6 tasks
ryangoree
added a commit
that referenced
this pull request
May 17, 2024
# Resolved Issues Related to #92 # Description This refactors the crates to remove all cyclic dependencies. Below is an illustration of the dependency tree before and after with a few notes: ![hyperdrive-rs-dep-trees](https://github.com/delvtech/hyperdrive-rs/assets/3289505/1ac060aa-58c9-4694-bf49-3ceff5aa2547) # Review Checklists Please check each item **before approving** the pull request. While going through the checklist, it is recommended to leave comments on items that are referenced in the checklist to make sure that they are reviewed. - [ ] **Testing** - [ ] Are there new or updated unit or integration tests? - [ ] Do the tests cover the happy paths? - [ ] Do the tests cover the unhappy paths? - [ ] Are there an adequate number of fuzz tests to ensure that we are covering the full input space? - [ ] If matching Solidity behavior, are there differential fuzz tests that ensure that Rust matches Solidity?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
hyperdrive-math was depending on test-utils and vice versa. The culprit is the
Agent
struct intest_utils
that required a lot of things inhyperdrive-math
for theAgent
methods.hyperdrive-math
then imported the Agent to run tests.This PR removes the cyclic dependency by extending the
Agent
struct inhyperdrive-math
with it's ownHyperdriveMathAgent
in test_utils/agent.rs. The agent module is#[cfg(test)]
decorated so it only compiles for tests and isn't included in the release binary.One caveat to this approach though is that other crates will not be able to import the HyperdriveMathAgent, including the tests/ directory, so I had to move tests/integration_tests.rs to src/test_utils/integration_tests.rs
Review Checklists
Please check each item before approving the pull request. While going
through the checklist, it is recommended to leave comments on items that are
referenced in the checklist to make sure that they are reviewed.
covering the full input space?
ensure that Rust matches Solidity?