Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds workspace-wide feature-powerset checks to CI and Makefile, centralizes Changes
Sequence Diagram(s)mermaid (Note: rectangles/boxes not required; flow shows primary interactions.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 108-111: The Makefile advertises and CI expects a
check-all-features target but it is missing; add a new phony alias target named
check-all-features that depends on the existing targets check-all-features-host
and check-all-features-wasm (so that running make check-all-features runs both
feature checks), and ensure .PHONY includes check-all-features; update the check
target to continue depending on check-all-features-host and
check-all-features-wasm (or on the new check-all-features alias) so CI and the
help text stop failing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ae3deee-2ae5-4e2e-b724-311ef9e1f2a3
📒 Files selected for processing (10)
.github/workflows/ci.ymlCargo.tomlMakefilecontracts/wallet/Cargo.tomlcrates/crypto/Cargo.tomlcrates/primitives/token-id/Cargo.tomlcrates/signatures/sep53/Cargo.tomlcrates/signatures/ton-connect/Cargo.tomlcrates/signatures/ton-connect/src/schema/mod.rscrates/testing/sandbox/src/extensions/defuse/tokens/mod.rs
Makefile
Outdated
|
|
||
| RUSTFLAGS_CHECK = -D warnings | ||
| # --cfg clippy: near-sdk compile_error!s on host unless one of its allowed cfgs is set | ||
| CARGO_CHECK_HOST = RUSTFLAGS='$(RUSTFLAGS_CHECK) --cfg clippy' cargo hack check --exclude-features contract --no-dev-deps |
There was a problem hiding this comment.
contract feature should be ok to be checked when near-sdk/unit-testing is enabled
There was a problem hiding this comment.
sure, but at the end of a day 2 configurations that we release are :
- host compiler +
abifeatures withoutcontractfeature - wasm compiler +
contractfeature
so using unit-testing feels hacky ....
wdyt
There was a problem hiding this comment.
Yes, but then it means that we don't run clippy on the code that we're unit-testing..
pityjllk
force-pushed
the
feat/cargo-check-features
branch
from
5511d95 to
3aba5f7
Compare
pityjllk
force-pushed
the
feat/cargo-check-features
branch
from
3aba5f7 to
0a02ec5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Makefile (1)
109-111:⚠️ Potential issue | 🔴 CriticalAdd missing
check-all-featurestarget to prevent CI failure.CI workflow (
.github/workflows/ci.ymlline 61) callsmake check-all-features, but this target doesn't exist. Line 88 advertises it in help, and line 109 declares.PHONY: checkbut notcheck-all-features. Runningmake check-all-featureswill fail with "No rule to make target".,
Proposed fix
-.PHONY: check check-all-features-host check-all-features-wasm check-examples +.PHONY: check check-all-features check-all-features-host check-all-features-wasm check-examples -check: check-all-features-host check-all-features-wasm check-examples +check-all-features: check-all-features-host check-all-features-wasm check-examples + +check: check-all-features🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 109 - 111, Add the missing Makefile target named check-all-features so the CI call succeeds: create a rule "check-all-features: check-all-features-host check-all-features-wasm check-examples" and add check-all-features to the .PHONY line (alongside check, check-all-features-host, check-all-features-wasm, check-examples) so running make check-all-features invokes the three sub-targets as intended.
🧹 Nitpick comments (1)
Makefile (1)
127-128: Missing blank line before.PHONY: fmt.For readability and consistency with the rest of the Makefile, add a blank line between the
check-all-features-wasmtarget and the.PHONY: fmtdeclaration.Proposed fix
$(foreach c,$(CRATES_AT_LEAST_ONE_VARIANT),\ $(CARGO_CHECK_WASM) -p $(call crate_name,$c) --feature-powerset --at-least-one-of $(call crate_features,$c) &&) true + .PHONY: fmt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 127 - 128, There is no blank line between the end of the check-all-features-wasm target (the recipe that ends with "$(CARGO_CHECK_WASM) -p $(call crate_name,$c) --feature-powerset --at-least-one-of $(call crate_features,$c) &&) true") and the `.PHONY: fmt` declaration; insert a single empty line between that target block and the `.PHONY: fmt` line so the Makefile matches the existing formatting and improves readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/signatures/sep53/Cargo.toml`:
- Line 9: The dependency entry in Cargo.toml for defuse-crypto includes an
unused "secp256k1" feature; edit the defuse-crypto line in
crates/signatures/sep53/Cargo.toml to remove "secp256k1" and leave only features
= ["ed25519"] (the sep53 crate's lib.rs imports Ed25519), then run cargo
check/build to verify compilation still succeeds.
In `@crates/signatures/ton-connect/src/schema/mod.rs`:
- Around line 29-30: Remove the consecutive blank lines in the top-level module
file mod.rs to satisfy the formatter; open
crates/signatures/ton-connect/src/schema/mod.rs and delete the extra empty lines
so there are no multiple consecutive blank lines between items (leave single
blank lines where appropriate).
---
Duplicate comments:
In `@Makefile`:
- Around line 109-111: Add the missing Makefile target named check-all-features
so the CI call succeeds: create a rule "check-all-features:
check-all-features-host check-all-features-wasm check-examples" and add
check-all-features to the .PHONY line (alongside check, check-all-features-host,
check-all-features-wasm, check-examples) so running make check-all-features
invokes the three sub-targets as intended.
---
Nitpick comments:
In `@Makefile`:
- Around line 127-128: There is no blank line between the end of the
check-all-features-wasm target (the recipe that ends with "$(CARGO_CHECK_WASM)
-p $(call crate_name,$c) --feature-powerset --at-least-one-of $(call
crate_features,$c) &&) true") and the `.PHONY: fmt` declaration; insert a single
empty line between that target block and the `.PHONY: fmt` line so the Makefile
matches the existing formatting and improves readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4c001f3-a258-440d-8354-6a14bffb1377
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
.github/workflows/ci.ymlCargo.tomlMakefilecontracts/wallet/Cargo.tomlcrates/crypto/Cargo.tomlcrates/near/nep245/src/checked.rscrates/signatures/sep53/Cargo.tomlcrates/signatures/ton-connect/Cargo.tomlcrates/signatures/ton-connect/src/schema/mod.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/ci.yml
- crates/crypto/Cargo.toml
- crates/signatures/ton-connect/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Makefile (1)
109-112:⚠️ Potential issue | 🟡 MinorAdd
check-all-featuresto.PHONY.Line 112 defines the aggregate target, but Line 109 still omits it from
.PHONY. If a file or directory namedcheck-all-featuresever appears,makecan treat the target as up to date and skip the checks.♻️ Proposed fix
-.PHONY: check check-all-features-host check-all-features-wasm check-examples +.PHONY: check check-all-features check-all-features-host check-all-features-wasm check-examples🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 109 - 112, The .PHONY line is missing the aggregate target check-all-features which can cause make to skip running it if a file/directory with that name exists; update the .PHONY declaration (currently listing check check-all-features-host check-all-features-wasm check-examples) to also include check-all-features so that the make target check-all-features is always treated as phony and always executed when invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 55-57: The "Run clippy for defuse" GitHub Actions step currently
runs `cargo clippy --manifest-path ./contracts/defuse/Cargo.toml
--features=contract --target wasm32-unknown-unknown` but doesn't promote
warnings to errors; update that step (the "Run clippy for defuse" workflow job)
to append the clippy flag to deny warnings by adding `-- -D warnings` to the
command so Clippy warnings fail CI.
---
Duplicate comments:
In `@Makefile`:
- Around line 109-112: The .PHONY line is missing the aggregate target
check-all-features which can cause make to skip running it if a file/directory
with that name exists; update the .PHONY declaration (currently listing check
check-all-features-host check-all-features-wasm check-examples) to also include
check-all-features so that the make target check-all-features is always treated
as phony and always executed when invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e68146fe-da5e-4491-8a25-d5bfad98ebe3
📒 Files selected for processing (2)
.github/workflows/ci.ymlMakefile
Makefile
Outdated
| @echo " clippy Run clippy lints" | ||
| @echo " check-all-features Check all feature combos with cargo-hack" |
There was a problem hiding this comment.
should we make it by default? i.e. check
There was a problem hiding this comment.
yep, only listed check in help
Makefile
Outdated
| $(foreach c,$(CRATES_AT_LEAST_ONE_VARIANT),\ | ||
| $(CARGO_CHECK_HOST) -p $(call crate_name,$c) --feature-powerset --at-least-one-of $(call crate_features,$c) &&) true |
There was a problem hiding this comment.
can we make them to appear as separate instructions instead of one-liner?
| $(foreach c,$(CRATES_AT_LEAST_ONE_VARIANT),\ | |
| $(CARGO_CHECK_HOST) -p $(call crate_name,$c) --feature-powerset --at-least-one-of $(call crate_features,$c) &&) true | |
| $(foreach c,$(CRATES_AT_LEAST_ONE_VARIANT),\ | |
| $(CARGO_CHECK_HOST) -p $(call crate_name,$c) --feature-powerset --at-least-one-of $(call crate_features,$c)) |
There was a problem hiding this comment.
made it work with TARGET::
make -n check
RUSTFLAGS='-D warnings --cfg clippy' cargo hack check --exclude-features contract --no-dev-deps --workspace --each-feature --exclude-no-default-features \
--exclude defuse-token-id --exclude defuse-ton-connect --exclude defuse-escrow-swap
RUSTFLAGS='-D warnings --cfg clippy' cargo hack check --exclude-features contract --no-dev-deps -p defuse-token-id --feature-powerset --at-least-one-of nep141,nep171,nep245,imt
RUSTFLAGS='-D warnings --cfg clippy' cargo hack check --exclude-features contract --no-dev-deps -p defuse-ton-connect --feature-powerset --at-least-one-of text,binary,cell
RUSTFLAGS='-D warnings --cfg clippy' cargo hack check --exclude-features contract --no-dev-deps -p defuse-escrow-swap --feature-powerset --at-least-one-of nep141,nep245
RUSTFLAGS='-D warnings' cargo hack check --target wasm32-unknown-unknown --exclude-features abi --exclude-features near-api-types --exclude-features near-api --no-dev-deps --workspace --each-feature --exclude-no-default-features \
--exclude defuse-token-id --exclude defuse-ton-connect --exclude defuse-escrow-swap \
--exclude defuse-test-utils --exclude defuse-sandbox --exclude defuse-randomness --exclude defuse-tests
RUSTFLAGS='-D warnings' cargo hack check --target wasm32-unknown-unknown --exclude-features abi --exclude-features near-api-types --exclude-features near-api --no-dev-deps -p defuse-token-id --feature-powerset --at-least-one-of nep141,nep171,nep245,imt
RUSTFLAGS='-D warnings' cargo hack check --target wasm32-unknown-unknown --exclude-features abi --exclude-features near-api-types --exclude-features near-api --no-dev-deps -p defuse-ton-connect --feature-powerset --at-least-one-of text,binary,cell
RUSTFLAGS='-D warnings' cargo hack check --target wasm32-unknown-unknown --exclude-features abi --exclude-features near-api-types --exclude-features near-api --no-dev-deps -p defuse-escrow-swap --feature-powerset --at-least-one-of nep141,nep245
RUSTFLAGS='-D warnings' cargo clippy --workspace --examples
There was a problem hiding this comment.
Nice! But now the question is: since we're not using clippy anymore but just faking it via --cfg clippy, does it mean that all clippy::* warnings are suspended now?
There was a problem hiding this comment.
yes it just satisfies the near-sdk and is general enough so work for both serde-sdk dependant and not dependant crate, i will move to clippy in one of the following prs, fo
Makefile
Outdated
| # Crates where every enum variant is feature-gated, requiring at least one | ||
| # variant feature. These need --feature-powerset + --at-least-one-of instead | ||
| # of --each-feature (which tests features in isolation). | ||
| # Format: crate=feature1,feature2,... | ||
| CRATES_AT_LEAST_ONE_VARIANT := \ | ||
| defuse-token-id=nep141,nep171,nep245,imt \ | ||
| defuse-ton-connect=text,binary,cell \ | ||
| defuse-escrow-swap=nep141,nep245 | ||
|
|
||
| # Testing crates that cannot compile for wasm32-unknown-unknown. | ||
| # defuse-randomness uses rand/getrandom which lacks wasm32 support; | ||
| # it only reaches the defuse contract via dev-dependencies, never in the WASM binary. | ||
| CRATES_HOST_ONLY := \ | ||
| defuse-test-utils \ | ||
| defuse-sandbox \ | ||
| defuse-randomness \ | ||
| defuse-tests |
There was a problem hiding this comment.
in general, what is the advantage of it compared to matrix of features in CI? Similar to how it's done in near-sdk repo
https://github.com/near/near-sdk-rs/blob/e484fd308b37ab1a08114ac9b820d71ff79c188a/.github/workflows/test.yml#L24-L27
There was a problem hiding this comment.
it checks combination of all possible features for each crate separately.
info: --no-dev-deps modifies real `Cargo.toml` while cargo-hack is running and restores it when finished
info: running `cargo check --no-default-features --features abi,arbitrary,binary,cell,text` on defuse-ton-connect (1/32)
info: running `cargo check --no-default-features --features binary` on defuse-ton-connect (2/32)
info: running `cargo check --no-default-features --features abi,binary` on defuse-ton-connect (3/32)
info: running `cargo check --no-default-features --features arbitrary,binary` on defuse-ton-connect (4/32)
info: running `cargo check --no-default-features --features abi,arbitrary,binary` on defuse-ton-connect (5/32)
info: running `cargo check --no-default-features --features cell` on defuse-ton-connect (6/32)
info: running `cargo check --no-default-features --features abi,cell` on defuse-ton-connect (7/32)
info: running `cargo check --no-default-features --features arbitrary,cell` on defuse-ton-connect (8/32)
info: running `cargo check --no-default-features --features abi,arbitrary,cell` on defuse-ton-connect (9/32)
info: running `cargo check --no-default-features --features binary,cell` on defuse-ton-connect (10/32)
info: running `cargo check --no-default-features --features abi,binary,cell` on defuse-ton-connect (11/32)
info: running `cargo check --no-default-features --features arbitrary,binary,cell` on defuse-ton-connect (12/32)
info: running `cargo check --no-default-features --features abi,arbitrary,binary,cell` on defuse-ton-connect (13/32)
info: running `cargo check --no-default-features --features default` on defuse-ton-connect (14/32)
info: running `cargo check --no-default-features --features abi,default` on defuse-ton-connect (15/32)
info: running `cargo check --no-default-features --features arbitrary,default` on defuse-ton-connect (16/32)
info: running `cargo check --no-default-features --features abi,arbitrary,default` on defuse-ton-connect (17/32)
info: running `cargo check --no-default-features --features text` on defuse-ton-connect (18/32)
info: running `cargo check --no-default-features --features abi,text` on defuse-ton-connect (19/32)
info: running `cargo check --no-default-features --features arbitrary,text` on defuse-ton-connect (20/32)
info: running `cargo check --no-default-features --features abi,arbitrary,text` on defuse-ton-connect (21/32)
info: running `cargo check --no-default-features --features binary,text` on defuse-ton-connect (22/32)
info: running `cargo check --no-default-features --features abi,binary,text` on defuse-ton-connect (23/32)
info: running `cargo check --no-default-features --features arbitrary,binary,text` on defuse-ton-connect (24/32)
info: running `cargo check --no-default-features --features abi,arbitrary,binary,text` on defuse-ton-connect (25/32)
info: running `cargo check --no-default-features --features cell,text` on defuse-ton-connect (26/32)
info: running `cargo check --no-default-features --features abi,cell,text` on defuse-ton-connect (27/32)
info: running `cargo check --no-default-features --features arbitrary,cell,text` on defuse-ton-connect (28/32)
info: running `cargo check --no-default-features --features abi,arbitrary,cell,text` on defuse-ton-connect (29/32)
info: running `cargo check --no-default-features --features binary,cell,text` on defuse-ton-connect (30/32)
info: running `cargo check --no-default-features --features abi,binary,cell,text` on defuse-ton-connect (31/32)
info: running `cargo check --no-default-features --features arbitrary,binary,cell,text` on defuse-ton-connect (32/32)
info: --no-dev-deps modifies real `Cargo.toml` while cargo-hack is running and restores it when finished
There was a problem hiding this comment.
I tried it, very cool!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 113-133: The check-all-features target can run
check-all-features-host and check-all-features-wasm in parallel causing
cargo-hack races; add a .NOTPARALLEL rule for check-all-features to serialize
execution (i.e., declare ".NOTPARALLEL: check-all-features") so the commands
that use CARGO_CHECK_HOST and CARGO_CHECK_WASM (which rewrite Cargo.toml) never
run concurrently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: badb781c-2ab4-4e24-aaaa-0aa79d9e7456
📒 Files selected for processing (4)
Makefilecrates/signatures/sep53/Cargo.tomlcrates/signatures/ton-connect/Cargo.tomlcrates/signatures/ton-connect/src/schema/mod.rs
✅ Files skipped from review due to trivial changes (2)
- crates/signatures/sep53/Cargo.toml
- crates/signatures/ton-connect/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/signatures/ton-connect/src/schema/mod.rs
Makefile
Outdated
| check-all-features: check-all-features-host check-all-features-wasm | ||
|
|
||
| check-examples: | ||
| RUSTFLAGS='$(RUSTFLAGS_CHECK)' cargo clippy --workspace --examples | ||
|
|
||
| check-all-features-host:: | ||
| $(CARGO_CHECK_HOST) --workspace --each-feature --exclude-no-default-features \ | ||
| $(foreach c,$(CRATES_AT_LEAST_ONE_VARIANT),--exclude $(call crate_name,$c)) | ||
|
|
||
| $(foreach c,$(CRATES_AT_LEAST_ONE_VARIANT),\ | ||
| $(eval check-all-features-host::; \ | ||
| $(CARGO_CHECK_HOST) -p $(call crate_name,$c) --feature-powerset --at-least-one-of $(call crate_features,$c))) | ||
|
|
||
| check-all-features-wasm:: | ||
| $(CARGO_CHECK_WASM) --workspace --each-feature --exclude-no-default-features \ | ||
| $(foreach c,$(CRATES_AT_LEAST_ONE_VARIANT),--exclude $(call crate_name,$c)) \ | ||
| $(addprefix --exclude ,$(CRATES_HOST_ONLY)) | ||
|
|
||
| $(foreach c,$(CRATES_AT_LEAST_ONE_VARIANT),\ | ||
| $(eval check-all-features-wasm::; \ | ||
| $(CARGO_CHECK_WASM) -p $(call crate_name,$c) --feature-powerset --at-least-one-of $(call crate_features,$c))) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Serialization guards:"
rg -n '^\.(NOTPARALLEL|WAIT)\b' Makefile || true
echo
echo "Aggregate feature-check target:"
sed -n '113,133p' Makefile
echo
echo "Manifest-mutating cargo-hack invocations:"
rg -n --fixed-strings '--no-dev-deps' MakefileRepository: near/intents
Length of output: 1562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Serialization guards in Makefile ==="
grep -n '^\.(NOTPARALLEL\|WAIT)\b' Makefile || echo "No serialization guards found"
echo
echo "=== Checking for cargo-hack in Makefile ==="
grep -n 'cargo.hack\|--no-dev-deps\|--feature-powerset' Makefile || echo "No direct cargo-hack invocations found"
echo
echo "=== Variable definitions for CARGO_CHECK_HOST and CARGO_CHECK_WASM ==="
grep -n 'CARGO_CHECK_HOST\|CARGO_CHECK_WASM' Makefile | head -20
echo
echo "=== Full Makefile content to understand variable usage ==="
wc -l MakefileRepository: near/intents
Length of output: 1687
Guard check-all-features against parallel cargo-hack runs.
Under make -j, the check-all-features target (line 113) schedules both check-all-features-host and check-all-features-wasm in parallel. Both use cargo hack check --no-dev-deps (defined in CARGO_CHECK_HOST and CARGO_CHECK_WASM on lines 30–31), which temporarily rewrites Cargo.toml. This can cause nondeterministic failures. Serialize this target by adding .NOTPARALLEL: check-all-features.
Minimal fix
+.NOTPARALLEL: check-all-features
check-all-features: check-all-features-host check-all-features-wasm🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 113 - 133, The check-all-features target can run
check-all-features-host and check-all-features-wasm in parallel causing
cargo-hack races; add a .NOTPARALLEL rule for check-all-features to serialize
execution (i.e., declare ".NOTPARALLEL: check-all-features") so the commands
that use CARGO_CHECK_HOST and CARGO_CHECK_WASM (which rewrite Cargo.toml) never
run concurrently.
Makefile
Outdated
| # Crates where every enum variant is feature-gated, requiring at least one | ||
| # variant feature. These need --feature-powerset + --at-least-one-of instead | ||
| # of --each-feature (which tests features in isolation). | ||
| # Format: crate=feature1,feature2,... | ||
| CRATES_AT_LEAST_ONE_VARIANT := \ | ||
| defuse-token-id=nep141,nep171,nep245,imt \ | ||
| defuse-ton-connect=text,binary,cell \ | ||
| defuse-escrow-swap=nep141,nep245 | ||
|
|
||
| # Testing crates that cannot compile for wasm32-unknown-unknown. | ||
| # defuse-randomness uses rand/getrandom which lacks wasm32 support; | ||
| # it only reaches the defuse contract via dev-dependencies, never in the WASM binary. | ||
| CRATES_HOST_ONLY := \ | ||
| defuse-test-utils \ | ||
| defuse-sandbox \ | ||
| defuse-randomness \ | ||
| defuse-tests |
There was a problem hiding this comment.
I tried it, very cool!
Makefile
Outdated
| CARGO_CHECK_HOST = RUSTFLAGS='$(RUSTFLAGS_CHECK) --cfg clippy' cargo hack check --exclude-features contract --no-dev-deps | ||
| CARGO_CHECK_WASM = RUSTFLAGS='$(RUSTFLAGS_CHECK)' cargo hack check --target wasm32-unknown-unknown --exclude-features abi --exclude-features near-api-types --exclude-features near-api --no-dev-deps |
There was a problem hiding this comment.
- Let's remove
--no-dev-deps, as it results in longer runs in IDE due to messing around with rust-analyzer - What prevents us from using
cargo hack clippyalready?
| CARGO_CHECK_HOST = RUSTFLAGS='$(RUSTFLAGS_CHECK) --cfg clippy' cargo hack check --exclude-features contract --no-dev-deps | |
| CARGO_CHECK_WASM = RUSTFLAGS='$(RUSTFLAGS_CHECK)' cargo hack check --target wasm32-unknown-unknown --exclude-features abi --exclude-features near-api-types --exclude-features near-api --no-dev-deps | |
| CARGO_CHECK_HOST = RUSTFLAGS='$(RUSTFLAGS_CHECK) --cfg clippy' cargo hack clippy --exclude-features contract | |
| CARGO_CHECK_WASM = RUSTFLAGS='$(RUSTFLAGS_CHECK)' cargo hack clippy --target wasm32-unknown-unknown --exclude-features abi --exclude-features near-api-types --exclude-features near-api |
There was a problem hiding this comment.
What prevents us from using cargo hack clippy already?
nothing just didnt wanted stack up multiple things in single pr, i think its better to keep prs minimalistis, as they are easy to review and maintatin 👍 the purpose of this pr is to verify that crates that define particular features are able to be compile with different features combinations (we did not have such checks before)
There was a problem hiding this comment.
Can you please move all the crappy stuff to the end of the file, while keeping minimalistic and understandable targets on top?
Makefile
Outdated
|
|
||
| RUSTFLAGS_CHECK = -D warnings | ||
| # --cfg clippy: near-sdk compile_error!s on host unless one of its allowed cfgs is set | ||
| CARGO_CHECK_HOST = RUSTFLAGS='$(RUSTFLAGS_CHECK) --cfg clippy' cargo hack check --exclude-features contract --no-dev-deps |
There was a problem hiding this comment.
Yes, but then it means that we don't run clippy on the code that we're unit-testing..
Makefile
Outdated
|
|
||
| .PHONY: check check-all-features-host check-all-features-wasm check-examples | ||
|
|
||
| check: check-all-features check-examples |
There was a problem hiding this comment.
Maybe add clippy target as a pre-requisite here as well? So I can run all of these simply by make check
There was a problem hiding this comment.
i would rather fix clippy findings in separate pr
this pr only adds new checks, does not remove nor modify any of the existing ones, lets improve things incrementally |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
95-96: Orphaned.PHONYdeclaration.The
.PHONY: fmtdeclaration on line 95 is isolated from its target definition on line 140. Consider moving it adjacent to thefmttarget for clarity, or grouping all.PHONYdeclarations together.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 95 - 96, Move the orphaned .PHONY declaration so it's adjacent to the fmt target (or group it with other .PHONY lines): locate the existing ".PHONY: fmt" and the "fmt" target in the Makefile and either relocate the .PHONY line directly above the fmt target definition or consolidate it with the file's other .PHONY declarations to keep declarations and targets together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Around line 95-96: Move the orphaned .PHONY declaration so it's adjacent to
the fmt target (or group it with other .PHONY lines): locate the existing
".PHONY: fmt" and the "fmt" target in the Makefile and either relocate the
.PHONY line directly above the fmt target definition or consolidate it with the
file's other .PHONY declarations to keep declarations and targets together.
3 participants
Summary
We use cargo-hack to verify that every feature in the workspace compiles correctly in isolation — both on the host and for wasm32-unknown-unknown. This catches issues like missing feature
gates, unused imports behind cfg flags, and broken feature combinations that normal cargo check misses because it only tests default features.
The check runs individual cargo check invocations across the workspace, covering:
Changes
ton-connect/schema/mod.rs
Summary by CodeRabbit
Chores
Refactor