Open
Conversation
carrion256
force-pushed
the
feature/eng-21-client-sdk
branch
5 times, most recently
from
e40dc06 to
539cd02
Compare
carrion256
force-pushed
the
feature/eng-21-client-sdk
branch
2 times, most recently
from
b4a1572 to
067931f
Compare
carrion256
changed the title
carrion256
force-pushed
the
vault-pre-audit
branch
4 times, most recently
from
c5260f2 to
63b75f6
Compare
carrion256
force-pushed
the
feature/eng-21-client-sdk
branch
from
34e4036 to
98fbf2d
Compare
Installed near-sandbox into /home/runner/work/contracts/contracts/target/debug/build/near-sandbox-utils-c1fa841a256894cc/out/.near/near-sandbox-2.8.0/near-sandbox Gas ReportSnapshot Limits
Estimated snapshot limit: 9631
Estimated snapshot limit: 11495 Action Gas Descriptors
Gas ReportEstimated allocation limit: 0 Action Gas Descriptors
|
carrion256
force-pushed
the
feature/eng-21-client-sdk
branch
from
98fbf2d to
6854290
Compare
… macros - Create view_core.rs with shared view_with_cache() and build_view_cache() - Add impl_view_cache_methods! macro for cache enable/disable/clear methods - Add impl_vault_view_methods! macro for complex view methods (get_cap_groups, get_pending_governance_actions, market lookups, snapshots, etc.) - Apply macros to VaultClient, VaultViewClient, and KeyPoolClient - Remove ~400 lines of duplicated view method implementations
- Add StorageBalanceBounds and StorageBalance types for NEP-145 support - Add storage_balance_bounds() and storage_balance_of() methods for vault - Add token_storage_balance_of() and token_storage_deposit() for any contract - Update smoke test to handle full curator setup flow: - Clear stuck withdrawals before starting (Step 0) - Register vault with itself, token, and market contracts - Add market via submit_cap/accept_cap if MARKET_ACCOUNT set - Set supply_queue when max_deposit is 0 - Check storage registration before depositing (saves NEAR) - Drive withdrawals to completion - Add MARKET_ACCOUNT env var for automatic market setup - Fix redeem storage deposit (0.003 NEAR required) - Make storage_balance_of DRY by delegating to token_storage_balance_of
- "Queue head (next to process)" instead of "Next pending withdrawal ID" - "Queue tail (next ID to assign)" instead of "Queue tail" - "Queue head after redeem" to clarify it's not the newly created ID
- Add @ prefix to commands to prevent echoing credentials - Add MARKET_ACCOUNT and RPC_API_KEY env vars - Update default RPC URL to fastnear.com
- Fix ft_transfer_call to compute `used = amount - unused` per NEP-141 spec (ft_on_transfer returns unused/refund amount, not used amount) - Update smoke test to show "unused/refunded" instead of "tokens used" - Add documentation warning that return value shouldn't be fully trusted - Clean up imports in key_pool/client.rs and view_client.rs - Format improvements for error handling blocks
- Fix ambiguous .into() on string literals in asset.rs (primitive_types conflict) - Fix AccountId version mismatch in price_transformer.rs (near_account_id v1 vs v2) - Fix unnecessary cast in wad.rs - Add workspace.package.version for client crate inheritance - Add missing_errors_doc/missing_panics_doc to workspace lint allows - Fix ~65 clippy warnings in client/vault (format strings, map_or, casts, dead code, etc.) - Regenerate Cargo.lock for client crate dependencies
carrion256
force-pushed
the
feature/eng-21-client-sdk
branch
from
6854290 to
2900e60
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Collaborator
Author
|
Tip For best results, initiate chat on the files or code changes.
Please let me know what you'd like me to do! |
Collaborator
Author
|
@CodeRabbit review my pr |
Collaborator
Author
review my pr |
Collaborator
|
@coderabbitai Review this PR. |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughVault FFI SDK Implementation (Python + other bindings) — concise reviewAdds a new UniFFI-backed Vault client SDK under client/vault with Python bindings, generated bindings artifacts, build automation, README, and a Python smoke-test suite. Introduces a new workspace client/* member and workspace.package version. High-level changes
Major Rust / FFI-visible surface
Supporting changes in other crates
Files with largest review effort
Critical review points (must inspect before merge)
Recommended immediate reviewer actions
Risk summary WalkthroughAdds a new templar-vault client crate with UniFFI bindings, multi-key key-pool transaction management (nonce caching, retries, view cache), extensive FFI exports and macros, Python binding generation and smoke tests, workspace metadata updates, and several common-library utilities and serializers. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Client
participant VC as VaultClient
participant KPC as KeyPoolClient
participant KP as KeyPool
participant KS as KeySlot
participant RPC as NEAR_RPC
User->>VC: ft_transfer_call(token, amount, msg)
VC->>KPC: vault_call(method, args)
KPC->>KP: select() %% choose least-loaded key
KP->>KS: acquire() %% lock slot, inc in-flight
KS->>RPC: fetch_access_key_data() %% get nonce & block_hash
RPC-->>KS: (next_nonce, block_hash)
KS->>KS: sign_transaction(next_nonce)
KS->>RPC: send_transaction(signed_tx)
RPC-->>KS: tx_hash
KS->>RPC: poll_tx_status(tx_hash)
RPC-->>KS: FinalExecutionStatus
KS->>KS: record_success/failure() %% update in-flight, health, counters
KS-->>KPC: result
KPC-->>VC: parsed_result
VC-->>User: OperationResult
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 28
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
common/src/vault/wad.rs (1)
383-390:⚠️ Potential issue | 🟡 MinorTypo in JSON schema description.
Line 386: "faction" should be "fraction".
📝 Proposed fix
fn json_schema(generator: &mut SchemaGenerator) -> Schema { let mut schema = generator.subschema_for::<Number>().into_object(); schema.metadata().description = - Some("Wad fixed faction back by 256-bit unsigned integer".to_string()); + Some("Wad fixed fraction backed by 256-bit unsigned integer".to_string()); schema.string().pattern = Some("^(0|[1-9][0-9]{0,77})$".to_string()); schema.into() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/src/vault/wad.rs` around lines 383 - 390, The JSON schema description in the json_schema function currently has a typo ("faction" instead of "fraction"); update the metadata description assigned to schema.metadata().description in the json_schema(&mut SchemaGenerator) -> Schema implementation for Number to read something like "Wad fixed fraction back by 256-bit unsigned integer" (or the exact intended wording), ensuring only the word is corrected and nothing else in the SchemaGenerator/Schema manipulation (schema.string().pattern, schema.into()) is changed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 26-29: Add an ignore rule for the smoke-test environment file by
updating .gitignore to include the literal entry ".env.smoketest" (place it with
other env-related ignores so it’s obvious), ensuring the smoke-test secrets
copied into .env.smoketest are not accidentally committed.
In `@client/vault/Cargo.toml`:
- Around line 15-41: The Cargo.toml currently uses wildcard versions for uniffi
and zeroize (uniffi in both [dependencies] and [build-dependencies], zeroize in
[dependencies]); pin these to concrete versions to ensure reproducible builds by
either adding uniffi and zeroize with exact versions to the workspace's
[workspace.dependencies] and changing their entries here to use workspace =
true, or by replacing the "*" entries locally with explicit version strings (and
ensure the build-dependencies uniffi matches the chosen pinned version and
required features like "build", "tokio", "wasm-unstable-single-threaded");
update both occurrences of uniffi and the zeroize entry accordingly.
In `@client/vault/Makefile`:
- Around line 58-82: The Makefile target smoke-test-full performs a preflight
check but omits USER_ACCOUNT; update the conditional in the smoke-test-full
target to also require USER_ACCOUNT (in addition to USER_SECRET_KEY) and include
USER_ACCOUNT in the printed usage text so the preflight exits with a clear error
when USER_ACCOUNT is missing; locate the smoke-test-full rule and modify the if
[ -z ... ] test and the echo usage block accordingly to reference USER_ACCOUNT.
- Around line 31-33: The gen Makefile target is passing `$@` (expands to "gen")
to uniffi-bindgen which is an invalid subcommand; update the gen target to call
a valid subcommand (e.g., "scaffolding") and supply the required arguments (UDL
input, `--language` and `--out-dir`) similar to how the `python` target is
invoked so uniffi-bindgen runs successfully (change the command in the `gen`
target to use `scaffolding` and the same argument pattern used by the `python`
target).
In `@client/vault/README.md`:
- Around line 163-201: The fenced code blocks under the Method Categories
section (the "Simple View Methods", "Parameterized View Methods", "Typed View
Methods", and "Call Methods" blocks that start with ```rust and list methods
like get_total_assets(), convert_to_shares(assets), get_configuration() ->
VaultConfiguration, deposit(amount, receiver, deposit_yocto), etc.) need a blank
line before and after each fenced block to satisfy MD031; update README.md by
inserting a single empty line immediately above each opening ``` and immediately
below each closing ``` for those four blocks.
- Around line 345-373: The fenced code blocks under the "Client Construction"
section (the examples using VaultClient.new_single_key_default and
VaultClient.new_key_pool) need blank lines inserted immediately before and after
each triple-backtick fence to satisfy MD031; edit the README.md section so there
is an empty line above the opening ``` and an empty line below the closing ```
for both the single-key and multi-key examples.
- Around line 83-97: Add a language tag to the fenced ASCII diagram block in the
README (the VaultClient ASCII diagram containing "VaultClient" and "KeySlot"
boxes) so markdownlint MD040 is satisfied; change the opening triple-backtick
fence to include a language identifier like "text" (e.g., ```text) for that
diagram block.
- Line 138: Update the README line that currently reads "Default: 2000 entries,
2 second TTL." to use a hyphenated compound modifier so it reads "Default: 2000
entries, 2‑second TTL." (replace the plain hyphen with a non‑breaking hyphen if
your linter requires it) — locate and edit the string in client/vault/README.md
where the default entry/TTL description appears.
- Around line 422-450: The fenced project-structure block in client/vault/
README.md is missing a language tag (MD040); update the opening fence for the
tree that begins with "contracts/client/vault/" to include a language label
(e.g., change ``` to ```text) so the code fence has an explicit language tag and
satisfies the linter rule.
- Around line 312-320: The Markdown code fence showing the ABI output tree (the
block containing "web/ ├── src/ ├── abi/ ├── generated/ └── vault.abi.json" and
"vault.ts") needs a language tag to satisfy MD040; update the opening fence from
``` to ```text so the tree rendering is marked as plain text and markdownlint
passes.
In `@client/vault/smoke_test.py`:
- Around line 71-166: Replace hardcoded secret key literals by introducing a
single DUMMY_SECRET_KEY constant read from the environment with a safe fallback
and use that constant when constructing KeyCredential in tests (refer to
KeyCredential and tests test_key_credential_creation,
test_invalid_credential_rejected, test_empty_credentials_rejected); remove
unused local assignments to client in the negative-path tests (the try blocks in
test_invalid_credential_rejected and test_empty_credentials_rejected) so you
don’t assign to client when you only expect an exception; ensure the constant
name DUMMY_SECRET_KEY is used everywhere instead of inline "ed25519:..." strings
and that no unused variables remain to address the Ruff S106 and F841 findings.
- Around line 1-64: SmokeTestConfig.from_env currently uses
os.environ["VAULT_ACCOUNT"] which raises a KeyError with little context; change
it to read VAULT_ACCOUNT via os.environ.get("VAULT_ACCOUNT") (or wrap the direct
access in a try/except KeyError) and raise a clear ValueError or RuntimeError if
missing that includes an actionable message like "VAULT_ACCOUNT environment
variable is required for SmokeTestConfig.from_env". Update the method signature
and docstring accordingly to indicate the explicit validation.
- Around line 267-400: test_view_methods currently creates a VaultClient with a
dummy credential even though it is read-only; replace that instantiation with
the read-only client class VaultViewClient (construct the client using
VaultViewClient with rpc_url, vault AccountId and the existing client_config,
omitting any KeyCredential) so the test exercises the view-only UniFFI surface
and view cache behavior; update references to client (created in
test_view_methods) to use the VaultViewClient instance and ensure the existing
view calls (get_total_assets, list_markets_with_ids, preview_deposit, etc.)
remain unchanged.
- Around line 402-1020: The smoke test test_happy_path_flow contains unbounded
while True loops (around the withdraw-driving and wait loops that call
get_withdrawing_op_id, get_current_withdraw_request_id,
execute_market_withdrawal, and execute_withdrawal) and several lint/exception
issues; add a finite timeout/deadline (e.g., loop with max_iterations or
deadline timestamp) to each wait loop so they break and report a timeout instead
of hanging, replace all bare except: and try/except:pass with except Exception
as e: and log/print e (references: the initial clearing loop that calls
allocator_client.execute_market_withdrawal, the donation/refresh try block, the
reallocation and flush loops), rename unused loop variable i to _i in the
clearing loop, remove stray f-string prefixes with no placeholders, and use or
print the before_idle variable (or remove it) to satisfy lint (references:
before_idle in the donation demo block and loop variables in the Step 0 clearing
loop and Step 5 flush loops). Ensure any added timeouts surface an explanatory
message before breaking so CI/users can see why the loop ended.
In `@client/vault/src/client.rs`:
- Around line 1-3: Remove the duplicated module documentation line at the top of
client.rs so only a single module doc comment remains (delete one of the
repeated "//! Canonical vault client for high-concurrency use." lines); ensure
the file still begins with a single module-level doc comment and no other
whitespace or content is changed.
- Around line 318-555: The PR currently maps contract-call failures to
ErrorWrapper::Wrapped(string), which prevents callers from programmatically
distinguishing RPC vs transaction failures; update the match arms in
storage_deposit (method storage_deposit) and storage_deposit_on (method
storage_deposit_on) to return more specific ErrorWrapper variants instead of
Wrapped: convert FinalExecutionStatus::Failure(err) to
Err(ErrorWrapper::TransactionFailed(err)) (or the equivalent TransactionFailed
constructor your crate exposes) and replace the generic "Unexpected execution
status" Wrapped with a distinct variant (e.g., ErrorWrapper::TransactionFailed
or a new ErrorWrapper::UnexpectedExecutionStatus containing the status) so
consumers (Python/TS) can branch on error type; keep the existing
map_err(ErrorWrapper::from) uses for view/call errors.
In `@client/vault/src/key_pool/client.rs`:
- Around line 326-397: submit_and_poll currently bails when
result.final_execution_outcome is None; instead, when
result.final_execution_outcome is None call and await
self.poll_tx_status(sender_account_id, tx_hash, deadline) and return that result
(so the function falls back to polling until deadline); keep the existing
behavior when final_execution_outcome is Some by extracting
outcome.into_outcome().status and returning Ok(status). Update the block that
handles `let Some(outcome) = result.final_execution_outcome else { ... }` to
perform the poll fallback using the existing deadline, sender_account_id, and
tx_hash symbols.
- Around line 39-81: The unsafe zeroization in KeyCredential::into_signer uses
an unsafe closure zeroize_secret that calls s.as_bytes_mut().zeroize(); remove
that unsafe closure and replace its uses with the safe Zeroize impl on String by
calling self.secret_key.zeroize() (or mutably zeroizing the String variable in
scope) wherever zeroize_secret(&mut self.secret_key) is invoked, and delete the
zeroize_secret helper; ensure you still zero the secret on all error paths and
before returning the InMemorySigner.
- Around line 464-479: is_invalid_nonce_error and is_rpc_invalid_nonce rely on
fragile string matching; update is_rpc_invalid_nonce to pattern-match the typed
InvalidTxError::InvalidNonce variant instead. Import use
near_primitives::errors::InvalidTxError and match against
RpcTransactionError::InvalidTransaction { context: InvalidTxError::InvalidNonce
{ .. }, .. } inside is_rpc_invalid_nonce (and remove the debug/string checks),
returning true on that match and false otherwise; leave is_invalid_nonce_error
either for non-RPC errors or similarly update it to inspect anyhow::Error
sources to detect InvalidTxError if applicable.
In `@client/vault/src/key_pool/pool.rs`:
- Around line 95-110: The selection logic can race because in_flight_count() may
change between computing min and filtering, potentially producing an empty
candidates vec; fix by snapshotting each slot's in_flight_count once (e.g., map
self.slots.iter() to a temporary Vec of (slot, count)), compute min_in_flight
from those counts, filter that snapshot for matching counts to build candidates,
then use next_index % candidates.len() and return candidates[idx].clone(); keep
the existing AllKeysUnhealthy error path when no healthy slots.
In `@client/vault/src/lib.rs`:
- Around line 967-974: parse_u128 currently returns ErrorWrapper::from(...)
which loses the specific InvalidU128 signal; change parse_u128 to validate both
direct parse and JSON-string parse and return ErrorWrapper::InvalidU128 with the
offending string on failure (use the original input or inner string in the JSON
case). Also audit the From<anyhow::Error> implementation and ensure it maps
specific error kinds to ErrorWrapper::Rpc or ErrorWrapper::TransactionFailed
where appropriate (e.g., RPC transport/timeout -> Rpc, transaction-specific
failure -> TransactionFailed) rather than collapsing everything into a generic
Wrapped/Serde variant; update the conversion logic to match those variants so
FFI callers can deterministically handle invalid numeric input, RPC errors, and
transaction failures.
- Around line 1192-1272: Tests pass but cache misses can occur if
ViewCacheKey.args (Vec<u8>) is created from nondeterministic serialization;
update the code to normalize args before using them in keys or add documentation
and a helper to enforce canonical serialization: either (A) add a helper
function (e.g., canonicalize_args or ViewCacheKey::from_serialized_args) that
canonicalizes map-like inputs (use BTreeMap ordering or a canonical serializer)
and call it where ViewCacheKey is constructed, or (B) add a clear doc comment on
the ViewCacheKey type and public constructors instructing callers to use
deterministic serialization (suggest BTreeMap or a canonical serializer) so
cache hits are reliable.
In `@client/vault/src/view_client.rs`:
- Around line 104-133: Define a single constant string (e.g., READ_ONLY_ERR:
&str) containing "VaultViewClient is read-only; contract calls are not
supported" and replace the duplicated literal in the impl methods
vault_call_with and vault_call_returning to return
ErrorWrapper::Wrapped(READ_ONLY_ERR.to_string()); keep vault_call unchanged and
ensure any tests that assert this message use the same constant or the unchanged
text.
- Around line 135-137: The VaultViewClient currently gets mutating vault APIs
via crate::impl_vault_methods!(VaultViewClient) which exposes methods (e.g.,
accept_guardian) that always fail at runtime; remove that macro invocation so
VaultViewClient only receives crate::impl_vault_view_methods!(VaultViewClient)
and crate::impl_view_cache_methods!(VaultViewClient), or if you need to preserve
names for other types, split the impls into distinct macros (e.g.,
impl_vault_readonly_methods! vs impl_vault_methods!) and apply only the readonly
macro to VaultViewClient; also add a UniFFI-facing docstring on VaultViewClient
clarifying it is read-only so IDEs/users don’t expect successful state-changing
calls.
In `@common/src/vault/wad.rs`:
- Around line 464-476: Add three edge-case unit tests for the Wad JSON
deserialization: implement wad_json_accepts_zero() to parse "\"0\"" and assert
Wad::is_zero or equivalent, wad_json_rejects_empty_string() to deserialize
"\"\"" and assert the error string contains "non-empty" (or the serializer's
non-empty error text), and wad_json_rejects_overflow() to deserialize a 79-digit
decimal string (e.g., "9".repeat(79) wrapped in quotes) and assert the error
indicates overflow; place these tests alongside the existing
wad_json_accepts_decimal_string and wad_json_rejects_hex_string tests in
common/src/vault/wad.rs so they run under the same test module and reference the
Wad type and serde_json deserialization used in the other tests.
- Around line 239-271: The deserializer for Wad (impl Deserialize for Wad -> fn
deserialize) currently accepts strings with leading zeros (e.g., "007") while
the JSON schema pattern forbids them; update the validation right after
obtaining s (the parsed String) to reject any input where s.len() > 1 &&
s.starts_with('0') by returning a serde::de::Error::custom (use a clear message
such as "Wad must not contain leading zeros") so only "0" or non-leading-zero
decimals are allowed, leaving the existing digit and overflow checks intact.
In `@test-utils/src/controller/vault.rs`:
- Around line 98-103: Add a brief doc comment above the refresh_idle_balance
declaration explaining that it is an intentional alias of resync_idle_balance
(both map to the same contract method) and state the reason (e.g., SDK naming
preference or backward compatibility) so future maintainers understand the
duplication; reference the functions resync_idle_balance and
refresh_idle_balance and the attribute ["resync_idle_balance"] in the comment.
---
Outside diff comments:
In `@common/src/vault/wad.rs`:
- Around line 383-390: The JSON schema description in the json_schema function
currently has a typo ("faction" instead of "fraction"); update the metadata
description assigned to schema.metadata().description in the json_schema(&mut
SchemaGenerator) -> Schema implementation for Number to read something like "Wad
fixed fraction back by 256-bit unsigned integer" (or the exact intended
wording), ensuring only the word is corrected and nothing else in the
SchemaGenerator/Schema manipulation (schema.string().pattern, schema.into()) is
changed.
Comment on lines
58
to
82
| # Full happy path integration tests (requires funded accounts) | ||
| # Usage: make smoke-test-full VAULT_ACCOUNT=<vault> USER_ACCOUNT=<acc> USER_SECRET_KEY=<key> ... | ||
| smoke-test-full: python | ||
| @if [ -z "$(VAULT_ACCOUNT)" ] || [ -z "$(USER_SECRET_KEY)" ]; then \ | ||
| echo "Usage: make smoke-test-full \\"; \ | ||
| echo " VAULT_ACCOUNT=<vault.testnet> \\"; \ | ||
| echo " USER_ACCOUNT=<user.testnet> \\"; \ | ||
| echo " USER_SECRET_KEY='ed25519:...' \\"; \ | ||
| echo " [ALLOCATOR_ACCOUNT=<allocator.testnet>] \\"; \ | ||
| echo " [ALLOCATOR_SECRET_KEY='ed25519:...']"; \ | ||
| exit 1; \ | ||
| fi | ||
| @PYTHONPATH="$(PY_OUT)" \ | ||
| VAULT_ACCOUNT="$(VAULT_ACCOUNT)" \ | ||
| UNDERLYING_TOKEN="$(UNDERLYING_TOKEN)" \ | ||
| MARKET_ACCOUNT="$(MARKET_ACCOUNT)" \ | ||
| USER_ACCOUNT="$(USER_ACCOUNT)" \ | ||
| USER_SECRET_KEY="$(USER_SECRET_KEY)" \ | ||
| ALLOCATOR_ACCOUNT="$(ALLOCATOR_ACCOUNT)" \ | ||
| ALLOCATOR_SECRET_KEY="$(ALLOCATOR_SECRET_KEY)" \ | ||
| RPC_URL="$(or $(RPC_URL),https://rpc.testnet.fastnear.com)" \ | ||
| RPC_API_KEY="$(RPC_API_KEY)" \ | ||
| RUN_FULL_TESTS=1 \ | ||
| python3 smoke_test.py --full | ||
|
|
There was a problem hiding this comment.
Also require USER_ACCOUNT in the preflight check.
The full-flow target uses USER_ACCOUNT but doesn’t validate it, leading to confusing runtime failures.
✅ Suggested preflight validation
- `@if` [ -z "$(VAULT_ACCOUNT)" ] || [ -z "$(USER_SECRET_KEY)" ]; then \
+ `@if` [ -z "$(VAULT_ACCOUNT)" ] || [ -z "$(USER_ACCOUNT)" ] || [ -z "$(USER_SECRET_KEY)" ]; then \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/vault/Makefile` around lines 58 - 82, The Makefile target
smoke-test-full performs a preflight check but omits USER_ACCOUNT; update the
conditional in the smoke-test-full target to also require USER_ACCOUNT (in
addition to USER_SECRET_KEY) and include USER_ACCOUNT in the printed usage text
so the preflight exits with a clear error when USER_ACCOUNT is missing; locate
the smoke-test-full rule and modify the if [ -z ... ] test and the echo usage
block accordingly to reference USER_ACCOUNT.
client/vault/src/view_client.rs
Outdated
Comment on lines
104
to
133
| #[allow(clippy::unused_async)] | ||
| async fn vault_call_with( | ||
| &self, | ||
| _method: &str, | ||
| _args: impl Serialize, | ||
| _gas: Option<Gas>, | ||
| _deposit: Option<u128>, | ||
| ) -> Result<(), ErrorWrapper> { | ||
| Err(ErrorWrapper::Wrapped( | ||
| "VaultViewClient is read-only; contract calls are not supported".to_string(), | ||
| )) | ||
| } | ||
|
|
||
| async fn vault_call(&self, method: &str, args: impl Serialize) -> Result<(), ErrorWrapper> { | ||
| self.vault_call_with(method, args, None, None).await | ||
| } | ||
|
|
||
| #[allow(clippy::unused_async)] | ||
| async fn vault_call_returning<T: DeserializeOwned>( | ||
| &self, | ||
| _method: &str, | ||
| _args: impl Serialize, | ||
| _gas: Option<Gas>, | ||
| _deposit: Option<u128>, | ||
| ) -> Result<T, ErrorWrapper> { | ||
| Err(ErrorWrapper::Wrapped( | ||
| "VaultViewClient is read-only; contract calls are not supported".to_string(), | ||
| )) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Read-only enforcement is clear; consider centralizing the error string.
The explicit failures in vault_call_with (Line 105+) / vault_call_returning (Line 122+) are good guardrails, and the message includes “read-only” which your test asserts on (Line 161-162).
To avoid future drift (e.g., someone edits one message and breaks bindings/tests), consider a single const READ_ONLY_ERR: &str = ... reused by both methods.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/vault/src/view_client.rs` around lines 104 - 133, Define a single
constant string (e.g., READ_ONLY_ERR: &str) containing "VaultViewClient is
read-only; contract calls are not supported" and replace the duplicated literal
in the impl methods vault_call_with and vault_call_returning to return
ErrorWrapper::Wrapped(READ_ONLY_ERR.to_string()); keep vault_call unchanged and
ensure any tests that assert this message use the same constant or the unchanged
text.
client/vault/src/view_client.rs
Outdated
Comment on lines
135
to
137
| crate::impl_view_cache_methods!(VaultViewClient); | ||
| crate::impl_vault_view_methods!(VaultViewClient); | ||
| crate::impl_vault_methods!(VaultViewClient); |
There was a problem hiding this comment.
Consider not exporting state-changing vault methods on VaultViewClient.
crate::impl_vault_methods!(VaultViewClient); (Line 137) likely exposes mutating calls (e.g., accept_guardian) on a type that is fundamentally read-only, and those will always error at runtime. For cross-language consumers this is a sharp edge: IDE auto-complete will suggest methods that can never succeed.
Options:
- Split macros so the view client only gets
impl_vault_view_methods!(+ cache methods), notimpl_vault_methods!, or - Rename the exported type/methods to make the “always errors” behavior unmissable (docstrings at the UniFFI boundary help).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/vault/src/view_client.rs` around lines 135 - 137, The VaultViewClient
currently gets mutating vault APIs via
crate::impl_vault_methods!(VaultViewClient) which exposes methods (e.g.,
accept_guardian) that always fail at runtime; remove that macro invocation so
VaultViewClient only receives crate::impl_vault_view_methods!(VaultViewClient)
and crate::impl_view_cache_methods!(VaultViewClient), or if you need to preserve
names for other types, split the impls into distinct macros (e.g.,
impl_vault_readonly_methods! vs impl_vault_methods!) and apply only the readonly
macro to VaultViewClient; also add a UniFFI-facing docstring on VaultViewClient
clarifying it is read-only so IDEs/users don’t expect successful state-changing
calls.
Comment on lines
98
to
103
| #[call(exec, tgas(30))] | ||
| pub fn resync_idle_balance(); | ||
|
|
||
| #[call(exec, tgas(30))] | ||
| pub fn refresh_idle_balance["resync_idle_balance"](); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a doc comment to clarify the alias relationship.
The new refresh_idle_balance function correctly aliases to the same contract method as resync_idle_balance. This creates two public functions that invoke identical behavior. If this duplication is intentional (e.g., for SDK naming preferences or backwards compatibility), a brief doc comment would help future maintainers understand why both exist.
📝 Suggested documentation
#[call(exec, tgas(30))]
pub fn resync_idle_balance();
+ /// Alias for `resync_idle_balance` - provided for SDK naming consistency.
#[call(exec, tgas(30))]
pub fn refresh_idle_balance["resync_idle_balance"]();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test-utils/src/controller/vault.rs` around lines 98 - 103, Add a brief doc
comment above the refresh_idle_balance declaration explaining that it is an
intentional alias of resync_idle_balance (both map to the same contract method)
and state the reason (e.g., SDK naming preference or backward compatibility) so
future maintainers understand the duplication; reference the functions
resync_idle_balance and refresh_idle_balance and the attribute
["resync_idle_balance"] in the comment.
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/vault/justfile`:
- Around line 39-52: The smoke-test-full Just recipe currently accepts
user_secret_key as a recipe argument (exposed in process lists and shell
history); remove user_secret_key from the recipe signature and instead require
USER_SECRET_KEY via environment. Modify the preflight check in smoke-test-full
to test -z "${USER_SECRET_KEY}" (and update the usage echo to not show the
secret on the command line), and in the invocation block pass
USER_SECRET_KEY="${USER_SECRET_KEY:-}" to the just python call; keep the other
env passthroughs (ALLOCATOR_ACCOUNT, UNDERLYING_TOKEN, etc.) unchanged and
reference the recipe name smoke-test-full and the env var USER_SECRET_KEY when
making these edits.
In `@client/vault/Makefile`:
- Around line 1-85: The repo prefers a Justfile instead of a Makefile; convert
the client/vault/Makefile targets (gen, python, smoke-test, smoke-test-view,
smoke-test-full) and variables (CRATE, LIB_NAME, DIST_DIR/PY_OUT, MODE,
UNIFFI_BINDGEN, DYLIB, CARGO_BUILD_FLAGS) into equivalent justfile recipes:
implement gen and python to run cargo build and uniffi-bindgen generate (using
DYLIB and PY_OUT), ensure python copies the dynamic library into PY_OUT,
recreate smoke-test, smoke-test-view and smoke-test-full recipes with the same
env var checks and usage messages (validate VAULT_ACCOUNT,
USER_ACCOUNT/USER_SECRET_KEY in smoke-test-full), and make directory creation
for PY_OUT; keep behavior identical (including MODE/release handling and
platform DYLIB resolution) and remove the Makefile or update docs to reference
the new Justfile.
In `@client/vault/README.md`:
- Around line 451-454: Reword the tooling/project-structure section to prefer
justfile over Makefile by listing "justfile" before "Makefile" and add a short
note indicating that "Makefile" is maintained for legacy/compatibility only;
update the file list lines that show "Makefile" and "justfile" (and the section
header "tooling/project-structure" if present) so the ordering is justfile first
and include a parenthetical or sentence mentioning Makefile = legacy/compat to
reflect the Templar-Protocol convention for the vault client SDK.
- Line 3: Replace the colloquial header line "gm ser - welcome to the Vault
SDK." with a professional, production-facing intro; locate that exact string in
the README and update it to a clear sentence such as "Welcome to the Vault SDK —
a secure, supported client for interacting with Vault services and secrets
management," ensuring tone is formal, grammatically correct, and suitable for
primary SDK documentation.
In `@client/vault/smoke_test.py`:
- Line 482: Several print statements in smoke_test.py use f-strings with no
placeholders (e.g., the prints that currently read `print(f" ⚠ Allocator client
NOT created (missing credentials)")` and the similar prints at the other flagged
locations); remove the unnecessary leading `f` from those string literals so
they become normal strings (e.g., `print(" ⚠ Allocator client NOT created
(missing credentials)")`) for all occurrences referenced (the prints at the
other flagged locations as well) while keeping the exact message text intact.
In `@client/vault/src/client.rs`:
- Around line 236-241: The FT transfer failure branches currently return
ErrorWrapper::Wrapped; update the failure handling in the ft_transfer_call and
ft_transfer code paths (the match arm catching
FinalExecutionStatus::Failure(err) and the other occurrence around lines
284-290) to return ErrorWrapper::TransactionFailed instead, including the
formatted on-chain error (e.g., format!("ft_transfer_call failed: {err:?}") or
similar) so the error variant matches storage_deposit/storage_deposit_on; ensure
the Unexpected execution status branch remains unchanged and keep the same
contextual message when wrapping the error.
In `@client/vault/src/key_pool/client.rs`:
- Around line 156-157: The tracing span on the constructor is logging the raw
rpc_url which may contain sensitive credentials; update the #[instrument(...)]
on the pub fn new(...) in client.rs to stop recording rpc_url directly—either
remove rpc_url from fields(...) or replace it with a non-sensitive
masked/derived value (e.g., host or a redact/mask of rpc_url) so that the
instrument attribute no longer emits the full rpc_url in tracing spans.
In `@client/vault/src/methods.rs`:
- Around line 176-199: get_vault_snapshot currently executes many independent
async getters sequentially, causing unnecessary latency; update
VaultClient::get_vault_snapshot to run independent calls in parallel (e.g., use
futures::try_join! or tokio::try_join!) for calls like get_configuration,
get_total_assets, get_last_total_assets, get_idle_balance, get_total_supply,
get_max_deposit, get_max_single_market_deposit, get_fee_anchor, get_fees,
get_restrictions, get_cap_groups, get_pending_governance_actions,
get_withdrawing_op_id, has_pending_market_withdrawal,
get_current_withdraw_request_id, queue_tail, peek_next_pending_withdrawal_id,
and list_markets_with_ids so you await a joined result and map the tuple into
VaultSnapshot, handling errors via the chosen try_join variant.
- Around line 201-223: The two functions resolve_market_ids and
resolve_market_accounts currently perform sequential awaits; change them to
spawn all lookup futures and await them together (preserving input order) using
futures::future::try_join_all (or your contract's batch query API) by mapping
markets -> get_market_id_of_account(m).await futures and market_ids ->
get_market_account_by_id(id) futures, collecting into a Vec<_> and awaiting
try_join_all to produce Result<Vec<Option<...>>, _>; add the appropriate use
import for futures::future::try_join_all and preserve capacity/ordering when
converting the joined results back to Vec.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
.gitignoreCargo.tomlclient/vault/Cargo.tomlclient/vault/Makefileclient/vault/README.mdclient/vault/justfileclient/vault/smoke_test.pyclient/vault/src/client.rsclient/vault/src/key_pool/client.rsclient/vault/src/key_pool/pool.rsclient/vault/src/lib.rsclient/vault/src/methods.rsclient/vault/src/view_client.rscommon/src/vault/wad.rstest-utils/src/controller/vault.rs
Comment on lines
1
to
85
| CRATE := templar-vault-client | ||
| LIB_NAME := templar_vault_client | ||
| DIST_DIR := dist | ||
| PY_OUT := $(DIST_DIR)/python | ||
|
|
||
| MODE ?= debug | ||
|
|
||
| UNIFFI_BINDGEN := cargo run -p $(CRATE) --bin uniffi-bindgen -- | ||
|
|
||
| ifeq ($(MODE),release) | ||
| CARGO_BUILD_FLAGS := --release | ||
| PROFILE := release | ||
| else | ||
| CARGO_BUILD_FLAGS := | ||
| PROFILE := debug | ||
| endif | ||
|
|
||
| ifeq ($(OS),Windows_NT) | ||
| DYLIB := ../../target/$(PROFILE)/$(LIB_NAME).dll | ||
| else | ||
| UNAME_S := $(shell uname -s) | ||
| ifeq ($(UNAME_S),Darwin) | ||
| DYLIB := ../../target/$(PROFILE)/lib$(LIB_NAME).dylib | ||
| else | ||
| DYLIB := ../../target/$(PROFILE)/lib$(LIB_NAME).so | ||
| endif | ||
| endif | ||
|
|
||
| .PHONY: gen python smoke-test smoke-test-view smoke-test-full | ||
|
|
||
| gen: | ||
| cargo build -p $(CRATE) $(CARGO_BUILD_FLAGS) | ||
| $(UNIFFI_BINDGEN) generate --language python --library "$(DYLIB)" --out-dir "$(PY_OUT)" --no-format | ||
|
|
||
| python: $(PY_OUT) | ||
| cargo build -p $(CRATE) $(CARGO_BUILD_FLAGS) | ||
| $(UNIFFI_BINDGEN) generate --language python --library "$(DYLIB)" --out-dir "$(PY_OUT)" --no-format | ||
| cp "$(DYLIB)" "$(PY_OUT)/" | ||
|
|
||
| # Basic smoke test (no env vars needed) | ||
| smoke-test: python | ||
| PYTHONPATH="$(PY_OUT)" python3 smoke_test.py | ||
|
|
||
| # View-only integration tests (read-only, requires VAULT_ACCOUNT) | ||
| # Usage: make smoke-test-view VAULT_ACCOUNT=myvault.testnet | ||
| smoke-test-view: python | ||
| @if [ -z "$(VAULT_ACCOUNT)" ]; then \ | ||
| echo "Usage: make smoke-test-view VAULT_ACCOUNT=<vault.testnet>"; \ | ||
| echo "Optional: UNDERLYING_TOKEN=<token.testnet>"; \ | ||
| exit 1; \ | ||
| fi | ||
| @PYTHONPATH="$(PY_OUT)" \ | ||
| VAULT_ACCOUNT="$(VAULT_ACCOUNT)" \ | ||
| UNDERLYING_TOKEN="$(UNDERLYING_TOKEN)" \ | ||
| RPC_URL="$(or $(RPC_URL),https://rpc.testnet.fastnear.com)" \ | ||
| RPC_API_KEY="$(RPC_API_KEY)" \ | ||
| python3 smoke_test.py | ||
|
|
||
| # Full happy path integration tests (requires funded accounts) | ||
| # Usage: make smoke-test-full VAULT_ACCOUNT=<vault> USER_ACCOUNT=<acc> USER_SECRET_KEY=<key> ... | ||
| smoke-test-full: python | ||
| @if [ -z "$(VAULT_ACCOUNT)" ] || [ -z "$(USER_ACCOUNT)" ] || [ -z "$(USER_SECRET_KEY)" ]; then \ | ||
| echo "Usage: make smoke-test-full \\"; \ | ||
| echo " VAULT_ACCOUNT=<vault.testnet> \\"; \ | ||
| echo " USER_ACCOUNT=<user.testnet> \\"; \ | ||
| echo " USER_SECRET_KEY='ed25519:...' \\"; \ | ||
| echo " [ALLOCATOR_ACCOUNT=<allocator.testnet>] \\"; \ | ||
| echo " [ALLOCATOR_SECRET_KEY='ed25519:...']"; \ | ||
| exit 1; \ | ||
| fi | ||
| @PYTHONPATH="$(PY_OUT)" \ | ||
| VAULT_ACCOUNT="$(VAULT_ACCOUNT)" \ | ||
| UNDERLYING_TOKEN="$(UNDERLYING_TOKEN)" \ | ||
| MARKET_ACCOUNT="$(MARKET_ACCOUNT)" \ | ||
| USER_ACCOUNT="$(USER_ACCOUNT)" \ | ||
| USER_SECRET_KEY="$(USER_SECRET_KEY)" \ | ||
| ALLOCATOR_ACCOUNT="$(ALLOCATOR_ACCOUNT)" \ | ||
| ALLOCATOR_SECRET_KEY="$(ALLOCATOR_SECRET_KEY)" \ | ||
| RPC_URL="$(or $(RPC_URL),https://rpc.testnet.fastnear.com)" \ | ||
| RPC_API_KEY="$(RPC_API_KEY)" \ | ||
| RUN_FULL_TESTS=1 \ | ||
| python3 smoke_test.py --full | ||
|
|
||
| $(PY_OUT): | ||
| mkdir -p "$(PY_OUT)" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider migrating to justfile.
The Makefile is functional and previous issues have been addressed (gen target now uses correct uniffi-bindgen generate subcommand, smoke-test-full validates USER_ACCOUNT). However, based on learnings from this repository, justfile is preferred over Makefile for build automation and tooling commands. Consider migrating these targets to a justfile for consistency with the repo's conventions.
Based on learnings: "In the Templar-Protocol/contracts repo, prefer using a Justfile over Makefile for build automation and tooling commands."
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 29-29: Missing required phony target "all"
(minphony)
[warning] 29-29: Missing required phony target "clean"
(minphony)
[warning] 29-29: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/vault/Makefile` around lines 1 - 85, The repo prefers a Justfile
instead of a Makefile; convert the client/vault/Makefile targets (gen, python,
smoke-test, smoke-test-view, smoke-test-full) and variables (CRATE, LIB_NAME,
DIST_DIR/PY_OUT, MODE, UNIFFI_BINDGEN, DYLIB, CARGO_BUILD_FLAGS) into equivalent
justfile recipes: implement gen and python to run cargo build and uniffi-bindgen
generate (using DYLIB and PY_OUT), ensure python copies the dynamic library into
PY_OUT, recreate smoke-test, smoke-test-view and smoke-test-full recipes with
the same env var checks and usage messages (validate VAULT_ACCOUNT,
USER_ACCOUNT/USER_SECRET_KEY in smoke-test-full), and make directory creation
for PY_OUT; keep behavior identical (including MODE/release handling and
platform DYLIB resolution) and remove the Makefile or update docs to reference
the new Justfile.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
client/vault/src/client.rs (1)
93-94:⚠️ Potential issue | 🟠 MajorStop logging raw
rpc_urlin constructor spans.Line 93, Line 103, Line 113, and Line 129 still include
fields(rpc_url = %rpc_url), which can leak embedded credentials/API tokens to logs.🔒 Proposed fix
- #[instrument(skip(credentials), fields(rpc_url = %rpc_url))] + #[instrument(skip(rpc_url, credentials))] @@ - #[instrument(skip(credential), fields(rpc_url = %rpc_url))] + #[instrument(skip(rpc_url, credential))] @@ - #[instrument(skip(credentials, config), fields(rpc_url = %rpc_url))] + #[instrument(skip(rpc_url, credentials, config))] @@ - #[instrument(skip(credential, config), fields(rpc_url = %rpc_url))] + #[instrument(skip(rpc_url, credential, config))]#!/bin/bash # Verify no constructor in VaultClient records raw rpc_url in tracing fields. rg -nP '#\[instrument\([^\)]*rpc_url\s*=\s*%rpc_url' client/vault/src/client.rs # Expected result after fix: # - No matches.Also applies to: 103-104, 113-114, 129-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/vault/src/client.rs` around lines 93 - 94, The constructor spans are logging the raw rpc_url (fields(rpc_url = %rpc_url)) which can leak secrets; edit each VaultClient constructor attribute (e.g., the #[instrument(...)] on new_key_pool_default and the other constructors such as new_key_pool, new_from_env, new) to remove the rpc_url field from the fields list (or replace it with a non-sensitive redacted/host-only field) so the raw URL is no longer emitted to tracing; update all occurrences so rg -nP '#\[instrument\([^\)]*rpc_url\s*=\s*%rpc_url' returns no matches.client/vault/Makefile (1)
69-77:⚠️ Potential issue | 🟠 MajorDo not instruct users to pass
USER_SECRET_KEYon themakecommand line.The usage on Line 69-Line 77 encourages a secret-in-history pattern. Please require pre-exported env usage and reject command-line origin for
USER_SECRET_KEY.🔐 Proposed fix
smoke-test-full: python + `@if` [ "$(origin USER_SECRET_KEY)" = "command line" ]; then \ + echo "ERROR: Do not pass USER_SECRET_KEY via make args."; \ + echo "Set it in environment first: export USER_SECRET_KEY='ed25519:...'"; \ + exit 1; \ + fi `@if` [ -z "$(VAULT_ACCOUNT)" ] || [ -z "$(USER_ACCOUNT)" ] || [ -z "$(USER_SECRET_KEY)" ]; then \ echo "Usage: make smoke-test-full \\"; \ echo " VAULT_ACCOUNT=<vault.testnet> \\"; \ echo " USER_ACCOUNT=<user.testnet> \\"; \ - echo " USER_SECRET_KEY='ed25519:...' \\"; \ + echo " USER_SECRET_KEY=<set in environment> \\"; \ echo " [ALLOCATOR_ACCOUNT=<allocator.testnet>] \\"; \ echo " [ALLOCATOR_SECRET_KEY='ed25519:...']"; \ exit 1; \ fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/vault/Makefile` around lines 69 - 77, Update the smoke-test-full Makefile target to stop encouraging passing secrets on the command line: remove USER_SECRET_KEY from the printed Usage text and add a runtime guard that rejects a command-line-provided secret by checking the make variable origin (use origin USER_SECRET_KEY) inside the smoke-test-full target; if origin is not "environment" emit an error asking the user to export USER_SECRET_KEY in their environment instead of supplying it on the make command line. Ensure the check references the smoke-test-full target and the USER_SECRET_KEY symbol so secrets are only accepted from exported env vars.client/vault/smoke_test.py (1)
97-104:⚠️ Potential issue | 🟠 MajorReplace literal
secret_key=strings to keep Ruff S106 green.Line 101 and Line 153 still pass hardcoded literals into
secret_key, which will keep triggering security lint errors.🧪 Proposed fix
+TEST_PLACEHOLDER_KEY = os.environ.get("SMOKE_TEST_PLACEHOLDER_KEY", "not-a-real-key") +TEST_INVALID_KEY = os.environ.get("SMOKE_TEST_INVALID_KEY", "not-a-valid-key") + def test_key_credential_creation(): """Test KeyCredential dataclass creation.""" cred = KeyCredential( account_id=AccountId("test.near"), - secret_key="not-a-real-key", + secret_key=TEST_PLACEHOLDER_KEY, ) @@ # Invalid secret key cred = KeyCredential( account_id=AccountId("test.near"), - secret_key="not-a-valid-key", + secret_key=TEST_INVALID_KEY, )Also applies to: 151-154
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/vault/smoke_test.py` around lines 97 - 104, The tests (e.g., test_key_credential_creation and the other KeyCredential test) currently pass a hardcoded secret_key string which triggers Ruff S106; replace the literal secret_key values with a non-literal generated value (for example use Python's secrets.token_hex() or a pytest fixture that returns a generated secret) and assign that to the KeyCredential secret_key parameter so the tests no longer contain hardcoded secret strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@client/vault/Makefile`:
- Around line 69-77: Update the smoke-test-full Makefile target to stop
encouraging passing secrets on the command line: remove USER_SECRET_KEY from the
printed Usage text and add a runtime guard that rejects a command-line-provided
secret by checking the make variable origin (use origin USER_SECRET_KEY) inside
the smoke-test-full target; if origin is not "environment" emit an error asking
the user to export USER_SECRET_KEY in their environment instead of supplying it
on the make command line. Ensure the check references the smoke-test-full target
and the USER_SECRET_KEY symbol so secrets are only accepted from exported env
vars.
In `@client/vault/smoke_test.py`:
- Around line 97-104: The tests (e.g., test_key_credential_creation and the
other KeyCredential test) currently pass a hardcoded secret_key string which
triggers Ruff S106; replace the literal secret_key values with a non-literal
generated value (for example use Python's secrets.token_hex() or a pytest
fixture that returns a generated secret) and assign that to the KeyCredential
secret_key parameter so the tests no longer contain hardcoded secret strings.
In `@client/vault/src/client.rs`:
- Around line 93-94: The constructor spans are logging the raw rpc_url
(fields(rpc_url = %rpc_url)) which can leak secrets; edit each VaultClient
constructor attribute (e.g., the #[instrument(...)] on new_key_pool_default and
the other constructors such as new_key_pool, new_from_env, new) to remove the
rpc_url field from the fields list (or replace it with a non-sensitive
redacted/host-only field) so the raw URL is no longer emitted to tracing; update
all occurrences so rg -nP '#\[instrument\([^\)]*rpc_url\s*=\s*%rpc_url' returns
no matches.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
client/vault/Cargo.tomlclient/vault/Makefileclient/vault/README.mdclient/vault/justfileclient/vault/smoke_test.pyclient/vault/src/client.rsclient/vault/src/key_pool/client.rsclient/vault/src/methods.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change is
@codeabbitai do an architecture diagram in my pr