-
Notifications
You must be signed in to change notification settings - Fork 4
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 integration tests and prop tests for Osmosis #95
Conversation
9bfb8c6
to
188632d
Compare
21596be
to
188632d
Compare
d310ac1
to
d829594
Compare
9142611
to
4c6307c
Compare
4c6307c
to
e325ec4
Compare
Signed-off-by: Bobby <[email protected]>
Signed-off-by: Bobby <[email protected]>
Signed-off-by: Bobby <[email protected]>
The advisory RUSTSEC-2020-0071 (alias RUSTSEC-2020-26235) does not apply to the `time` dependency in `chrono`, which is a dependency of `osmosis-std`. After some investigation it seems the advisory can safely be ignored, but care must be taken that it is not ignored for any other crates, should it appear anywhere else. Signed-off-by: Bobby <[email protected]>
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.
I have added an ignore rule for RUSTSEC-2020-0071, which was incorrectly causing cargo deny
to report a vulnerability for the chrono
dependency in osmosis-std
. Please see my latest commit message for some more details.
We must take care to not ignore this in the future if it resurfaces.
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.
Nice work! Approving, but see comments. Could be nice to clarify some stuff.
cw-storage-plus = "0.15.1" | ||
cw2 = "0.15.1" |
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.
Let's update to >= 1.0
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.
ack
cw-utils = "0.16" | ||
cw20 = "0.16" |
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.
why not using workspace version?
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.
ack
) { | ||
let (funds, _) = separate_natives_and_cw20s(&assets); | ||
|
||
// TODO: Increase allowance for cw20 assets |
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.
Todo comment for when we do astroport tests or? Lets not leave TODO comments, instead mention it in the relevant issue.
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.
cw-asset = { workspace = true } | ||
osmosis-testing = { workspace = true } | ||
osmosis-std = { workspace = true } | ||
apollo-utils = { git = "https://github.com/apollodao/apollo-utils.git", rev = "bfd1abd8cd9716dccad3e74aeb3704cad9f1f41a" } |
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.
why no workspace dep here?
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.
it depends on a specific version to have the same version of cw-asset but cw-dex uses a function from a later commit. #118
@@ -0,0 +1,31 @@ | |||
folder: "./tests/configs" |
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.
Should we remove configs folder until we add support for testcontainers?
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.
ack
@@ -39,6 +26,30 @@ jobs: | |||
uses: actions-rs/cargo@v1 | |||
with: | |||
command: test | |||
args: --locked --all-features | |||
args: --locked --lib |
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.
Why remove --all-features? Are there no unit tests in feature gated modules we want to test?
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.
ack
Ok(()) | ||
} | ||
|
||
// TODO: For some reason it fails when swap amount is 1. Need to investigate |
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.
Either fix i this PR or open an issue and remove comment.
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.
) | ||
} | ||
|
||
fn test_multi_pool_provide_liquidity( |
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.
Add doc comment
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.
Why called multi_pool? What is multi?
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.
ack
ask_idx | ||
}; | ||
|
||
// Offer amount cannot be larger than the pool liquidity |
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.
Why not? Getting a weird error? :p
Had the same issue in my tests, but on testnet it works fine...
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.
Maybe clarify the comment for those who aren't aware of this bug(?).
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.
yeah getting some error from osmosis
// Setting both fork and timeout is redundant since timeout implies | ||
// fork, but both are shown for clarity. |
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.
Outdated comment? Don't see mention of "timeout" nor "fork" anywhere.
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.
ack
Addressed comments and opened a new PR containing only the osmosis tests here #116 Lets merge that in first since it is the most important and then rebase the astroport tests later |
cargo make wasm
Closes #114