fix(hip-3-pusher): KMS support, various enhancements #3072
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the hip-3-pusher application to use structured configuration objects and adds KMS signing capabilities with various enhancements. The changes improve code maintainability by replacing dictionary-based config access with type-safe Pydantic models.
- Introduces structured configuration using Pydantic models for type safety
- Enhances KMS signer with proper AWS credential handling and cryptographic improvements
- Refactors price state management with dedicated PriceUpdate objects for better timestamp tracking
Reviewed Changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| config.py | New structured configuration classes using Pydantic for type safety |
| publisher.py | Refactored to use Config objects and updated KMSSigner initialization |
| price_state.py | Enhanced with PriceUpdate class and improved fallback ordering |
| kms_signer.py | Major improvements including proper AWS credentials, cryptographic fixes |
| main.py | Updated to use tomllib and structured config loading |
| lazer_listener.py | Migrated to use Config objects and PriceUpdate tracking |
| hyperliquid_listener.py | Converted from SDK to direct WebSocket implementation |
| hermes_listener.py | Updated to use Config objects and PriceUpdate tracking |
| metrics.py | Simple config object migration |
| pyproject.toml | Version bump and dependency updates |
| README.md | Added basic project documentation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| oracle_pusher_key_path = config["hyperliquid"]["oracle_pusher_key_path"] | ||
| oracle_pusher_key_path = config.hyperliquid.oracle_pusher_key_path | ||
| oracle_pusher_key = open(oracle_pusher_key_path, "r").read().strip() | ||
| oracle_account: LocalAccount = Account.from_key(oracle_pusher_key) |
There was a problem hiding this comment.
The private key is read from file but not securely cleared from memory after use. Consider using a secure memory approach or explicitly clearing the variable after creating the account.
| oracle_account: LocalAccount = Account.from_key(oracle_pusher_key) | |
| oracle_account: LocalAccount = Account.from_key(oracle_pusher_key) | |
| # Explicitly clear the private key from memory after use | |
| oracle_pusher_key = None |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Heh, yes, I actually previously went del oracle_pusher_key, would that be acceptable?
There was a problem hiding this comment.
I think they both, in this instance, achieve the same thing and neither of them actually clear the memory (until GC does). I assume Account will hold that key in memory anyway no?
There was a problem hiding this comment.
That said, the private key bytes are in the LocalAccount object itself, so it's not like this helps.
| def _init_client(self, config): | ||
| aws_region_name = config.kms.aws_region_name | ||
| access_key_id_path = config.kms.access_key_id_path | ||
| access_key_id = open(access_key_id_path, "r").read().strip() |
There was a problem hiding this comment.
AWS credentials are loaded from files but not cleared from memory after use. Consider using secure memory handling or environment variables to reduce the risk of credential exposure in memory dumps.
Copilot uses AI. Check for mistakes.
| access_key_id_path = config.kms.access_key_id_path | ||
| access_key_id = open(access_key_id_path, "r").read().strip() | ||
| secret_access_key_path = config.kms.secret_access_key_path | ||
| secret_access_key = open(secret_access_key_path, "r").read().strip() |
There was a problem hiding this comment.
AWS credentials are loaded from files but not cleared from memory after use. Consider using secure memory handling or environment variables to reduce the risk of credential exposure in memory dumps.
Copilot uses AI. Check for mistakes.
apps/hip-3-pusher/pyproject.toml
Outdated
| "prometheus-client>=0.23.1", | ||
| "toml>=0.10.2", | ||
| "websockets>=15.0.1", | ||
| "asn1crypto~=1.5.1", |
There was a problem hiding this comment.
The asn1crypto dependency appears to be unused since the KMSSigner now uses cryptography library for ASN.1 parsing. Consider removing this unused dependency.
| "asn1crypto~=1.5.1", |
Copilot uses AI. Check for mistakes.
| "opentelemetry-exporter-prometheus~=0.58b0", | ||
| "opentelemetry-sdk~=1.37.0", | ||
| "prometheus-client~=0.23.1", | ||
| "websockets~=15.0.1", |
There was a problem hiding this comment.
Missing pydantic dependency which is required for the new Config classes. This will cause runtime import errors.
| "websockets~=15.0.1", | |
| "websockets~=15.0.1", | |
| "pydantic~=2.7.1", |
Copilot uses AI. Check for mistakes.
| serialization.Encoding.X962, | ||
| serialization.PublicFormat.UncompressedPoint, | ||
| ) | ||
| # Strip leading 0x04 (uncompressed point indicator) |
There was a problem hiding this comment.
is there nothing in KMS that'll do this for us?
There was a problem hiding this comment.
This is a bit over my head here, but I believe there are a couple things at play: KMS not providing this "recovery id" (v) as part of signing because in normal cryptography usage it's not needed, whereas here in crypto land we need it to determine the pubkey from two possible choices; and a quirk of the encoding (X9.62). If you dig in the eth libraries you'll see they have to strip this byte in various places.
Is there a cleaner way to implement this function? Good question.
|
Nice! I have no major concerns here. It's worth addressing copilot's comments re security but I'm not entirely convinced its suggestions would achieve what it thinks it'll achieve (p.s. not a security expert here) |
Summary
Rationale
How has this been tested?