Add Bankr signer support#16
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
hyperliquid-feedback | 3d5c1eb | May 20 2026, 07:31 PM |
There was a problem hiding this comment.
Pull request overview
This PR adds Bankr Wallet API as an additional signer backend in hyperliquid-cli, wiring it through CLI/runtime signer selection and exposing per-command Bankr capability metadata in the command registry/schema/catalog, along with documentation and contract tests.
Changes:
- Introduces a new
bankrsigner backend with wallet address resolution, typed-data signing, message signing, signature normalization, and recovered-address verification. - Adds
--bankr-signerto CLI/runtime resolution and threads Bankr selection through wallet commands and signer resolvers (while keeping raw Hyperliquid L1 signing fail-closed for Bankr). - Extends command metadata/schema/catalog + contract tests and documents Bankr signer usage and rollout plan.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/bankr.rs |
New Bankr Wallet API integration (resolve address, sign typed data/messages, normalize + verify signatures). |
src/signing.rs |
Adds SignerSource::Bankr / SignerBackend::Bankr and routes signing methods; raw L1 signing fails closed. |
src/auth.rs |
Adds resolve_bankr_signer to produce a ResolvedSigner backed by Bankr. |
src/resolvers.rs |
Extends signer resolver input with bankr_selector and resolves Bankr when explicitly selected. |
src/main.rs |
Adds --bankr-signer global flag and conflict rules with other signer options. |
src/cli_runtime.rs |
Propagates Bankr selector through runtime context, avoids private-key auto-resolution when Bankr is selected, and passes Bankr selector into wallet commands and signer resolution. |
src/commands/wallet.rs |
Adds Bankr-aware wallet show/address resolution and display labeling. |
src/command_registry.rs |
Adds BankrSupport enum + bankr_signer field to command contracts and catalog metadata plumbing. |
src/command_metadata.rs |
Adds bankr_signer inference rules for per-command capability classification. |
src/commands/schema.rs |
Emits bankr_signer in schema output surface. |
src/command_catalog.json |
Materializes bankr_signer metadata for commands in the catalog. |
tests/schema_contracts.rs |
Extends schema fixture assertions to include bankr_signer. |
tests/registry_contracts.rs |
Extends registry/schema drift assertions to include bankr_signer. |
tests/release_artifacts.rs |
Ensures tool catalog explicitly materializes bankr_signer metadata. |
README.md |
Documents Bankr signer usage and reinforces fail-closed raw L1 limitation; updates safety rules. |
docs/plans/bankr-signer-support.md |
Adds detailed Bankr signer rollout/implementation plan and constraints. |
src/lib.rs |
Exposes the new bankr module. |
Cargo.toml / Cargo.lock |
Enables reqwest blocking feature and updates lockfile for new dependency graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "dry_run": schema["dry_run"], | ||
| "raw_payload": schema["raw_payload"], | ||
| "confirmation": schema["confirmation"], | ||
| "ows_signer": schema["json_schema"]["x-hyperliquid"]["ows_signer"], | ||
| "bankr_signer": schema["json_schema"]["x-hyperliquid"]["bankr_signer"], | ||
| }) |
| assert_eq!( | ||
| command_value["ows_signer"], schema["json_schema"]["x-hyperliquid"]["ows_signer"], | ||
| "registry/schema drift for {command_key} field ows_signer" | ||
| ); | ||
| assert_eq!( | ||
| command_value["bankr_signer"], schema["json_schema"]["x-hyperliquid"]["bankr_signer"], | ||
| "registry/schema drift for {command_key} field bankr_signer" | ||
| ); |
| "dry_run": "not_supported", | ||
| "raw_payload": "unsupported", | ||
| "confirmation": "none", | ||
| "ows_signer": "address_selector_supported" | ||
| "ows_signer": "address_selector_supported", | ||
| "bankr_signer": "not_required" | ||
| }, |
| "dry_run": "optional", | ||
| "raw_payload": "dry_run_only", | ||
| "confirmation": "prompt", | ||
| "ows_signer": "experimental_feature_gated" | ||
| "ows_signer": "experimental_feature_gated", | ||
| "bankr_signer": "not_required" | ||
| }, |
| "confirmation": "none", | ||
| "ows_signer": "local_only" | ||
| "ows_signer": "local_only", | ||
| "bankr_signer": "not_required" |
| context: &'static str, | ||
| ) -> Result<T, CliError> { | ||
| let url = bankr_url(api_base_url, path)?; | ||
| let response = reqwest::blocking::Client::builder() | ||
| .timeout(DEFAULT_TIMEOUT) | ||
| .build() | ||
| .map_err(|err| CliError::Internal(anyhow::anyhow!(err)))? | ||
| .get(url) | ||
| .header("X-API-Key", api_key) | ||
| .send() | ||
| .map_err(|err| CliError::Unavailable(format!("Check your network connection. {err}")))?; |
| # Primary SDK for Hyperliquid API | ||
| hypersdk = "0.2" | ||
| reqwest = { version = "0.13", features = ["json"] } | ||
| reqwest = { version = "0.13", features = ["blocking", "json"] } |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| context: &'static str, | ||
| ) -> Result<T, CliError> { | ||
| let url = bankr_url(api_base_url, path)?; | ||
| let response = reqwest::blocking::Client::builder() |
There was a problem hiding this comment.
WARNING: New reqwest::blocking::Client created per request — should be reused for connection pooling
Each call to get_json and post_json builds a new blocking HTTP client with its own connection pool that is immediately discarded. For a CLI making multiple Bankr API calls (resolve address + sign), this means no TLS session reuse and no connection pooling. Consider building the client once in resolve_selector and storing it in BankrSigningConfig, or lazily initializing a shared client.
| } | ||
|
|
||
| fn is_hex_encoded_eip712_scalar(field_type: &str) -> bool { | ||
| field_type == "bytes" |
There was a problem hiding this comment.
SUGGESTION: Redundant check — field_type == "bytes" is already covered by field_type.starts_with("bytes") on line 402
| field_type == "bytes" | |
| fn is_hex_encoded_eip712_scalar(field_type: &str) -> bool { | |
| field_type.starts_with("bytes") | |
| || field_type.starts_with("uint") | |
| || field_type.starts_with("int") | |
| } |
| "not_required".to_string() | ||
| } | ||
|
|
||
| fn inferred_bankr_signer(command_key: &str, command: &impl CatalogCommandMetadata) -> String { |
There was a problem hiding this comment.
WARNING: inferred_bankr_signer is dead code — catalog JSON provides bankr_signer for all 112 commands
This function contains correct logic (e.g., returning unsupported_raw_l1_signing for auth-required commands, address_selector_supported for wallet show/address), but it is never invoked because every command in command_catalog.json already has an explicit bankr_signer: "not_required" value. Since all 112 catalog entries are "not_required", the nuanced inference logic is entirely unreachable.
This also means:
BankrSupport::TypedDataSupported,UnsupportedRawL1Signing,LocalOnly,NotApplicable, andAddressSelectorSupportedvariants are never assigned- Agents relying on schema metadata cannot determine which commands work with Bankr
- The fail-closed metadata promise from the plan document is not delivered
Either remove the catalog overrides to let inference run, or update the catalog entries to use the correct values from the inference function.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Other Observations (not in diff)Issues found in unchanged code or spanning many lines that cannot receive inline comments:
Changes Since Last ReviewNew commit
Files Reviewed (21 files)
Key Takeaway: The most important issue remains the catalog metadata. The Fix these issues in Kilo Cloud Reviewed by glm-5.1 · Incremental review from commit 4884e40 → fedb003 Reviewed by glm-5.1 · 303,868 tokens |
wtfsayo
force-pushed
the
feat/bankr-signer-support
branch
from
c45d0b7 to
ec75d36
Compare
wtfsayo
force-pushed
the
feat/bankr-signer-support
branch
from
7d80fe0 to
4884e40
Compare
Use async reqwest with block_in_place so --bankr-signer works inside the Tokio runtime, normalize EIP-712 integers for Bankr validation, and surface sign/400 failures with clearer auth errors until live signing is unblocked. Co-authored-by: Cursor <cursoragent@cursor.com>
| } | ||
| } | ||
|
|
||
| fn parse_bankr_integer_scalar(text: &str) -> Option<u64> { |
There was a problem hiding this comment.
WARNING: parse_bankr_integer_scalar returns Option<u64>, which cannot represent values above 2^64 - 1. EIP-712 types like uint256 can hold values up to 2^256 - 1. When a hex/decimal string exceeds u64::MAX, the parse silently fails (returns None), leaving the field as a string instead of a JSON number. If Bankr's API requires numeric JSON values for integer fields, large values will cause a runtime error downstream.
For Hyperliquid actions specifically, nonce fields (millisecond timestamps ~1.8×10¹²) fit in u64, but this function is called generically for all uint*/int* EIP-712 fields including amount and size which could theoretically exceed u64 in edge cases. Consider using a big-integer parser or at minimum documenting the u64 ceiling.
Classify bankr_signer per command (unsupported_raw_l1_signing for auth-required mutating commands, address_selector_supported for wallet show/address, local_only for prio bid, not_applicable for local-only) so registry inference is not overridden incorrectly. Regenerate characterization fixtures and drop redundant bytes type check. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
/wallet/meaddress resolution, typed-data signing, personal message signing, signature normalization, and recovered-address verification--bankr-signer defaultthrough CLI/runtime/auth/signer resolution and wallet address/show flowsBefore / After
Before:
After:
--bankr-signer defaultresolves the Bankr EVM wallet fromBANKR_API_KEYand can participate in supported typed-data/message signing pathsbankr_signermetadata for agent-readable command compatibilityVerification
cargo fmt --checkcargo test --lib bankr -- --nocapture— 7 passedgit diff --checkcargo check -qLive Smoke
Read-only API keyNotes
mainand the PR base is nowmain.