test(rs-sdk): replace print-based DPNS tests with assertions, add identity error paths#3131
test(rs-sdk): replace print-based DPNS tests with assertions, add identity error paths#3131thepastaclaw wants to merge 1 commit intodashpay:v3.1-devfrom
Conversation
thepastaclaw
requested review from
QuantumExplorer and
shumkov
as code owners
📝 WalkthroughWalkthroughRefactored tests to use assertions, added identity-not-found/balance-not-found tests and constants, suppressed dead-code warnings on two SDK parse helpers, changed FFI address provider vtable to raw pointers with helper accessors, added iOS static-lib resolver and tests, and introduced multi-tool symbol validation for the Swift build. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 2
🧹 Nitpick comments (1)
packages/rs-sdk/tests/dpns_queries_test.rs (1)
84-113: Optional: extract shared SDK setup into a helper to remove duplication.Both test functions build the SDK with identical configuration (same
TrustedHttpContextProvider, same address, same network). If a third integration test is added later, the boilerplate will be copy-pasted a third time.♻️ Suggested helper (outside both test functions)
+ async fn build_testnet_sdk() -> dash_sdk::Sdk { + use rs_sdk_trusted_context_provider::TrustedHttpContextProvider; + use std::num::NonZeroUsize; + const TESTNET_DAPI_ADDRESS: &str = "https://52.12.176.90:1443"; + + let context_provider = TrustedHttpContextProvider::new( + Network::Testnet, + None, + NonZeroUsize::new(100).unwrap(), + ) + .expect("Failed to create context provider"); + + let address_list = TESTNET_DAPI_ADDRESS + .parse() + .expect("Failed to parse address"); + SdkBuilder::new(address_list) + .with_network(Network::Testnet) + .with_context_provider(context_provider) + .build() + .expect("Failed to create SDK") + }Then replace the repeated setup blocks in both tests with
let sdk = build_testnet_sdk().await;.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/tests/dpns_queries_test.rs` around lines 84 - 113, Extract the repeated SDK setup into a single async helper function (e.g., build_testnet_sdk) that constructs the TrustedHttpContextProvider, parses the testnet DAPI address constant, and returns the built Sdk via SdkBuilder; then replace the duplicated setup inside test_dpns_search_variations (and the other test) with a call like let sdk = build_testnet_sdk().await; reference the existing types/functions TrustedHttpContextProvider, NonZeroUsize::new, SdkBuilder::new and the DAPI address string when implementing the helper so it mirrors the current configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk/tests/dpns_unit_tests.rs`:
- Around line 61-67: The assert_eq! invocation testing is_contested_username
uses old-style positional format args which triggers
clippy::uninlined_format_args; update the assertion to use Rust's inline capture
format syntax (e.g., "is_contested_username({name}) should be {expected}")
instead of passing name and expected as separate format arguments so the message
is consistent with other assertions in the file and clippy no longer warns.
In `@packages/rs-sdk/tests/fetch/identity.rs`:
- Around line 108-109: The assertion asserting revision >= 1 couples the test to
fixture state; either remove the revision assertion (keep only the fetch success
already enforced by the prior .expect() and the balance check) or, if you truly
intend to verify the fixture was previously updated, replace the assertion with
an explicit precondition comment and ensure the fixture
(cfg.existing_identity_id) is documented/initialized to an updated identity;
locate the variables revision and balance in tests/fetch/identity.rs and
update/remove the assert!(revision >= 1, ...) accordingly.
---
Nitpick comments:
In `@packages/rs-sdk/tests/dpns_queries_test.rs`:
- Around line 84-113: Extract the repeated SDK setup into a single async helper
function (e.g., build_testnet_sdk) that constructs the
TrustedHttpContextProvider, parses the testnet DAPI address constant, and
returns the built Sdk via SdkBuilder; then replace the duplicated setup inside
test_dpns_search_variations (and the other test) with a call like let sdk =
build_testnet_sdk().await; reference the existing types/functions
TrustedHttpContextProvider, NonZeroUsize::new, SdkBuilder::new and the DAPI
address string when implementing the helper so it mirrors the current
configuration.
4862c3a to
83620ef
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/rs-sdk/src/sdk.rs (1)
251-263: Consider documenting the intent of the#[allow(dead_code)]suppressions or removing the unused wrappers.
parse_proofandparse_proof_with_metadataare confirmed to be dead code—they arepub(crate)convenience wrappers that only delegate toparse_proof_with_metadata_and_proofand are never called externally. While silencing the lint is valid if these are placeholders for future use, a one-line comment such as// TODO: future API wrapperwould clarify intent and avoid confusion for future readers. Alternatively, if these methods are not needed in the near term, removing them is cleaner than a blanket lint suppression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/src/sdk.rs` around lines 251 - 263, The #[allow(dead_code)] on the pub(crate) wrapper functions parse_proof and parse_proof_with_metadata hides the fact they are unused; either add a one-line comment explaining they are intentional placeholders (e.g., "// TODO: future API wrapper for parse_proof_with_metadata_and_proof") or remove the unused wrappers entirely; locate the methods parse_proof and parse_proof_with_metadata in the file and either annotate them with a brief explanatory comment or delete both and update any callers to use parse_proof_with_metadata_and_proof directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/rs-sdk/src/sdk.rs`:
- Around line 251-263: The #[allow(dead_code)] on the pub(crate) wrapper
functions parse_proof and parse_proof_with_metadata hides the fact they are
unused; either add a one-line comment explaining they are intentional
placeholders (e.g., "// TODO: future API wrapper for
parse_proof_with_metadata_and_proof") or remove the unused wrappers entirely;
locate the methods parse_proof and parse_proof_with_metadata in the file and
either annotate them with a brief explanatory comment or delete both and update
any callers to use parse_proof_with_metadata_and_proof directly.
|
Applied rustfmt fix for the failing |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
packages/rs-sdk-ffi/src/address_sync/provider.rs (1)
64-71: ConsiderOption<unsafe extern "C" fn(...)>instead of*const c_voidfor optional callbacks.Rust guarantees that
Option<unsafe extern "C" fn(...)>has the same ABI as a nullable C function pointer (null =None). Using it here would:
- Eliminate the
transmutecalls in the helper methods andmod.rsdestruction path.- Provide compile-time type safety — callers can't accidentally assign a wrong-signature function pointer.
- Remain
#[repr(C)]-compatible with no ABI change for Swift/C consumers.♻️ Suggested change
/// Check if there are still pending addresses /// If null, the default implementation (pending_addresses is non-empty) is used - pub has_pending: *const c_void, + pub has_pending: Option<HasPendingFn>, /// Get the highest found index /// If null, returns None - pub highest_found_index: *const c_void, + pub highest_found_index: Option<GetHighestFoundIndexFn>, /// Optional destructor for cleanup - pub destroy: *const c_void, + pub destroy: Option<DestroyProviderFn>,This would simplify the helpers to direct field reads (e.g.,
(*self.ffi.vtable).has_pending) and the destroy path inmod.rsto a simpleif let Some(destroy) = vtable.destroy { destroy(provider.context); }.Test initialization would use
Noneinstead ofstd::ptr::null().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/src/address_sync/provider.rs` around lines 64 - 71, Change the three nullable callback fields (has_pending, highest_found_index, destroy) in the provider vtable from raw pointers to typed nullable function pointers using Option<unsafe extern "C" fn(...)> with the correct signatures; update any helpers that transmute these fields (e.g., those reading (*self.ffi.vtable).has_pending and highest_found_index) to read the field directly and call it via if let Some(cb) = vtable.has_pending { cb(args) }, and simplify the destruction path in mod.rs to if let Some(destroy) = vtable.destroy { destroy(provider.context) }; also update tests/initializers to use None instead of std::ptr::null() and adjust any call sites to match the new function-pointer types for compile-time safety.packages/rs-sdk/tests/fetch/identity.rs (1)
227-232: Consider renamingUNIQUE_KEY_HASH— the new comments make the contradiction more visible.The new doc comment explicitly states the constant is "Used to test fetching identities by non-unique public key hash," yet the constant is named
UNIQUE_KEY_HASH. The comments were added to clarify intent, but they inadvertently highlight the misleading name.♻️ Proposed rename
- /// Key hash generated during test data creation in `create_genesis_state`. - /// Used to test fetching identities by non-unique public key hash. - /// Source: packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/mod.rs - const UNIQUE_KEY_HASH: [u8; 20] = [ + /// Key hash generated during test data creation in `create_genesis_state`. + /// Used to test fetching identities by non-unique public key hash (shared across multiple identities). + /// Source: packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/mod.rs + const NON_UNIQUE_KEY_HASH: [u8; 20] = [(Also update the three uses of
UNIQUE_KEY_HASHbelow within the same function.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk/tests/fetch/identity.rs` around lines 227 - 232, Rename the misleading constant UNIQUE_KEY_HASH to a name that reflects it is a non-unique test key (for example NON_UNIQUE_KEY_HASH or DUPLICATE_KEY_HASH) and update all references to it in the same test function (there are three usages mentioned) so the constant name and comments align; ensure the constant identifier is changed where declared and where referenced in tests to avoid any compile errors.packages/swift-sdk/scripts/symbol_validation.sh (2)
85-85: ShellCheck SC2034 false positives — these globals are intentional caller-facing outputs.
FOUND_SYMBOL_TOOLandLAST_NM_STDERR_HINTare documented as caller-visible globals (lines 58-60) and are consumed by bothbuild_ios.shand the test harness. Consider adding# shellcheck disable=SC2034on the assignment lines to suppress the warnings explicitly.Also applies to: 93-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/scripts/symbol_validation.sh` at line 85, Add explicit ShellCheck suppression comments for the intentional caller-facing globals to silence SC2034 false positives: next to the assignment to FOUND_SYMBOL_TOOL and the assignment to LAST_NM_STDERR_HINT in symbol_validation.sh, add a preceding comment "# shellcheck disable=SC2034" so ShellCheck knows these variables are intentionally exported/consumed by build_ios.sh and the test harness; keep the comment immediately above each assignment to make the suppression local and clear.
84-84: Substring match risk in symbol lookup.
contains_fixed_in_file "$prefixed_symbol" "$out_file"uses a fixed-string search, which will match_dash_spv_ffi_config_add_peerinside_dash_spv_ffi_config_add_peers_from_list(hypothetical longer symbol). In practice, nm output is space-delimited so this is low risk, but a word-boundary-aware match (e.g.,grep -wForrg -wF) would be more precise.♻️ Suggested tighter matching
if [[ "$SYMBOL_SEARCH_TOOL" == "rg" ]]; then - rg -F -- "$needle" "$file" >/dev/null 2>&1 + rg -wF -- "$needle" "$file" >/dev/null 2>&1 else - grep -F -- "$needle" "$file" >/dev/null 2>&1 + grep -wF -- "$needle" "$file" >/dev/null 2>&1 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/scripts/symbol_validation.sh` at line 84, The current check in contains_fixed_in_file called with prefixed_symbol and bare_symbol can match substrings (e.g., matching _dash_spv_ffi_config_add_peer inside a longer _dash_spv_ffi_config_add_peers_from_list); update contains_fixed_in_file (or its callers in symbol_validation.sh where prefixed_symbol and bare_symbol are checked against out_file) to perform word-boundary-aware fixed-string matches (for example use grep -wF or rg -wF) so the lookup only matches whole symbols rather than substrings; modify the implementation of contains_fixed_in_file to accept and use the word-boundary flag or replace the call sites to invoke a word-boundary-aware search utility.packages/swift-sdk/build_ios.sh (1)
44-71: Hardcodedios-arm64-simulatorpath assumes Apple Silicon.
SIM_LIB_DIRis set to$DEST_XCFRAMEWORK_DIR/ios-arm64-simulator. On Intel Macs (x86_64), the xcframework slice directory is typically namedios-x86_64-simulator(orios-arm64_x86_64-simulatorfor a fat simulator slice). If this build is ever run on an Intel CI runner or contributor machine, the directory won't exist and the script will fail at line 47 with a somewhat misleading error.Consider auto-detecting the simulator slice:
♻️ Suggested auto-detection
-SIM_LIB_DIR="$DEST_XCFRAMEWORK_DIR/ios-arm64-simulator" -if [[ ! -d "$SIM_LIB_DIR" ]]; then - echo "❌ Missing simulator library directory at $SIM_LIB_DIR" - exit 1 -fi +SIM_LIB_DIR="" +for candidate in "$DEST_XCFRAMEWORK_DIR"/ios-*-simulator; do + if [[ -d "$candidate" ]]; then + SIM_LIB_DIR="$candidate" + break + fi +done +if [[ -z "$SIM_LIB_DIR" ]]; then + echo "❌ No simulator library directory found under $DEST_XCFRAMEWORK_DIR" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/swift-sdk/build_ios.sh` around lines 44 - 71, The script currently hardcodes SIM_LIB_DIR="$DEST_XCFRAMEWORK_DIR/ios-arm64-simulator", which fails on Intel macs; change it to auto-detect the simulator slice by checking a prioritized list of possible directories (e.g., "ios-arm64-simulator", "ios-x86_64-simulator", "ios-arm64_x86_64-simulator", etc.) under DEST_XCFRAMEWORK_DIR and set SIM_LIB_DIR to the first existing one; update the error path check (the block referencing SIM_LIB_DIR and the message "Missing simulator library directory") to print which candidates were attempted if none exist and then exit; ensure the rest of the script still uses SIM_LIB_DIR, SIM_LIBS, and SEEN_LIB unchanged so library discovery logic remains intact.packages/rs-sdk-ffi/tests/build_ios_resolver_test.sh (2)
42-72: No test coverage for the "library not found" failure pathAll four cases verify that
resolve_static_lib_pathreturns the correct path when the file exists. There is no case that verifies the exit code is1and diagnostics are emitted when no candidate file is present. This leaves theprint_resolve_attempts+exit 1branch of--resolve-static-libuntested.♻️ Suggested additional test case
# Case 5: not found – expect exit code 1 assert_not_found() { local test_name="$1" local target="$2" local exit_code env -u CARGO_TARGET_DIR "$BUILD_SCRIPT" --resolve-static-lib "$LIB_NAME" "$target" "$CRATE_DIR" "$WORKSPACE_DIR" \ >/dev/null 2>&1 && exit_code=0 || exit_code=$? if [ "$exit_code" -ne 1 ]; then echo "[$test_name] expected exit 1 but got $exit_code" >&2 exit 1 fi echo "[$test_name] ok" } assert_not_found "not-found" "nonexistent-target-triple"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/tests/build_ios_resolver_test.sh` around lines 42 - 72, Add a test that covers the failure path when resolve_static_lib_path cannot find any candidate: create a helper (e.g., assert_not_found) that runs the build script with --resolve-static-lib and a non-existent target, captures its exit code and stderr, and asserts the process exits with code 1 and emits diagnostics (which exercise print_resolve_attempts); call this helper (e.g., "not-found", "nonexistent-target-triple") alongside the existing cases so the print_resolve_attempts + exit 1 branch is tested.
28-37: Silent test exit whenBUILD_SCRIPTfails unexpectedly underset -euo pipefail
actual="$(... "$BUILD_SCRIPT" --resolve-static-lib ...)"— if the build script exits non-zero for any reason (e.g., a missing bash dependency, a bad env),set -efires at the assignment and the script exits without printing the[$test_name] expected … got …diagnostic. The failure would surface only as a non-zero exit code with no test context.A simple guard that preserves the informative error message:
♻️ Proposed fix
if [ -n "$cargo_target_dir_value" ]; then - actual="$(CARGO_TARGET_DIR="$cargo_target_dir_value" "$BUILD_SCRIPT" --resolve-static-lib "$LIB_NAME" "$target" "$CRATE_DIR" "$WORKSPACE_DIR")" + actual="$(CARGO_TARGET_DIR="$cargo_target_dir_value" "$BUILD_SCRIPT" --resolve-static-lib "$LIB_NAME" "$target" "$CRATE_DIR" "$WORKSPACE_DIR")" \ + || { echo "[$test_name] build script exited $? unexpectedly" >&2; exit 1; } else - actual="$(env -u CARGO_TARGET_DIR "$BUILD_SCRIPT" --resolve-static-lib "$LIB_NAME" "$target" "$CRATE_DIR" "$WORKSPACE_DIR")" + actual="$(env -u CARGO_TARGET_DIR "$BUILD_SCRIPT" --resolve-static-lib "$LIB_NAME" "$target" "$CRATE_DIR" "$WORKSPACE_DIR")" \ + || { echo "[$test_name] build script exited $? unexpectedly" >&2; exit 1; } fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/tests/build_ios_resolver_test.sh` around lines 28 - 37, Temporarily disable errexit around the BUILD_SCRIPT invocation so a failing build script doesn't abort the test without context: do "set +e" before running BUILD_SCRIPT to capture its stdout into actual and its exit code into a new variable (e.g., build_status), then "set -e" to restore behavior; if build_status is non-zero, print a clear diagnostic using test_name and the build_status (and any stderr if captured) and exit 1, otherwise continue to compare actual to expected as before. Ensure you reference BUILD_SCRIPT, actual, test_name, expected, and the new build_status variable in the updated block.packages/rs-sdk-ffi/build_ios.sh (1)
18-25:shiftinsidefor arg in "$@"desynchronises positional parameters from the loopThe for-loop iterates over a snapshot of
$@;shiftmutates$@while the loop continues over the old snapshot. If two architecture tokens appear back-to-back,$@loses extra elements, potentially misaligning later reads of$1/$#(e.g., the--resolve-static-libhandler at line 110). Awhile-style loop is the idiomatic pattern:♻️ Proposed refactor
-# Parse arguments -BUILD_ARCH="${1:-arm}" - -# Parse command line arguments -for arg in "$@"; do - case $arg in - arm|x86|universal) - BUILD_ARCH="$arg" - shift - ;; - esac -done +# Parse arguments – consume leading arch token if present +BUILD_ARCH="arm" +if [[ "${1:-}" =~ ^(arm|x86|universal)$ ]]; then + BUILD_ARCH="$1" + shift +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/rs-sdk-ffi/build_ios.sh` around lines 18 - 25, The for-loop over "$@" uses shift which mutates positional parameters and desynchronizes later handlers (e.g., the --resolve-static-lib flag), so replace the for arg in "$@" loop with a while [ "$#" -gt 0 ] loop that reads arg="$1", handles arm|x86|universal by setting BUILD_ARCH="$arg" and then shift to consume the processed token, and otherwise simply shift to advance; this ensures positional parameters stay in sync for later checks like the --resolve-static-lib handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/rs-sdk-ffi/build_ios.sh`:
- Around line 99-108: The diagnostic function print_resolve_attempts currently
writes ANSI warnings to stdout; change its echo/printf calls so they write to
stderr instead (e.g., redirect each echo/printf to >&2) so that
machine-parseable stdout (used by --resolve-static-lib) remains clean; update
the top-level header line and the "Searched for:" loop outputs inside
print_resolve_attempts to redirect to stderr and keep the same message content
and formatting.
In `@packages/rs-sdk/tests/fetch/identity.rs`:
- Line 34: Rename the two new tests to follow the "should_*" convention: change
test_identity_read_not_found to should_identity_read_not_found and
test_identity_balance_read_not_found to should_identity_balance_read_not_found;
update the async test function names (e.g. the functions annotated with
#[tokio::test] in packages/rs-sdk/tests/fetch/identity.rs) and any references or
uses of those symbols (calls, ignores, or test lists) so the new names are
consistently used.
In `@packages/swift-sdk/scripts/symbol_validation.sh`:
- Line 22: The script uses a nameref on the line "local -n out_tools_ref=$1"
which requires Bash 4.3+, so add a runtime guard near the top of the script that
checks BASH_VERSINFO (or $BASH_VERSION) and exits with a clear error if the
major/minor version is less than 4.3, or alternatively add a short note in the
README/build guide stating the Bash 4.3+ requirement; update the script header
to print the version requirement and fail fast before any use of "local -n"
(reference the "local -n out_tools_ref=$1" usage to locate the affected
function).
---
Nitpick comments:
In `@packages/rs-sdk-ffi/build_ios.sh`:
- Around line 18-25: The for-loop over "$@" uses shift which mutates positional
parameters and desynchronizes later handlers (e.g., the --resolve-static-lib
flag), so replace the for arg in "$@" loop with a while [ "$#" -gt 0 ] loop that
reads arg="$1", handles arm|x86|universal by setting BUILD_ARCH="$arg" and then
shift to consume the processed token, and otherwise simply shift to advance;
this ensures positional parameters stay in sync for later checks like the
--resolve-static-lib handler.
In `@packages/rs-sdk-ffi/src/address_sync/provider.rs`:
- Around line 64-71: Change the three nullable callback fields (has_pending,
highest_found_index, destroy) in the provider vtable from raw pointers to typed
nullable function pointers using Option<unsafe extern "C" fn(...)> with the
correct signatures; update any helpers that transmute these fields (e.g., those
reading (*self.ffi.vtable).has_pending and highest_found_index) to read the
field directly and call it via if let Some(cb) = vtable.has_pending { cb(args)
}, and simplify the destruction path in mod.rs to if let Some(destroy) =
vtable.destroy { destroy(provider.context) }; also update tests/initializers to
use None instead of std::ptr::null() and adjust any call sites to match the new
function-pointer types for compile-time safety.
In `@packages/rs-sdk-ffi/tests/build_ios_resolver_test.sh`:
- Around line 42-72: Add a test that covers the failure path when
resolve_static_lib_path cannot find any candidate: create a helper (e.g.,
assert_not_found) that runs the build script with --resolve-static-lib and a
non-existent target, captures its exit code and stderr, and asserts the process
exits with code 1 and emits diagnostics (which exercise print_resolve_attempts);
call this helper (e.g., "not-found", "nonexistent-target-triple") alongside the
existing cases so the print_resolve_attempts + exit 1 branch is tested.
- Around line 28-37: Temporarily disable errexit around the BUILD_SCRIPT
invocation so a failing build script doesn't abort the test without context: do
"set +e" before running BUILD_SCRIPT to capture its stdout into actual and its
exit code into a new variable (e.g., build_status), then "set -e" to restore
behavior; if build_status is non-zero, print a clear diagnostic using test_name
and the build_status (and any stderr if captured) and exit 1, otherwise continue
to compare actual to expected as before. Ensure you reference BUILD_SCRIPT,
actual, test_name, expected, and the new build_status variable in the updated
block.
In `@packages/rs-sdk/tests/fetch/identity.rs`:
- Around line 227-232: Rename the misleading constant UNIQUE_KEY_HASH to a name
that reflects it is a non-unique test key (for example NON_UNIQUE_KEY_HASH or
DUPLICATE_KEY_HASH) and update all references to it in the same test function
(there are three usages mentioned) so the constant name and comments align;
ensure the constant identifier is changed where declared and where referenced in
tests to avoid any compile errors.
In `@packages/swift-sdk/build_ios.sh`:
- Around line 44-71: The script currently hardcodes
SIM_LIB_DIR="$DEST_XCFRAMEWORK_DIR/ios-arm64-simulator", which fails on Intel
macs; change it to auto-detect the simulator slice by checking a prioritized
list of possible directories (e.g., "ios-arm64-simulator",
"ios-x86_64-simulator", "ios-arm64_x86_64-simulator", etc.) under
DEST_XCFRAMEWORK_DIR and set SIM_LIB_DIR to the first existing one; update the
error path check (the block referencing SIM_LIB_DIR and the message "Missing
simulator library directory") to print which candidates were attempted if none
exist and then exit; ensure the rest of the script still uses SIM_LIB_DIR,
SIM_LIBS, and SEEN_LIB unchanged so library discovery logic remains intact.
In `@packages/swift-sdk/scripts/symbol_validation.sh`:
- Line 85: Add explicit ShellCheck suppression comments for the intentional
caller-facing globals to silence SC2034 false positives: next to the assignment
to FOUND_SYMBOL_TOOL and the assignment to LAST_NM_STDERR_HINT in
symbol_validation.sh, add a preceding comment "# shellcheck disable=SC2034" so
ShellCheck knows these variables are intentionally exported/consumed by
build_ios.sh and the test harness; keep the comment immediately above each
assignment to make the suppression local and clear.
- Line 84: The current check in contains_fixed_in_file called with
prefixed_symbol and bare_symbol can match substrings (e.g., matching
_dash_spv_ffi_config_add_peer inside a longer
_dash_spv_ffi_config_add_peers_from_list); update contains_fixed_in_file (or its
callers in symbol_validation.sh where prefixed_symbol and bare_symbol are
checked against out_file) to perform word-boundary-aware fixed-string matches
(for example use grep -wF or rg -wF) so the lookup only matches whole symbols
rather than substrings; modify the implementation of contains_fixed_in_file to
accept and use the word-boundary flag or replace the call sites to invoke a
word-boundary-aware search utility.
packages/rs-sdk-ffi/build_ios.sh
Outdated
| print_resolve_attempts() { | ||
| local label="$1" | ||
| local candidate | ||
|
|
||
| echo -e "${YELLOW}⚠ $label${NC}" | ||
| echo -e "${YELLOW} Searched for:${NC}" | ||
| for candidate in "${RESOLVE_LAST_SEARCH_PATHS[@]}"; do | ||
| echo -e "${YELLOW} - $candidate${NC}" | ||
| done | ||
| } |
There was a problem hiding this comment.
print_resolve_attempts should write diagnostics to stderr, not stdout
In --resolve-static-lib mode stdout is the machine-parseable channel (the resolved path). Any caller capturing output via command substitution would get the yellow ANSI escape sequences mixed into the captured string on failure:
path=$(./build_ios.sh --resolve-static-lib librs_sdk_ffi.a aarch64-apple-ios . .) || ...
# on failure: $path contains "⚠ Could not resolve …" instead of being empty/unset🐛 Proposed fix – redirect diagnostics to stderr
print_resolve_attempts() {
local label="$1"
local candidate
- echo -e "${YELLOW}⚠ $label${NC}"
- echo -e "${YELLOW} Searched for:${NC}"
+ echo -e "${YELLOW}⚠ $label${NC}" >&2
+ echo -e "${YELLOW} Searched for:${NC}" >&2
for candidate in "${RESOLVE_LAST_SEARCH_PATHS[@]}"; do
- echo -e "${YELLOW} - $candidate${NC}"
+ echo -e "${YELLOW} - $candidate${NC}" >&2
done
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-sdk-ffi/build_ios.sh` around lines 99 - 108, The diagnostic
function print_resolve_attempts currently writes ANSI warnings to stdout; change
its echo/printf calls so they write to stderr instead (e.g., redirect each
echo/printf to >&2) so that machine-parseable stdout (used by
--resolve-static-lib) remains clean; update the top-level header line and the
"Searched for:" loop outputs inside print_resolve_attempts to redirect to stderr
and keep the same message content and formatting.
| /// Given a non-existent identity ID, when I fetch the identity, I get None. | ||
| #[tokio::test(flavor = "multi_thread", worker_threads = 1)] | ||
| #[ignore = "requires mock vectors to be generated against a running Platform"] | ||
| async fn test_identity_read_not_found() { |
There was a problem hiding this comment.
New tests must follow the should_* naming convention.
Both test_identity_read_not_found and test_identity_balance_read_not_found are new tests added in this PR. The coding guidelines require test names to start with should …, and the established learning enforces this for new tests while preserving existing test_* names.
♻️ Proposed rename
-async fn test_identity_read_not_found() {
+async fn should_return_none_when_fetching_non_existent_identity() {-async fn test_identity_balance_read_not_found() {
+async fn should_return_none_when_fetching_non_existent_identity_balance() {As per coding guidelines: "Name tests descriptively, starting with 'should …'" (**/tests/**/*.rs), and based on learnings: "Enforce descriptive naming for new tests in test files with names starting with 'should …'. Do not rename existing tests that already follow a different convention."
Also applies to: 114-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/rs-sdk/tests/fetch/identity.rs` at line 34, Rename the two new tests
to follow the "should_*" convention: change test_identity_read_not_found to
should_identity_read_not_found and test_identity_balance_read_not_found to
should_identity_balance_read_not_found; update the async test function names
(e.g. the functions annotated with #[tokio::test] in
packages/rs-sdk/tests/fetch/identity.rs) and any references or uses of those
symbols (calls, ignores, or test lists) so the new names are consistently used.
| } | ||
|
|
||
| detect_nm_tools() { | ||
| local -n out_tools_ref=$1 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for bash version guards and documentation
rg -rn 'bash.*4\.' packages/swift-sdk/ 2>/dev/null || echo "No bash version references found"
rg -rn 'BASH_VERSION\|BASH_VERSINFO' packages/swift-sdk/ 2>/dev/null || echo "No bash version checks found"
# Check the shebang lines across the swift-sdk scripts
rg -n '^#!' packages/swift-sdk/scripts/ packages/swift-sdk/build_ios.sh packages/swift-sdk/Tests/symbol_validation_harness.sh 2>/dev/null || trueRepository: dashpay/platform
Length of output: 475
🏁 Script executed:
# Let's examine the actual symbol_validation.sh file to see the context and line 22
cat -n packages/swift-sdk/scripts/symbol_validation.sh | head -50Repository: dashpay/platform
Length of output: 1409
🏁 Script executed:
# Check if there's any documentation about bash version requirements in README or setup files
find packages/swift-sdk -type f \( -name "README*" -o -name "*.md" -o -name "setup*" -o -name "INSTALL*" \) | head -20Repository: dashpay/platform
Length of output: 1062
🏁 Script executed:
# Check the actual bash version requirement for nameref - verify when it was introduced
# and look at how scripts handle this
rg -n 'local.*-n' packages/swift-sdk/scripts/Repository: dashpay/platform
Length of output: 138
🏁 Script executed:
# Check if there's any CI configuration or setup process that handles bash version
find packages/swift-sdk -type f \( -name ".github" -o -name "*.yml" -o -name "*.yaml" -o -name "setup*" -o -name "Makefile" \) 2>/dev/null
cat packages/swift-sdk/build_ios.sh | head -30Repository: dashpay/platform
Length of output: 987
🏁 Script executed:
# Check if there's any documentation about build prerequisites
grep -r "bash\|Bash\|BASH" packages/swift-sdk/README.md packages/swift-sdk/BUILD_GUIDE_FOR_AI.md packages/swift-sdk/BUILD_TROUBLESHOOTING.md 2>/dev/null | head -20Repository: dashpay/platform
Length of output: 1004
Add Bash version guard or document the 4.3+ requirement for local -n nameref on line 22.
local -n out_tools_ref=$1 requires Bash 4.3+, but stock macOS ships Bash 3.2. This will fail with "local: -n: invalid option" on macOS without Homebrew Bash, even though #!/usr/bin/env bash at the top assumes a compatible version is available. While the build environment typically provides Homebrew Bash, add an explicit version check near the top of the file or document this requirement in the build guide to prevent confusing failures for new contributors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/swift-sdk/scripts/symbol_validation.sh` at line 22, The script uses
a nameref on the line "local -n out_tools_ref=$1" which requires Bash 4.3+, so
add a runtime guard near the top of the script that checks BASH_VERSINFO (or
$BASH_VERSION) and exits with a clear error if the major/minor version is less
than 4.3, or alternatively add a short note in the README/build guide stating
the Bash 4.3+ requirement; update the script header to print the version
requirement and fail fast before any use of "local -n" (reference the "local -n
out_tools_ref=$1" usage to locate the affected function).
63971b5 to
d8495d9
Compare
| /// Token ID that doesn't exist in test state. | ||
| /// NOTE: This has the same byte pattern as IDENTITY_ID_1 ([1; 32]). This is acceptable because | ||
| /// token IDs and identity IDs occupy different namespaces in the state tree. |
| /// Key hash generated during test data creation in `create_genesis_state`. | ||
| /// Used to test fetching identities by non-unique public key hash. | ||
| /// Source: packages/rs-drive-abci/src/execution/platform_events/initialization/create_genesis_state/mod.rs |
There was a problem hiding this comment.
more comment changes? please review this pr, and minimize unneeded changes
thepastaclaw
force-pushed
the
test/rs-sdk-dpns-assertions
branch
2 times, most recently
from
804372c to
705fc5c
Compare
thepastaclaw
force-pushed
the
test/rs-sdk-dpns-assertions
branch
from
705fc5c to
94bd5fc
Compare
2 participants
Summary
Replace print-based "visual inspection" tests in
rs-sdkwith real assertions, add error path tests for identity fetches, and document magic values.Addresses audit findings C01, I02, I03 from the SDK test quality audit.
Changes
C01: DPNS tests — println → assertions
dpns_unit_tests.rs(3 unit tests):test_dpns_validation_functions: Replace emoji/println withassert_eq!for username validation, homograph conversion, and contested name checks. Expected values were already present in the code but never compared programmatically.test_dpns_edge_cases: Replace println withassert!/assert!(!...)for length boundaries, special characters, and unicode rejection.test_dpns_homograph_safety: Replace println withassert_eq!for homograph conversions and idempotency checks.dpns_queries_test.rs(2 integration tests,#[ignore]):test_dpns_queries_from_docs: Replace match/println error swallowing with.expect()— network errors now properly fail the test.test_dpns_search_variations: Same treatment.I02/I03: Identity fetch assertions + error paths
identity.rs:test_identity_balance_read: Addassert!(balance > 0).test_identity_balance_revision_read: Addassert!(balance > 0)andassert!(revision >= 1).test_identity_read_not_found— fetch withUNKNOWN_IDENTITY_ID, assertOk(None).test_identity_balance_read_not_found— fetch balance withUNKNOWN_IDENTITY_ID, assertOk(None).Magic value documentation
identity.rs: DocumentUNIQUE_KEY_HASHsource (genesis state creation).dpns_queries_test.rs: DocumentTEST_IDENTITY_ID,TEST_USERNAME,TEST_PREFIXsources and testnet node address.generated_data.rs: DocumentUNKNOWN_TOKEN_IDcollision withIDENTITY_ID_1and why it's acceptable (different namespaces).Verification
cargo check -p dash-sdk --features=mocks --testspasses cleanly (only pre-existingdead_codewarnings).#[ignore](require running testnet node).Summary by CodeRabbit
Tests
Build / Tooling
Documentation
Validation
What was tested
Results
Environment
dashpay/platform