Skip to content

chore: make API types specific per chain#2079

Merged
DSharifi merged 8 commits intomainfrom
2074-revisit-the-response-format-of-foreign-rpc-requests
Feb 13, 2026
Merged

chore: make API types specific per chain#2079
DSharifi merged 8 commits intomainfrom
2074-revisit-the-response-format-of-foreign-rpc-requests

Conversation

@DSharifi
Copy link
Contributor

closes #2074

@DSharifi DSharifi linked an issue Feb 13, 2026 that may be closed by this pull request
@claude
Copy link

claude bot commented Feb 13, 2026

Code Review

I've analyzed this PR focusing on critical issues. Here are my findings:

⚠️ CRITICAL ISSUES

1. Missing SolanaExtractedValue Type - Data Model Incompleteness (crates/contract-interface/src/types/foreign_chain.rs:286-289)

The PR introduces chain-specific extracted value types (EvmExtractedValue, BitcoinExtractedValue) but is missing SolanaExtractedValue:

pub enum ExtractedValue {
    BitcoinExtractedValue(BitcoinExtractedValue),
    EvmExtractedValue(EvmExtractedValue),
    // Missing: SolanaExtractedValue variant
}

Impact:

  • Solana transaction verification cannot work properly - there's no way to represent Solana-specific extracted values
  • The test at line 654-680 uses EvmExtractedValue for a Solana request, which is semantically incorrect
  • This is a logic flaw that breaks the stated goal of making API types chain-specific

Required Fix:

  1. Add SolanaExtractedValue enum with appropriate variants (likely ProgramId(Hash256) and DataHash(Hash256) based on SolanaExtractor)
  2. Add SolanaExtractedValue variant to ExtractedValue enum
  3. Update the test at line 665-671 to use SolanaExtractedValue instead of EvmExtractedValue
  4. Update node implementation to construct SolanaExtractedValue when processing Solana requests

2. Missing Abstract Chain Handler (crates/contract-interface/src/types/foreign_chain.rs:74)

The PR adds Abstract(EvmRpcRequest) variant but there's no evidence this chain is handled anywhere:

pub enum ForeignChainRpcRequest {
    Abstract(EvmRpcRequest),  // New variant
    Ethereum(EvmRpcRequest),
    Solana(SolanaRpcRequest),
    Bitcoin(BitcoinRpcRequest),
}

Impact:

  • If users submit requests for the Abstract chain, the node code likely has no handler for it, causing panics or silent failures
  • Missing edge case handling

Required Fix:

  • Verify that all match statements on ForeignChainRpcRequest handle the Abstract variant
  • Check crates/node/src/providers/verify_foreign_tx/ for missing patterns

3. Backward Compatibility - Breaking Borsh Serialization (crates/contract-interface/src/types/foreign_chain.rs:97-100)

Adding the finality field to EvmRpcRequest changes Borsh serialization format:

pub struct EvmRpcRequest {
    pub tx_id: EvmTxId,
    pub extractors: Vec<EvmExtractor>,
    pub finality: EvmFinality,  // NEW FIELD
}

Impact:

  • During rolling updates, old nodes won't be able to deserialize new requests
  • ForeignTxSignPayload::compute_msg_hash() depends on Borsh serialization - different node versions will compute different hashes for the same logical request
  • Multi-cluster deployment will break during rollout

Required Fix:

  • This needs careful migration strategy or the PR should use a new request version (e.g., V2) instead of modifying existing types
  • Check if there's existing data in production that could be affected

📋 ADDITIONAL FINDINGS

4. Documentation Out of Sync (docs/foreign_chain_transactions.md)

The documentation still references old type names:

  • Line 110: EthereumRpcRequest (now EvmRpcRequest)
  • Line 123: Finality enum with Optimistic|Final (now split into EvmFinality/SolanaFinality with different variants)

Impact: Developer confusion, incorrect implementation by users

5. Incomplete Migration (crates/node/src/tests.rs:195)

The node tests still reference old Finality::Final instead of NEAR indexer-specific finality enum.


Recommendation

DO NOT MERGE until:

  1. SolanaExtractedValue type is added
  2. Backward compatibility strategy is confirmed for Borsh serialization changes
  3. All code paths handle the new Abstract chain variant
  4. Documentation is updated

⚠️

Copy link
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to update the response types in the design doc as well.

netrome
netrome previously approved these changes Feb 13, 2026
Copy link
Collaborator

@netrome netrome left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@gilcu3 gilcu3 force-pushed the 2074-revisit-the-response-format-of-foreign-rpc-requests branch from cdafe6b to 0a0dccb Compare February 13, 2026 11:35
@DSharifi DSharifi requested a review from netrome February 13, 2026 11:42
@DSharifi DSharifi enabled auto-merge February 13, 2026 11:42
@DSharifi DSharifi added this pull request to the merge queue Feb 13, 2026
Merged via the queue into main with commit ab324c1 Feb 13, 2026
9 checks passed
@DSharifi DSharifi deleted the 2074-revisit-the-response-format-of-foreign-rpc-requests branch February 13, 2026 12:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

revisit the response format of foreign RPC requests

3 participants