Conversation
| dtos::ForeignChain::Abstract | ||
| | dtos::ForeignChain::Solana |
There was a problem hiding this comment.
This is the broken-window fix
There was a problem hiding this comment.
strange that we missed this, didn't we have an end2end test for Abstract?
| "request": { | ||
| "request": { | ||
| "Starknet": { | ||
| "tx_id": "025c84e568751d2e3189357b3f48690e4e987084204bc82ebde1d51ec3948fd8", |
There was a problem hiding this comment.
This will probably not work after some time as starknet RPCs may not serve all txs forever, but adding some "refresh" or query script to generate these args felt out of scope for now.
83ba95f to
4268e58
Compare
|
PR title type suggestion: This PR modifies a source code file ( |
|
PR title type suggestion: This PR modifies source code in |
Code ReviewThis PR adds Abstract and Starknet RPC configuration to the localnet guide, adds a Starknet foreign tx verification args file, and fixes a bug where Potential Issue
The Since the enum is
This is not a merge blocker given the existing code already had this pattern, but it's worth addressing since this PR is literally fixing the exact class of bug this pattern enables. Minor noteThe test No other critical issues found. The docs changes, config template updates, and the bug fix with its accompanying test all look correct. ✅ |
Nah, that's a broken-window fix. Could be broken out but this is mainly a docs PR. |
Thanks, you make some good points - but the concrete suggestions doesn't seem to improve the situation as it would still be easy to "forget" about this change and they don't provide compile time guarantees. We should focus on better and more comprehensive testing instead imo. |
| pub struct ForeignChainsConfig { | ||
| #[serde(default)] | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub solana: Option<SolanaChainConfig>, | ||
| #[serde(default)] | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub bitcoin: Option<BitcoinChainConfig>, | ||
| #[serde(default)] | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub ethereum: Option<EthereumChainConfig>, | ||
| #[serde(default)] | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| #[serde(rename = "abstract")] | ||
| pub abstract_chain: Option<AbstractChainConfig>, | ||
| #[serde(default)] | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub starknet: Option<StarknetChainConfig>, |
There was a problem hiding this comment.
Another unrelated broken-window fix. I noted a bunch of ethereum: null and solana: null fields appearing in the config. Don't know where we write to the config file, but let's primarily just not add these.
There was a problem hiding this comment.
Ah I see we added this for the web_ui host+port to SocketAddr change, that explains why we're writing to the config. Fair enough.
|
PR title type suggestion: This PR modifies source code files (crates/node/src/) in addition to documentation, so the type prefix should probably be |
|
PR title type suggestion: This PR adds new source code functionality for Abstract chain RPC configuration support across multiple files (foreign_chains.rs, coordinator.rs). While documentation is included, the primary intent is implementing a new feature. The type prefix should probably be Suggested title: |
| # Keep generated YAML tags intact by editing only the trailing `foreign_chains` section. | ||
| config_text = ( | ||
| re.sub(r"\nforeign_chains:\n[\s\S]*\Z", "\n", config_text).rstrip() + "\n" | ||
| re.sub(r"\nforeign_chains:[\s\S]*\Z", "\n", config_text).rstrip() + "\n" |
There was a problem hiding this comment.
Needed because the serialization format changed now. This would look like foreign_chains: {}
closes #2177
Additionally, this PR also adds configuration for Starknet and two small broken-window fixes: