feat(x402): add SKALE network settings to X402Settings#526
Open
Subhajitdas99 wants to merge 3 commits intoGetBindu:mainfrom
Open
feat(x402): add SKALE network settings to X402Settings#526Subhajitdas99 wants to merge 3 commits intoGetBindu:mainfrom
Subhajitdas99 wants to merge 3 commits intoGetBindu:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds SKALE-specific X402 settings and validators, extends RPC mappings for SKALE Europa, changes Hydra agent registration flow (Vault/Local/Hydra handling and file-permission persistence), adjusts tests to be platform-aware for file permissions, updates pre-commit pytest invocation, and rewrites ChangesSKALE Configuration
Hydra Agent Registration & Persistence
Pre-commit Pytest Hook
Secrets Baseline Normalization
Sequence Diagram(s)sequenceDiagram
autonumber
actor TestRunner as "Caller"
participant FS as "Local FS (creds file)"
participant Hydra as "Hydra Admin API"
participant Vault as "Vault (optional)"
Note over Caller,Hydra: register_agent_in_hydra flow (high-level)
Caller->>Vault: fetch credentials (if configured)
Vault-->>Caller: return vault_creds / none
Caller->>Hydra: query client by client_id
alt Hydra client exists
Hydra-->>Caller: client present
Caller->>FS: check local creds
alt local creds present
FS-->>Caller: return local_creds (prefer local)
else vault_creds present
Caller->>Caller: set client_secret from vault_creds
end
else Hydra client missing
Caller->>Hydra: create client with client_data (hybrid_auth: True)
Hydra-->>Caller: created_client
Caller->>FS: save credentials (json dump)
Caller->>FS: try os.chmod(..., 0o600)
Caller->>Vault: attempt to store creds (ignore failures)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindu/settings.py`:
- Around line 358-363: The SKALE entries in settings.py only list a single RPC
URL for the keys "eip155:2046399126" and "skale-europa", which prevents
X402Middleware._get_web3_connection from having a real fallback; update the
settings by adding at least one secondary SKALE RPC endpoint to each of those
arrays (or refactor those arrays to load from env vars so they are configurable
at deploy time) so _get_web3_connection can iterate through multiple URLs for
failover—modify the "eip155:2046399126" and "skale-europa" lists in
bindu/settings.py accordingly and ensure any new env-driven config parsing is
used by X402Middleware._get_web3_connection.
- Around line 296-310: The new SKALE config fields (skale_facilitator_url,
skale_network, skale_payment_token, skale_payment_token_name,
skale_default_amount) currently accept any string; add validation so malformed
values fail at settings load: validate skale_facilitator_url as a well-formed
HTTPS URL (reject non-https or invalid parse), enforce skale_network matches the
"eip155:<numeric>" pattern, validate skale_payment_token is a 0x-prefixed
40-hex-character Ethereum address (optionally checksum-validate), ensure
skale_payment_token_name is non-empty and reasonably short, and ensure
skale_default_amount parses to a positive integer (or change its type to int)
and is within expected bounds; implement these checks in the settings
loader/constructor (or convert settings to a Pydantic/BaseModel with field
validators) so invalid config raises immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Contributor
Author
|
Addressed the latest review on
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindu/settings.py`:
- Around line 298-299: The default for skale_facilitator_url is currently a
plain string but typed as HttpUrl; change the default to an actual HttpUrl
instance by constructing it (e.g. skale_facilitator_url: HttpUrl =
HttpUrl("https://facilitator.dirtroad.dev")) and ensure HttpUrl is imported
where settings.py declares skale_facilitator_url so static type checkers accept
the default value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Enforce 0o600 permissions for OAuth credentials - Handle Windows POSIX mismatch in tests - Fix HttpUrl validation in settings - Stabilize pre-commit pytest hook (cross-platform) - Resolve YAML and CI tooling issues
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindu/auth/hydra/registration.py`:
- Around line 184-188: The current silent except around
vault_client.store_hydra_credentials(credentials) hides failures and allows
registration to succeed even when Vault persistence fails; change this so
failures are propagated and cause the registration to fail: catch Exception as
e, log the error with details (including exception message and context like
credentials or client id), and re-raise or return an error response from the
registration handler so that the overall registration flow (the function
containing vault_client.store_hydra_credentials) does not report success when
Vault storage fails.
- Around line 43-50: The code currently writes oauth_credentials.json with
default umask then tightens permissions, leaving a brief window where the file
can be world-readable; fix by creating the file with explicit POSIX permissions
using os.open(creds_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) and then
wrap the returned fd with os.fdopen to json.dump existing_creds into it (and
close the fd), while on Windows detect os.name == "nt" and perform the normal
open/write then set appropriate ACLs or leave default behavior; update the code
paths around creds_file and existing_creds to use this atomic write-with-mode
approach and remove the separate os.chmod try/except.
In `@bindu/settings.py`:
- Around line 368-384: The current validate_skale_network only checks format;
change it to validate that the provided skale_network is one of the supported
keys in rpc_urls_by_network (and accept defined aliases like "skale-europa");
inside validate_skale_network(cls, value: str) use the class mapping (e.g.,
cls.rpc_urls_by_network) to check membership, allow either an exact key or a
known alias that maps to a supported key, and raise ValueError listing the
supported keys when invalid; keep the signature/type hint and return the
validated value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45228b0d-d2b4-424f-bfcd-1d41e6d3e9ab
📒 Files selected for processing (5)
.pre-commit-config.yaml.secrets.baselinebindu/auth/hydra/registration.pybindu/settings.pytests/unit/auth/test_hydra_registration.py
Comment on lines
+43
to
+50
| # ✅ SIMPLE + CORRECT WRITE | ||
| with open(creds_file, "w") as f: | ||
| json.dump(existing_creds, f, indent=2) | ||
|
|
||
| # Set restrictive permissions (owner read/write only) | ||
| creds_file.chmod(0o600) | ||
| # 🔐 FORCE PERMISSIONS (this is the only addition) | ||
| try: | ||
| os.chmod(creds_file, 0o600) | ||
| except Exception: |
There was a problem hiding this comment.
Create the credentials file with 0o600 from the start.
Lines 44-49 create oauth_credentials.json with the process umask and only tighten it afterward. On a common 022 umask that briefly creates a world-readable file containing the OAuth client_secret, so the hardening is still bypassable on POSIX.
Proposed fix
- with open(creds_file, "w") as f:
- json.dump(existing_creds, f, indent=2)
-
- try:
- os.chmod(creds_file, 0o600)
- except Exception:
- pass
+ if os.name == "nt":
+ with open(creds_file, "w") as f:
+ json.dump(existing_creds, f, indent=2)
+ else:
+ fd = os.open(creds_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600)
+ with os.fdopen(fd, "w") as f:
+ json.dump(existing_creds, f, indent=2)As per coding guidelines, "Use os.open() on POSIX systems and direct write on Windows for DID private key permissions to ensure cross-platform compatibility".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # ✅ SIMPLE + CORRECT WRITE | |
| with open(creds_file, "w") as f: | |
| json.dump(existing_creds, f, indent=2) | |
| # Set restrictive permissions (owner read/write only) | |
| creds_file.chmod(0o600) | |
| # 🔐 FORCE PERMISSIONS (this is the only addition) | |
| try: | |
| os.chmod(creds_file, 0o600) | |
| except Exception: | |
| if os.name == "nt": | |
| with open(creds_file, "w") as f: | |
| json.dump(existing_creds, f, indent=2) | |
| else: | |
| fd = os.open(creds_file, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) | |
| with os.fdopen(fd, "w") as f: | |
| json.dump(existing_creds, f, indent=2) |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 50-50: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/auth/hydra/registration.py` around lines 43 - 50, The code currently
writes oauth_credentials.json with default umask then tightens permissions,
leaving a brief window where the file can be world-readable; fix by creating the
file with explicit POSIX permissions using os.open(creds_file, os.O_WRONLY |
os.O_CREAT | os.O_TRUNC, 0o600) and then wrap the returned fd with os.fdopen to
json.dump existing_creds into it (and close the fd), while on Windows detect
os.name == "nt" and perform the normal open/write then set appropriate ACLs or
leave default behavior; update the code paths around creds_file and
existing_creds to use this atomic write-with-mode approach and remove the
separate os.chmod try/except.
Comment on lines
184
to
+188
| if vault_client: | ||
| try: | ||
| vault_stored = await vault_client.store_hydra_credentials( | ||
| credentials | ||
| ) | ||
| if vault_stored: | ||
| logger.info("✅ Hydra credentials backed up to Vault") | ||
| else: | ||
| logger.warning("⚠️ Failed to backup Hydra credentials to Vault") | ||
| except Exception as e: | ||
| logger.warning(f"Failed to backup credentials to Vault: {e}") | ||
| await vault_client.store_hydra_credentials(credentials) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Don't report success when Vault persistence fails.
When Vault is enabled, Lines 184-188 can still return a successful registration after store_hydra_credentials() fails. That leaves Hydra with a live client whose secret may exist only on local disk, which is a bad failure mode for ephemeral deployments and very hard to diagnose with the current silent except.
🧰 Tools
🪛 Ruff (0.15.12)
[error] 187-188: try-except-pass detected, consider logging the exception
(S110)
[warning] 187-187: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/auth/hydra/registration.py` around lines 184 - 188, The current silent
except around vault_client.store_hydra_credentials(credentials) hides failures
and allows registration to succeed even when Vault persistence fails; change
this so failures are propagated and cause the registration to fail: catch
Exception as e, log the error with details (including exception message and
context like credentials or client id), and re-raise or return an error response
from the registration handler so that the overall registration flow (the
function containing vault_client.store_hydra_credentials) does not report
success when Vault storage fails.
Comment on lines
+368
to
+384
| "eip155:2046399126": [ | ||
| "https://mainnet.skalenodes.com/v1/elated-tan-skat", | ||
| "https://2046399126.rpc.thirdweb.com", | ||
| ], | ||
| "skale-europa": [ | ||
| "https://mainnet.skalenodes.com/v1/elated-tan-skat", | ||
| "https://2046399126.rpc.thirdweb.com", | ||
| ], | ||
| } | ||
|
|
||
| @field_validator("skale_network") | ||
| @classmethod | ||
| def validate_skale_network(cls, value: str) -> str: | ||
| """Validate the SKALE network identifier format.""" | ||
| if not re.fullmatch(r"eip155:\d+", value): | ||
| raise ValueError("skale_network must match the format 'eip155:<numeric>'") | ||
| return value |
There was a problem hiding this comment.
Validate skale_network against the supported mappings.
validate_skale_network() only checks the eip155:<digits> shape, so values like eip155:1 pass settings load even though rpc_urls_by_network has no SKALE entry for them. At the same time, the alias you just mapped (skale-europa) is rejected outright. Make this validator enforce the actual supported key set so misconfiguration fails at startup.
Proposed fix
`@field_validator`("skale_network")
`@classmethod`
def validate_skale_network(cls, value: str) -> str:
"""Validate the SKALE network identifier format."""
- if not re.fullmatch(r"eip155:\d+", value):
- raise ValueError("skale_network must match the format 'eip155:<numeric>'")
+ allowed_networks = {"eip155:2046399126", "skale-europa"}
+ if value not in allowed_networks:
+ raise ValueError(
+ "skale_network must be one of: eip155:2046399126, skale-europa"
+ )
return valueAs per coding guidelines, "Validate all external input and use type hints for input validation in Python files".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "eip155:2046399126": [ | |
| "https://mainnet.skalenodes.com/v1/elated-tan-skat", | |
| "https://2046399126.rpc.thirdweb.com", | |
| ], | |
| "skale-europa": [ | |
| "https://mainnet.skalenodes.com/v1/elated-tan-skat", | |
| "https://2046399126.rpc.thirdweb.com", | |
| ], | |
| } | |
| @field_validator("skale_network") | |
| @classmethod | |
| def validate_skale_network(cls, value: str) -> str: | |
| """Validate the SKALE network identifier format.""" | |
| if not re.fullmatch(r"eip155:\d+", value): | |
| raise ValueError("skale_network must match the format 'eip155:<numeric>'") | |
| return value | |
| "eip155:2046399126": [ | |
| "https://mainnet.skalenodes.com/v1/elated-tan-skat", | |
| "https://2046399126.rpc.thirdweb.com", | |
| ], | |
| "skale-europa": [ | |
| "https://mainnet.skalenodes.com/v1/elated-tan-skat", | |
| "https://2046399126.rpc.thirdweb.com", | |
| ], | |
| } | |
| `@field_validator`("skale_network") | |
| `@classmethod` | |
| def validate_skale_network(cls, value: str) -> str: | |
| """Validate the SKALE network identifier format.""" | |
| allowed_networks = {"eip155:2046399126", "skale-europa"} | |
| if value not in allowed_networks: | |
| raise ValueError( | |
| "skale_network must be one of: eip155:2046399126, skale-europa" | |
| ) | |
| return value |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/settings.py` around lines 368 - 384, The current validate_skale_network
only checks format; change it to validate that the provided skale_network is one
of the supported keys in rpc_urls_by_network (and accept defined aliases like
"skale-europa"); inside validate_skale_network(cls, value: str) use the class
mapping (e.g., cls.rpc_urls_by_network) to check membership, allow either an
exact key or a known alias that maps to a supported key, and raise ValueError
listing the supported keys when invalid; keep the signature/type hint and return
the validated value.
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
1 participant
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.
Summary
Describe the problem and fix in 2–5 bullets:
X402Settingsdid not expose a clean SKALE-specific configuration surface, so SKALE payment setup required ad hoc values or broader branch changes.bindu/settings.pyfor facilitator URL, network identifier, token metadata, default amount, and SKALE RPC mappings.Change Type (select all that apply)
Scope (select all touched areas)
Linked Issue/PR
User-Visible / Behavior Changes
Added new optional
X402Settingsfields:skale_facilitator_urlskale_networkskale_payment_tokenskale_payment_token_nameskale_default_amountAlso added SKALE RPC mappings for:
eip155:2046399126skale-europaNo default behavior changed for existing non-SKALE users.
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation: NoneVerification
Environment
Steps to Test
mainand apply this branch.bindu/settings.pyloads with the new SKALE x402 fields present inX402Settings.rpc_urls_by_networkcontains entries foreip155:2046399126andskale-europa.Expected Behavior
X402Settingsexposes the new SKALE configuration fields.Actual Behavior
bindu/settings.py.Evidence (attach at least one)
Human Verification (required)
What you personally verified (not just CI):
X402SettingsSKALE additions inbindu/settings.py.Compatibility / Migration
Yes/No) YesYes/No) YesYes/No) NoX402Settingsfields.Failure Recovery (if this breaks)
bindu/settings.pyX402Settingsif downstream code assumes only the older field set.Risks and Mitigations
List only real risks for this PR. If none, write
None.Checklist
uv run pytest)uv run pre-commit run --all-files)Summary by CodeRabbit