feat(apps/argus): init argus #2417
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Skipped Deployments
|
…on for Pulse contract deployment
…emove unused fields
…lated configuration
…lified block number retrieval
…w RPC and contract address
There was a problem hiding this comment.
This is the first review on the overall structure. it looks good. I read README, tried to compile the program and faced some issues (see my comments). In the subsequent reviews I'll look at the business logic.
p.s: I also don't know how much of the functionality is similar to Fortuna but the structure looks identical. I'd recommend you reduce the redundancies as much as you can. At this point I'm also wondering whether we could unify these contracts and services entirly.
p.s 2: the existence of both config file and CLI arguments is a bit confusing to me and it seems that it's from fortuna. I'll try to understand it later.
| @@ -0,0 +1,18 @@ | |||
| ARG RUST_VERSION=1.82.0 | |||
There was a problem hiding this comment.
I recommend adding CI for Docker (and the code itself)
| WORKDIR /src | ||
| COPY apps/argus apps/argus | ||
| COPY pythnet pythnet | ||
| COPY target_chains/ethereum/entropy_sdk/solidity/abis target_chains/ethereum/entropy_sdk/solidity/abis |
There was a problem hiding this comment.
you'll probably need to add fortuna too
| - Parsing the response and converting it to the format expected by the Pulse contract | ||
| - Error handling and retries | ||
|
|
||
| ## Local Development |
There was a problem hiding this comment.
We need a deployed contract for this right? If so let's add it here.
For future: it's good if we can create a fully local deployment setup (that either creates a new ETH network or forks ETH and deploys required contracts)
| @@ -0,0 +1,413 @@ | |||
| /// Hermes Integration | |||
There was a problem hiding this comment.
Since this is a client for another API I think keeper might not be the best module for it. You can have an adaptor module for Hermes.
For future: Pyth Native, Lazer, and now Argus are all using Hermes in Rust and each created their own client for it. We should make a Rust client for Hermes!
| // contract in the same repo. | ||
| abigen!( | ||
| Pulse, | ||
| "../../target_chains/ethereum/contracts/out/IPulse.sol/IPulse.abi.json" |
There was a problem hiding this comment.
This seems like an external dependency. either manually move it in, or add it to the build script of this repo; also mention it in README
There was a problem hiding this comment.
and i'm wondering what built command you used, i needed to run forge build --extra-output-files abi
| # Hermes API configuration | ||
| hermes: | ||
| # Base URL for the Hermes API | ||
| # This can be overridden by setting the HERMES_BASE_URL environment variable |
There was a problem hiding this comment.
Why only this one supports env var? I saw it's defined in config. I recommend against having multiple definitions for a config.
| [lib] | ||
| name = "argus" | ||
| path = "src/lib.rs" |
There was a problem hiding this comment.
I doubt you need this section
|
|
||
| // Server TODO list: | ||
| // - Tests | ||
| // - Reduce memory requirements for storing hash chains to increase scalability |
There was a problem hiding this comment.
this seems to be from Fortuna
| @@ -0,0 +1,41 @@ | |||
| #![allow(clippy::just_underscores_and_digits)] | |||
| /// Maximum number of hashes to record in a request. | ||
| /// This should be set according to the maximum gas limit the provider supports for callbacks. | ||
| pub max_num_hashes: Option<u32>, | ||
|
|
||
| /// A list of delays (in blocks) that indicates how many blocks should be delayed | ||
| /// before we process a block. For retry logic, we can process blocks multiple times | ||
| /// at each specified delay. For example: [5, 10, 20]. | ||
| #[serde(default = "default_block_delays")] | ||
| pub block_delays: Vec<u64>, |
There was a problem hiding this comment.
Do we need these in Argus?
|
|
||
| #[arg(long = "url")] | ||
| #[arg(default_value = super::DEFAULT_HTTP_ADDR)] | ||
| pub url: Url, |
There was a problem hiding this comment.
aren't these in the config file?
| let multicall_exists = rpc_provider | ||
| .get_code(ethers::contract::MULTICALL_ADDRESS, None) | ||
| .await | ||
| .expect("Failed to get code") |
There was a problem hiding this comment.
does it mean that even if the address doesn't have a contract get_code won't return an Error and the length is 0? (If so it's an odd design)
| let latest_sequence_number = current_sequence_number.saturating_sub(1); | ||
| let mut current_request_number = latest_sequence_number; | ||
|
|
||
| println!("Latest sequence number: {}", current_request_number); |
There was a problem hiding this comment.
use tracing. i recommend following one unified logging style everywhere
| Ok(()) | ||
| } | ||
|
|
||
| async fn process_request(request: Request) -> Result<()> { |
There was a problem hiding this comment.
i suggest renamin it to inspect_request because we are not processing it, we are just dumping its details.
| #[arg(long = "price-ids")] | ||
| #[arg(env = "ARGUS_PRICE_IDS")] | ||
| #[arg(required = true)] | ||
| pub price_ids: String, |
There was a problem hiding this comment.
why haven't you made this a Vector of price ids to be simpler to parse?
| } | ||
|
|
||
| // Helper function to parse comma-separated hex strings into price IDs | ||
| fn parse_price_ids(price_ids_str: &str) -> Result<Vec<[u8; 32]>> { |
There was a problem hiding this comment.
i think if you have made this a vector of price feed ids (as pyth-sdk) you wouldn't need any of it
2 participants
Summary
This PR creates a new Pulse contract interface by adapting the structure from Fortuna (which was written for Entropy), implements new storage query methods, integrates Hermes API for price updates, and tests individual command functionality.
Rationale
These changes are necessary to:
How has this been tested?
So far, we have created the Pulse contract interface based on the Fortuna/Entropy structure and made the codebase compilable. We will test individual functions in subsequent PRs, focusing on: