feat: add environment configuration, update Pyth SDK, implement optio…#17
feat: add environment configuration, update Pyth SDK, implement optio…#17devgreek wants to merge 1 commit into
Conversation
…nal mint decimals for HumidiFi, and improve bootstrap loading logic
📝 WalkthroughWalkthroughThe changes update Pyth dependency sources, improve Pyth key conversion, allow Humidifi to resolve missing mint decimals on-chain, and skip unsupported DEX entries during bootstrap loading. ChangesPyth integration updates
Humidifi mint configuration
Bootstrap filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. 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: 3
🧹 Nitpick comments (3)
crates/magnus/Cargo.toml (1)
47-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin git dependencies to a specific commit or tag for reproducibility.
Switching to the official
pyth-network/pyth-sdk-rsrepo is a good supply-chain improvement. However, neither dependency specifies arevortag, so builds will track the default branch and can break unexpectedly if the upstream API changes. Since thestate.rschanges already indicate API differences from the fork, pinning is especially important here.♻️ Suggested change
-pyth-sdk = { git = "https://github.com/pyth-network/pyth-sdk-rs", package = "pyth-sdk" } -pyth-sdk-solana = { git = "https://github.com/pyth-network/pyth-sdk-rs", package = "pyth-sdk-solana" } +pyth-sdk = { git = "https://github.com/pyth-network/pyth-sdk-rs", rev = "<commit-sha>", package = "pyth-sdk" } +pyth-sdk-solana = { git = "https://github.com/pyth-network/pyth-sdk-rs", rev = "<commit-sha>", package = "pyth-sdk-solana" }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/magnus/Cargo.toml` around lines 47 - 48, Pin both the pyth-sdk and pyth-sdk-solana git dependencies in Cargo.toml to the same reviewed commit or release tag from the official pyth-network/pyth-sdk-rs repository by adding a rev or tag constraint. Preserve the existing package names and repository URL while ensuring builds no longer follow the default branch.crates/magnus/src/bootstrap.rs (2)
25-26: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmpty
.expect("")messages hurt debuggability.Lines 26 and 30 use
.expect("")with an empty string. If these fail, the panic message provides no context. Use descriptive messages.♻️ Proposed fix
- let dex = Dex::from_str(dex_str).map_err(|e| eyre!(e)).expect(""); + let dex = Dex::from_str(dex_str).map_err(|e| eyre!(e)).expect("failed to parse DEX");- let cfg = HumidifiCfg::try_from(item).map_err(|e| eyre!(e)).expect(""); + let cfg = HumidifiCfg::try_from(item).map_err(|e| eyre!(e)).expect("failed to parse Humidifi config");Also applies to: 30-30
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/magnus/src/bootstrap.rs` around lines 25 - 26, Replace the empty expect messages in the Dex::from_str conversion and the corresponding failure at line 30 with concise, descriptive messages that identify the failed operation and relevant context, while preserving the existing error behavior.
36-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
warninstead ofinfofor skipped DEXes.A configured DEX being silently skipped may indicate a misconfiguration or stale config.
tracing::warn!would make these events more visible in operational dashboards.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/magnus/src/bootstrap.rs` at line 36, Update the logging call in the bootstrap DEX handling path to use tracing::warn! instead of tracing::info! when reporting skipped unsupported or non-bootstrapped DEXes, while preserving the existing message and dex context.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/magnus/src/adapters/amms/humidifi.rs`:
- Around line 99-106: Update the opt_dec fallback in the surrounding
token-decimal resolution flow to support both legacy SPL Token and Token-2022
mint accounts. Determine the mint’s owning program, keep
spl_token::state::Mint::unpack for legacy mints, and use the Token-2022 mint
parser for Token-2022 accounts so extensions do not cause failure; retain the
configured decimals path unchanged.
- Around line 114-115: Replace the three unwrap calls in the account-loading
flow around get_multiple_accounts and chroot.load_accounts with ok_or_else-based
error handling that returns an error identifying the specific missing key for
cfg.market, cfg.base_ta, or cfg.quote_ta. Preserve the existing account ordering
and propagate the resulting errors instead of panicking.
In `@crates/magnus/src/bootstrap.rs`:
- Around line 24-39: Update the Humidifi branch in the bootstrap loader to
handle `Humidifi::new(cfg, client)` failure without panicking: log the
initialization error and return `None` for that entry so subsequent entries
continue processing. Preserve successful initialization by returning the boxed
AMM as before.
---
Nitpick comments:
In `@crates/magnus/Cargo.toml`:
- Around line 47-48: Pin both the pyth-sdk and pyth-sdk-solana git dependencies
in Cargo.toml to the same reviewed commit or release tag from the official
pyth-network/pyth-sdk-rs repository by adding a rev or tag constraint. Preserve
the existing package names and repository URL while ensuring builds no longer
follow the default branch.
In `@crates/magnus/src/bootstrap.rs`:
- Around line 25-26: Replace the empty expect messages in the Dex::from_str
conversion and the corresponding failure at line 30 with concise, descriptive
messages that identify the failed operation and relevant context, while
preserving the existing error behavior.
- Line 36: Update the logging call in the bootstrap DEX handling path to use
tracing::warn! instead of tracing::info! when reporting skipped unsupported or
non-bootstrapped DEXes, while preserving the existing message and dex context.
🪄 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: b48d449b-7788-4d1b-af5a-89c7397e5c0e
📒 Files selected for processing (4)
crates/magnus/Cargo.tomlcrates/magnus/src/adapters/amms/humidifi.rscrates/magnus/src/adapters/amms/obric_v2/state.rscrates/magnus/src/bootstrap.rs
| let dec = match opt_dec { | ||
| Some(d) => d, | ||
| None => { | ||
| let account = client.get_account(&mint)?; | ||
| let mint_state = spl_token::state::Mint::unpack(&account.data)?; | ||
| mint_state.decimals | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
spl-token 9.0.0 Mint::unpack Token-2022 extensions compatibility
💡 Result:
In SPL Token 9.0.0, the Mint::unpack method remains designed specifically for the original SPL Token program's fixed-size 82-byte data structure [1][2]. It is not compatible with Token-2022 mint accounts that utilize extensions [3]. Key technical distinctions include: 1. Data Layout: The original SPL Token program stores mint data in a fixed 82-byte layout [1][2]. Token-2022 utilizes a Type-Length-Value (TLV) structure to support extensions, meaning mint accounts can have a variable size greater than 82 bytes [3][4]. 2. Unpacking Mechanism: Because Mint::unpack expects exactly 82 bytes, attempting to use it on a Token-2022 mint account (which contains additional TLV data) will fail if the data length validation is strictly enforced, or it will ignore the extension data entirely if only the first 82 bytes are read [4][1]. 3. Recommended Approach: To interact with Token-2022 mints, you must use the spl-token-2022 crate, which provides PodStateWithExtensions or equivalent helpers to correctly parse the base state along with the appended TLV extension data [3][4]. For developers needing a unified approach without deep dependencies, generic tools like spl-generic-token may be used for basic fields (supply, decimals) [5], but they intentionally exclude access to extensions. Any logic requiring extension support (e.g., transfer hooks, metadata, or confidential transfers) must utilize the spl-token-2022 library structures [3][4].
Citations:
- 1: https://github.com/solana-program/token/blob/program%40v9.0.0/interface/src/state.rs
- 2: https://docs.rs/spl-token-2022-interface/latest/src/spl_token_2022_interface/state.rs.html
- 3: https://taichiaudit.com/blog/solana-security-series-4
- 4: https://github.com/solana-labs/solana-program-library/blob/9c1ee63f97843d70d0f9c83b7b48e61775945614/token/js/src/state/mint.ts
- 5: https://docs.rs/crate/spl-generic-token/latest
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant region.
wc -l crates/magnus/src/adapters/amms/humidifi.rs
ast-grep outline crates/magnus/src/adapters/amms/humidifi.rs --view expanded
sed -n '1,180p' crates/magnus/src/adapters/amms/humidifi.rs
# Look for related mint-decimals handling and token-program branching in the repo.
rg -n "Mint::unpack|spl_token::state::Mint|Token-2022|token-2022|owner.*mint|decimals" crates/magnus/src -SRepository: LimeChain/magnus
Length of output: 12471
Handle Token-2022 mints here too
Mint::unpack only reads the legacy 82-byte mint layout, so any reserve mint with Token-2022 extensions will fail when decimals is omitted. Either require decimals in config for those mints or branch on the mint program and parse Token-2022 state accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/magnus/src/adapters/amms/humidifi.rs` around lines 99 - 106, Update
the opt_dec fallback in the surrounding token-decimal resolution flow to support
both legacy SPL Token and Token-2022 mint accounts. Determine the mint’s owning
program, keep spl_token::state::Mint::unpack for legacy mints, and use the
Token-2022 mint parser for Token-2022 accounts so extensions do not cause
failure; retain the configured decimals path unchanged.
| let accs = client.get_multiple_accounts(&[cfg.market, cfg.base_ta, cfg.quote_ta])?; | ||
| chroot.load_accounts(vec![(cfg.market, accs[0].clone().unwrap()), (cfg.base_ta, accs[1].clone().unwrap()), (cfg.quote_ta, accs[2].clone().unwrap())])?; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and inspect the relevant section with line numbers.
git ls-files crates/magnus/src/adapters/amms/humidifi.rs
wc -l crates/magnus/src/adapters/amms/humidifi.rs
sed -n '90,140p' crates/magnus/src/adapters/amms/humidifi.rs
# Find the surrounding types and helper methods involved in this path.
rg -n "get_multiple_accounts|load_accounts|struct .*Config|impl .*humidifi|new\(" crates/magnus/src/adapters/amms/humidifi.rs crates/magnus/src -g '!target'Repository: LimeChain/magnus
Length of output: 15133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the bootstrap path and account-loading helper to understand how failures surface.
sed -n '1,120p' crates/magnus/src/bootstrap.rs
sed -n '110,150p' crates/magnus/src/adapters/amms.rsRepository: LimeChain/magnus
Length of output: 4459
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm every call site of Humidifi::new and how its Result is handled.
rg -n "Humidifi::new\(" crates/magnus/src
rg -n "unable to initialise humidifi|expect\\(\"unable to initialise humidifi\"\\)" crates/magnus/srcRepository: LimeChain/magnus
Length of output: 426
Replace the unwrap()s with error handling for missing accounts. get_multiple_accounts can return None for any of cfg.market, cfg.base_ta, or cfg.quote_ta; using ok_or_else(...) here would make startup failures point to the missing key instead of panicking.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/magnus/src/adapters/amms/humidifi.rs` around lines 114 - 115, Replace
the three unwrap calls in the account-loading flow around get_multiple_accounts
and chroot.load_accounts with ok_or_else-based error handling that returns an
error identifying the specific missing key for cfg.market, cfg.base_ta, or
cfg.quote_ta. Preserve the existing account ordering and propagate the resulting
errors instead of panicking.
| .filter_map(|item| -> Option<Box<dyn Amm>> { | ||
| let dex_str = item.get("dex").and_then(|dex| dex.as_str()).expect("no DEX provided"); | ||
| let dex = Dex::from_str(dex_str).map_err(|e| eyre!(e)).expect(""); | ||
|
|
||
| let init: Box<dyn Amm> = match dex { | ||
| match dex { | ||
| Dex::HumidiFi => { | ||
| let cfg = HumidifiCfg::try_from(item).map_err(|e| eyre!(e)).expect(""); | ||
| let amm = Humidifi::new(cfg, client).expect("unable to initialise humidifi"); | ||
|
|
||
| Box::new(amm) | ||
| Some(Box::new(amm) as Box<dyn Amm>) | ||
| } | ||
| _ => panic!("Unsupported DEX: {}", dex), | ||
| }; | ||
|
|
||
| init | ||
| _ => { | ||
| tracing::info!("Skipping unsupported/non-bootstrapped DEX in bootstrap load: {}", dex); | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Humidifi::new panic on transient RPC failure undermines the resilience goal.
The PR prevents panics for unsupported DEX types, but line 31's .expect("unable to initialise humidifi") will still panic if Humidifi::new fails. Since new makes live RPC calls (client.get_account, client.get_multiple_accounts), a transient network error or missing account on a single entry will abort the entire bootstrap, skipping all subsequent entries. Consider logging and returning None (or propagating the error) instead of panicking.
🛡️ Proposed fix: skip failed Humidifi entries instead of panicking
match dex {
Dex::HumidiFi => {
- let cfg = HumidifiCfg::try_from(item).map_err(|e| eyre!(e)).expect("");
- let amm = Humidifi::new(cfg, client).expect("unable to initialise humidifi");
-
- Some(Box::new(amm) as Box<dyn Amm>)
+ match HumidifiCfg::try_from(item).map_err(|e| eyre!(e)) {
+ Ok(cfg) => match Humidifi::new(cfg, client) {
+ Ok(amm) => Some(Box::new(amm) as Box<dyn Amm>),
+ Err(e) => {
+ tracing::warn!("Failed to initialise Humidifi AMM: {}", e);
+ None
+ }
+ },
+ Err(e) => {
+ tracing::warn!("Failed to parse Humidifi config: {}", e);
+ None
+ }
+ }
}📝 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.
| .filter_map(|item| -> Option<Box<dyn Amm>> { | |
| let dex_str = item.get("dex").and_then(|dex| dex.as_str()).expect("no DEX provided"); | |
| let dex = Dex::from_str(dex_str).map_err(|e| eyre!(e)).expect(""); | |
| let init: Box<dyn Amm> = match dex { | |
| match dex { | |
| Dex::HumidiFi => { | |
| let cfg = HumidifiCfg::try_from(item).map_err(|e| eyre!(e)).expect(""); | |
| let amm = Humidifi::new(cfg, client).expect("unable to initialise humidifi"); | |
| Box::new(amm) | |
| Some(Box::new(amm) as Box<dyn Amm>) | |
| } | |
| _ => panic!("Unsupported DEX: {}", dex), | |
| }; | |
| init | |
| _ => { | |
| tracing::info!("Skipping unsupported/non-bootstrapped DEX in bootstrap load: {}", dex); | |
| None | |
| } | |
| } | |
| .filter_map(|item| -> Option<Box<dyn Amm>> { | |
| let dex_str = item.get("dex").and_then(|dex| dex.as_str()).expect("no DEX provided"); | |
| let dex = Dex::from_str(dex_str).map_err(|e| eyre!(e)).expect(""); | |
| match dex { | |
| Dex::HumidiFi => { | |
| match HumidifiCfg::try_from(item).map_err(|e| eyre!(e)) { | |
| Ok(cfg) => match Humidifi::new(cfg, client) { | |
| Ok(amm) => Some(Box::new(amm) as Box<dyn Amm>), | |
| Err(e) => { | |
| tracing::warn!("Failed to initialise Humidifi AMM: {}", e); | |
| None | |
| } | |
| }, | |
| Err(e) => { | |
| tracing::warn!("Failed to parse Humidifi config: {}", e); | |
| None | |
| } | |
| } | |
| } | |
| _ => { | |
| tracing::info!("Skipping unsupported/non-bootstrapped DEX in bootstrap load: {}", dex); | |
| None | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/magnus/src/bootstrap.rs` around lines 24 - 39, Update the Humidifi
branch in the bootstrap loader to handle `Humidifi::new(cfg, client)` failure
without panicking: log the initialization error and return `None` for that entry
so subsequent entries continue processing. Preserve successful initialization by
returning the boxed AMM as before.
1 participant
A JSON formatting mismatch in cfg/payloads/pmms.json where reserve_mints was provided as a string address array for some markets rather than an array of [address, decimals]. I also resolved the subsequent panic: Unsupported DEX: tessera by skipping non-bootstrapped DEXes.
Summary by CodeRabbit
New Features
Bug Fixes
Compatibility