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

chore(toolchain): upgrade to 1.72 nightly #2149

Merged
merged 12 commits into from
Jun 27, 2024
Merged

Conversation

onur-ozkan
Copy link
Member

@onur-ozkan onur-ozkan commented Jun 25, 2024

This should greatly help for people working on solana-sdk upgrade and the stable toolchain migration work.

Should be easy to review commit-by-commit.

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

Bless you 👼, LGTM!

I only skimmed over clippy and fmt changes. I guess we don't really have a choice with these but to follow them until another toolchain upgrade.

mm2src/mm2_main/src/database/stats_nodes.rs Outdated Show resolved Hide resolved
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Comment on lines +75 to +76
opt-level = 1 # TODO: MIR fails on optimizing this dependency, remove that..
Copy link
Collaborator

Choose a reason for hiding this comment

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

mocktopus should be a dev only dependency, but since TestCoin is used outside coins crate I will use for-tests feature instead. Will push a commit that fixes that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed here db8aedc

In the future, we should move TestCoin to mm2_test_helpers or another test only crate that we import as a dev only dependency.

Copy link
Collaborator

@shamardy shamardy Jun 26, 2024

Choose a reason for hiding this comment

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

I will revert the last commit for now, I thought everything was fine because clippy for all targets and features worked locally, same for tests but for CI jobs it's different. It will need a bigger refactor.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO we should just get rid of that outdated/dead dependency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW I made mocktopus optional in my PR #2112 too.
But I had to add it also as dev-dependencies as some tests in mm2_main rely on mocks in coins: f3625e5 (otherwise mocks did not do anything)

Copy link
Collaborator

@shamardy shamardy Jun 27, 2024

Choose a reason for hiding this comment

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

BTW I made mocktopus optional in my PR #2112 too.
But I had to add it also as dev-dependencies as some tests in mm2_main rely on mocks in coins: f3625e5 (otherwise mocks did not do anything)

Thanks @dimxy for this note, I guess we can merge this PR with [profile.release.package.mocktopus] and you can remove it in your PR if you can to not duplicate the effort.

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

Thank you for the update! Only one comment from my side.

mm2src/adex_cli/Cargo.lock Outdated Show resolved Hide resolved
Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan onur-ozkan force-pushed the update-nightly-toolchain branch 2 times, most recently from 7864eaf to ece8b26 Compare June 26, 2024 04:34
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Comment on lines 1066 to 1070
debug!("total_available: {}", available);
#[allow(clippy::redundant_clone)] // This is a false-possitive bug from clippy
let min_tx_amount = qtum_min_tx_amount.clone();
let expected_max_taker_vol =
max_taker_vol_from_available(MmNumber::from(available), "QTUM", "MYCOIN", &min_tx_amount)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
debug!("total_available: {}", available);
#[allow(clippy::redundant_clone)] // This is a false-possitive bug from clippy
let min_tx_amount = qtum_min_tx_amount.clone();
let expected_max_taker_vol =
max_taker_vol_from_available(MmNumber::from(available), "QTUM", "MYCOIN", &min_tx_amount)
debug!("total_available: {}", available);
let expected_max_taker_vol =
max_taker_vol_from_available(MmNumber::from(available), "QTUM", "MYCOIN", &qtum_min_tx_amount)
.expect("max_taker_vol_from_available");

Copy link
Member Author

Choose a reason for hiding this comment

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

I kind of liked the way we explicitly defined the minimum transaction amount. The change in suggestion is not really clear at first glance (to figure out the min tx amount, we need to look at max_taker_vol_from_available and check what the last argument stands for). Considering that this is just a test function, I am not sure if this change is worth applying. These are my thoughts, if you still think we should apply this change, please feel completely free to do that!

@laruh
Copy link
Member

laruh commented Jun 27, 2024

Thanks for the toolchain upgrade. Now wasm-bindgen-cli does not conflict with the toolchain version on macOS.

Note: wasm-pack downloads wasm-bindgen-cli automatically when you run the build command, as wasm-pack itself does not have some binaries for macOS.

Could you please write for OSX users (Apple Silicon): instead of for OSX users (M1):, so we don't need to repeat all new M chip versions

https://github.com/KomodoPlatform/komodo-defi-framework/blob/update-nightly-toolchain/docs/WASM_BUILD.md?plain=1#L28

https://github.com/KomodoPlatform/komodo-defi-framework/blob/update-nightly-toolchain/docs/DEV_ENVIRONMENT.md?plain=1#L64

Signed-off-by: onur-ozkan <work@onurozkan.dev>
@onur-ozkan
Copy link
Member Author

Thanks for the toolchain upgrade. Now wasm-bindgen-cli does not conflict with the toolchain version on macOS.

Note: wasm-pack downloads wasm-bindgen-cli automatically when you run the build command, as wasm-pack itself does not have some binaries for macOS.

Could you please write for OSX users (Apple Silicon): instead of for OSX users (M1):, so we don't need to repeat all new M chip versions

https://github.com/KomodoPlatform/komodo-defi-framework/blob/update-nightly-toolchain/docs/WASM_BUILD.md?plain=1#L28

https://github.com/KomodoPlatform/komodo-defi-framework/blob/update-nightly-toolchain/docs/DEV_ENVIRONMENT.md?plain=1#L64

Done 058afa0

Copy link
Collaborator

@shamardy shamardy left a comment

Choose a reason for hiding this comment

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

🔥

@shamardy shamardy merged commit 6db5b9f into dev Jun 27, 2024
22 of 25 checks passed
@shamardy shamardy deleted the update-nightly-toolchain branch June 27, 2024 21:24
dimxy added a commit that referenced this pull request Jul 3, 2024
* dev:
  fix(restrictions): remove wallet-only restriction from max_maker_vol (#2153)
  feat(tendermint): support unsigned txs for ledger's keplr extension (#2148)
  chore(toolchain): upgrade to 1.72 nightly (#2149)
  feat(solana-swap): solana swap protocol v1 POC (#2091)
  fix(docs): update cargo test command (#2144)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request under review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants