Skip to content

chore: add From and TryFrom conversions between dto and foreign chain inspector types#2104

Merged
DSharifi merged 5 commits intomainfrom
2085-add-from-conversions
Feb 16, 2026
Merged

chore: add From and TryFrom conversions between dto and foreign chain inspector types#2104
DSharifi merged 5 commits intomainfrom
2085-add-from-conversions

Conversation

@DSharifi
Copy link
Contributor

closes #2085

@DSharifi DSharifi marked this pull request as ready for review February 16, 2026 10:41
@claude
Copy link

claude bot commented Feb 16, 2026

Code Review

Found critical issues that need addressing before merge:

1. ⚠️ Asymmetric Conversion - Data Loss Risk

File: crates/foreign-chain-inspector/src/contract_interface_conversions.rs:41-51

The TryFrom<dtos::EvmFinality> for EthereumFinality conversion rejects EvmFinality::Latest even though EthereumFinality::Latest exists (line 36). This creates an asymmetric conversion:

  • EthereumFinality::Latest → dtos::EvmFinality::Latest works
  • dtos::EvmFinality::Latest → EthereumFinality::Latest fails

Impact: If the contract sends a finality level of Latest, follower nodes will fail to process verify_foreign_tx requests.

Fix: Either:

  • Support bidirectional conversion by handling Latest case, OR
  • Document why Latest is intentionally unsupported in the reverse direction

2. ⚠️ Integer Overflow Risk (Platform-Dependent)

File: crates/foreign-chain-inspector/src/contract_interface_conversions.rs:150, 162

log_index: log_index as u64,    // line 150 - usize → u64
log_index: log_index as usize,  // line 162 - u64 → usize

Issue: On 32-bit platforms, casting u64 → usize can silently overflow when log_index > 2^32.

Impact:

  • Wrong log extracted from transaction on 32-bit systems
  • Silent data corruption (no error, just wrong index)

Fix:

// Line 162
log_index: log_index.try_into()
    .map_err(|_| UnsupportedVariantError { 
        context: "log_index exceeds platform usize" 
    })?,

3. 💭 Future Maintainability Concern

All TryFrom implementations use catch-all patterns (_ => Err(...)) for #[non_exhaustive] enums. When new variants are added to DTOs:

  • ✅ Compilation succeeds (no warnings)
  • ❌ Runtime conversions fail silently
  • ⚠️ No visibility that conversions need updating

Recommendation: Add explicit variant matches instead of wildcards where possible to get compiler warnings on missing cases.


⚠️ Issues Found - Please address the integer overflow and asymmetric conversion issues before merge.

@DSharifi
Copy link
Contributor Author

@claude

@DSharifi DSharifi force-pushed the 2085-add-from-conversions branch from 04b3ac0 to 035068f Compare February 16, 2026 11:20
netrome
netrome previously approved these changes Feb 16, 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.

Lgtm, a few nits

gilcu3
gilcu3 previously approved these changes Feb 16, 2026
Copy link
Contributor

@gilcu3 gilcu3 left a comment

Choose a reason for hiding this comment

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

LGTM!

}

// --- Log (RPC) <-> EvmLog (DTO) ---
fn log_to_evm_log(value: Log) -> dtos::EvmLog {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit surprised that the Log type was not defined in this crate, therefore conversion cannot be done idiomatically.

@DSharifi DSharifi dismissed stale reviews from gilcu3 and netrome via 710c277 February 16, 2026 12:17
gilcu3
gilcu3 previously approved these changes Feb 16, 2026
@DSharifi DSharifi enabled auto-merge February 16, 2026 12:21
@DSharifi DSharifi requested a review from netrome February 16, 2026 12:23
netrome
netrome previously approved these changes Feb 16, 2026
@DSharifi DSharifi added this pull request to the merge queue Feb 16, 2026
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Feb 16, 2026
@DSharifi DSharifi dismissed stale reviews from netrome and gilcu3 via ba3bb55 February 16, 2026 13:28
@DSharifi DSharifi force-pushed the 2085-add-from-conversions branch from ba3bb55 to 5ab4597 Compare February 16, 2026 13:37
@DSharifi DSharifi enabled auto-merge February 16, 2026 13:47
@DSharifi DSharifi added this pull request to the merge queue Feb 16, 2026
Merged via the queue into main with commit 09e24f0 Feb 16, 2026
10 checks passed
@DSharifi DSharifi deleted the 2085-add-from-conversions branch February 16, 2026 15:16
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.

create From/TryFrom conversions between contract-interface types and foreign-inspector-rpc types

3 participants