-
-
Notifications
You must be signed in to change notification settings - Fork 105
fix: add contract feature to freenet-stdlib dependency for ping app #2030
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
Conversation
…rtially connected network test ## Why Issue #2022 reported that partially connected network tests were failing with "channel closed" errors during startup. Investigation revealed the root cause was a missing `contract` feature preventing contract compilation. ## What Changed 1. **Added `contract` feature to freenet-stdlib dependency** in `apps/freenet-ping/types/Cargo.toml`: - Enables `freenet_stdlib::time` module needed for contract execution - Required when building without `std` feature (WASM contracts) - This was the primary blocker preventing tests from running 2. **Fixed contract loading in run_app.rs**: - Changed from `std::fs::read()` to `common::load_contract()` - Ensures contract is compiled at test time (consistent with other tests) 3. **Removed `#[ignore]` annotation** from single-gateway test: - `apps/freenet-ping/app/tests/run_app_partially_connected_network.rs` now runs 4. **Documented remaining issue with 3-gateway variant**: - Multi-gateway test in `run_app.rs` remains flaky in CI - Gateways timeout during initialization with 3+ gateways - Kept ignored with updated description referencing working single-gateway variant - This is a separate issue (#2024 or #2025) related to multi-gateway coordination ## Test Results - ✅ Single-gateway test (1 gateway, 7 nodes) in `run_app_partially_connected_network.rs`: **PASSING** -⚠️ Multi-gateway test (3 gateways, 7 nodes) in `run_app.rs`: Remains flaky, kept ignored Full test suite passes with no regressions. The core issue blocking these tests (contract compilation) is fixed. ## Root Cause Analysis The primary issue was missing `contract` feature causing `freenet_stdlib::time::now()` to be unavailable during contract compilation. Tests never reached node startup phase. The multi-gateway variant has additional timing/coordination issues that require deeper investigation into gateway initialization and peer discovery with multiple gateways. This is documented separately. Partially addresses #2022 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ed network tests ## Why Issue #2022 reported that partially connected network tests were failing with "channel closed" errors during startup. Investigation revealed the root cause was a missing `contract` feature preventing contract compilation. Additionally, the multi-gateway test needed more initialization time in CI. ## What Changed 1. **Added `contract` feature to freenet-stdlib dependency** in `apps/freenet-ping/types/Cargo.toml`: - Enables `freenet_stdlib::time` module needed for contract execution - Required when building without `std` feature (WASM contracts) - This was the primary blocker preventing tests from running 2. **Fixed contract loading in run_app.rs**: - Changed from `std::fs::read()` to `common::load_contract()` - Ensures contract is compiled at test time (consistent with other tests) 3. **Removed `#[ignore]` annotations** from both test variants: - `apps/freenet-ping/app/tests/run_app_partially_connected_network.rs` (1 gateway) - `apps/freenet-ping/app/tests/run_app.rs` (3 gateways) 4. **Increased gateway initialization delay** in run_app.rs: - Changed from 2 seconds to 10 seconds - Multiple gateways need more time to coordinate in CI environment - Addresses "deadline has elapsed" timeouts during gateway startup ## Test Results Both test variants now pass locally: - Single-gateway test (1 gateway, 7 nodes): ✓ - Multi-gateway test (3 gateways, 7 nodes): ✓ ## Root Cause Analysis The primary issue was missing `contract` feature causing `freenet_stdlib::time::now()` to be unavailable during contract compilation. Tests never reached node startup phase where they could run. The secondary issue was insufficient initialization time for multiple gateways to coordinate in resource-constrained CI environments. Closes #2022 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…rtially connected network test ## Why Issue #2022 reported that partially connected network tests were failing with "channel closed" errors during startup. Investigation revealed the root cause was a missing `contract` feature preventing contract compilation. ## What Changed 1. **Added `contract` feature to freenet-stdlib dependency** in `apps/freenet-ping/types/Cargo.toml`: - Enables `freenet_stdlib::time` module needed for contract execution - Required when building without `std` feature (WASM contracts) - This was the primary blocker preventing tests from running 2. **Fixed contract loading in run_app.rs**: - Changed from `std::fs::read()` to `common::load_contract()` - Ensures contract is compiled at test time (consistent with other tests) 3. **Removed `#[ignore]` annotation from single-gateway test**: - `apps/freenet-ping/app/tests/run_app_partially_connected_network.rs` now runs in CI 4. **Documented multi-gateway test issue**: - Multi-gateway test in `run_app.rs` remains ignored (see #2029) - Gateways crash during startup with "channel closed" in CI - Test passes locally but fails in CI due to gateway coordination issues - Requires deeper investigation into multi-gateway initialization ## Test Results - ✅ Single-gateway test (1 gateway, 7 nodes) in `run_app_partially_connected_network.rs`: **PASSING** -⚠️ Multi-gateway test (3 gateways, 7 nodes) in `run_app.rs`: Remains ignored (see #2029) Full test suite passes with no regressions. ## Root Cause Analysis The primary issue was missing `contract` feature causing `freenet_stdlib::time::now()` to be unavailable during contract compilation. Tests never reached node startup phase where they could run. The multi-gateway variant has a separate CI-specific issue with gateway coordination during initialization, tracked in #2029. Partially addresses #2022 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
## Why The ping app's contract failed to compile because the `contract` feature was not enabled on the freenet-stdlib dependency. This feature provides `freenet_stdlib::time::now()` which is required by the ping contract. This was preventing: - Contract compilation at test time - Local testing of partially connected network functionality - Investigation of test failures described in #2022 ## What Changed **Added `contract` feature** to freenet-stdlib dependency in `apps/freenet-ping/types/Cargo.toml`: ```toml -freenet-stdlib = { workspace = true } +freenet-stdlib = { workspace = true, features = ["contract"] } ``` ## Impact - ✅ Ping contract now compiles successfully - ✅ Enables local testing and development - ✅ Unblocks investigation of #2022 test failures - ℹ️ Tests remain ignored pending resolution of CI-specific flakiness ## Note on Test Status While this fix enables the contract to compile, the partially connected network tests remain ignored due to CI-specific flakiness discovered during investigation. These tests pass locally but fail in CI with timing-related issues (#2022, #2029). The dependency fix is valuable on its own as it unblocks local testing and development. Related to #2022 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
| clap = { version = "4", features = ["derive"], optional = true } | ||
|
|
||
| freenet-stdlib = { workspace = true } | ||
| freenet-stdlib = { workspace = true, features = ["contract"] } |
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.
mmmm this depends upon who is compiling, so ti should be feature gated.
e.g. types is imported both in the app and in the contract itself, the first shouldn't not enable "contract" feature, the 2nd should do it.
|
@iduartgomez Good catch on the inline comment! You're absolutely right - the feature should be gated. Looking at the code, #[cfg(feature = "std")]
let now = Utc::now();
#[cfg(not(feature = "std"))]
let now = freenet_stdlib::time::now();So the types library only needs The proper fix should be: Option 1: Add a # types/Cargo.toml
[features]
default = ["std"]
std = ["chrono/default"]
contract = ["freenet-stdlib/contract"] # <-- add thisThen the contract crate enables it: # contracts/ping/Cargo.toml
freenet-ping-types = { path = "../../types", default-features = false, features = ["contract"] }Option 2: Check if PR #2031 (which modifies the test compilation pipeline) already handles this properly. Looking at PR #2031, it adds Since #2031 handles this more comprehensively (fixes the handshake timeout issue + contract compilation), should I close this PR in favor of #2031, or would you prefer we implement Option 1 here? [AI-assisted debugging and comment] |
|
If this was only added for that PR then yes, close this. |
|
@iduartgomez Agreed - closing this PR. PR #2031 handles the contract feature properly via the build tooling ( Additionally, #2031 fixes the actual runtime issue (handshake timeout causing gateway crashes) that was the root cause of the CI failures. This PR was useful for identifying the missing contract feature, but #2031 provides a more comprehensive and properly-architected solution. [AI-assisted debugging and comment] |
fix: add contract feature to freenet-stdlib dependency for ping app
Why
The ping app's contract failed to compile because the
contractfeaturewas not enabled on the freenet-stdlib dependency. This feature provides
freenet_stdlib::time::now()which is required by the ping contract.This was preventing:
What Changed
Added
contractfeature to freenet-stdlib dependency inapps/freenet-ping/types/Cargo.toml:Impact
Note on Test Status
While this fix enables the contract to compile, the partially connected
network tests remain ignored due to CI-specific flakiness discovered during
investigation. These tests pass locally but fail in CI with timing-related
issues (#2022, #2029).
The dependency fix is valuable on its own as it unblocks local testing
and development.
Related to #2022
🤖 Generated with Claude Code
Co-Authored-By: Claude [email protected]