blockworks-foundation / mango-v3

Mango Markets V3 Smart Contract
MIT License
152 stars 91 forks source link

Load serum_dex dependency with a commit ref #173

Closed jkbpvsc closed 2 years ago

jkbpvsc commented 2 years ago

Problem

Recently the mango fork of serum_dex was merged with the upstream version 0.5.5. The main mango program is importing the serum_dex package with version 0.4.0 from a git repository.

If you delete Cargo.lock, the build will fail with

cargo build-bpf
Failed to obtain package metadata: `cargo metadata` exited with an error:     Updating git repository `https://github.com/blockworks-foundation/serum-dex.git`
error: failed to select a version for the requirement `serum_dex = "^0.4.0"`
candidate versions found which didn't match: 0.5.5
location searched: Git repository https://github.com/blockworks-foundation/serum-dex.git
required by package `mango v3.4.3 (/Users/jakob/Projects/crypto/blockworks-foundation/mango-v3/program)`

because while Cargo.toml specifies version 0.4.0, when importing from a git repo, the latest 0.5.5 from upstream is used.

Fix

Build with serum_dex 0.5.5 is breaking

error[E0063]: missing field `max_ts` in initializer of `NewOrderInstructionV3`
    --> program/src/instruction.rs:1621:10
     |
1621 |     Some(serum_dex::instruction::NewOrderInstructionV3 {
     |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `max_ts`

error[E0061]: this function takes 1 argument but 0 arguments were supplied
    --> program/src/state.rs:2302:11
     |
2302 |     state.check_flags()?;
     |           ^^^^^^^^^^^- supplied 0 arguments
     |           |
     |           expected 1 argument
     |
note: associated function defined here
    --> /Users/jakob/.cargo/git/checkouts/serum-dex-ac3ff2707f895b6e/7f55a5e/dex/src/state.rs:440:12
     |
440  |     pub fn check_flags(&self, allow_disabled: bool) -> DexResult {

so as a quick fix, I removed the version from the serum_dex and fixated it to the commit before the upstream merge.

dafyddd commented 2 years ago

I just merged fixes to main which makes 0.5.5 work well. We can do git ref as well if you redo the PR with new serum dex

jkbpvsc commented 2 years ago

Ok, I changed it, I think this is still preferable, as the version tags are irrelevant for crates imported from git, and just confuse Cargo. Also when you eventually update serum_dex again, we can avoid this situation.

dafyddd commented 2 years ago

@acamill Can you make it so UXD composability tests don't fail anymore?

acamill commented 2 years ago

@acamill Can you make it so UXD composability tests don't fail anymore?

Will look into it now

acamill commented 2 years ago

Secrets are not passed to workflows that are triggered by a pull request from a fork @dafyddd issue seems to be that secret won't work for fork PR.

End of may we will have another audit that will yield having the place_perp_order_v2 on mainnet (Still using some derported logic).

Once this is reached, the likelyness of breaking something will be very low, and changes to mangoMarketsV3 shouldn't be plentyfull either with V4 in the work.

I propose to remove the CI until we open source in the coming month, and in the meantime of update the program to use place_perp_order_v2, we can communicate on changes.

Opened #175