Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/pr-review.yml
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
name: PR Review

on:
workflow_dispatch:
# pull_request:
# types: [opened, synchronize, ready_for_review, reopened]
pull_request:
types: [opened, synchronize, ready_for_review, reopened]

jobs:
review-with-tracking:
if: contains(github.event.pull_request.labels.*.name, 'jobtaker')
runs-on: ${{ vars.RUNNER }}
permissions:
contents: read
Expand Down
2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ alloy-provider = { version = "1", default-features = false, features = [
alloy-rpc-types = "1"
alloy-sol-types = { version = "1", features = ["json"] }
alloy-transport = { version = "1", default-features = false }
Copy link

Choose a reason for hiding this comment

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

Dependency Removed: The bincode dependency has been removed but there's no context in the PR about why.

Questions:

  1. Was bincode actually unused?
  2. Are there any breaking changes this might cause for downstream users?
  3. Should this be mentioned in a CHANGELOG?

Consider documenting dependency removals in commit messages or PR descriptions.

bincode = "1"
nitrogen-circle-message-transmitter-v2-encoder = { git = "https://github.com/CarteraMesh/nitrogen.git", branch = "main" }
nitrogen-circle-token-messenger-minter-v2-encoder = { git = "https://github.com/CarteraMesh/nitrogen.git", branch = "main" }
nitrogen-instruction-builder = { git = "https://github.com/CarteraMesh/nitrogen.git", branch = "main" }
Expand All @@ -42,6 +41,7 @@ solana-signature = "2"
solana-signer = "2"
spl-associated-token-account = { version = "7.0.0", features = ["no-entrypoint"] }
thiserror = "2"
tokio = { version = "1", default-features = false, features = ["time"] }
Copy link

Choose a reason for hiding this comment

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

Dependency Addition: Adding tokio with time feature makes sense given the async/await refactoring from std::thread::sleep to tokio::time::sleep.

Consideration: The PR changes from blocking std::thread::sleep to async tokio::time::sleep, which is correct for async functions. However, verify that:

  1. This doesn't introduce a new runtime dependency burden for users
  2. The features are minimal (good: only time feature is enabled)
  3. Version "1" is intentionally broad - consider if a more specific version is needed for stability

tracing = "0.1"

[dev-dependencies]
Expand Down
64 changes: 56 additions & 8 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,62 @@
[![CI](https://github.com/CarteraMesh/cctp-bridge/workflows/test/badge.svg)](https://github.com/CarteraMesh/cctp-bridge/actions)
[![Cov](https://codecov.io/github/CarteraMesh/cctp-bridge/graph/badge.svg?token=dILa1k9tlW)](https://codecov.io/github/CarteraMesh/cctp-bridge)

## Installation

### Cargo

* Install the rust toolchain in order to have cargo installed by following
[this](https://www.rust-lang.org/tools/install) guide.
* run `cargo install cctp-bridge`

## About

cctp-bridge is a Rust-based helper library for the Cross-Chain Token Protocol [CCTP](https://developers.circle.com/cctp). It facilitates the transfer of USDC between different blockchain networks.
This crates provides flexible control over the transfer process, allowing users to customize various aspects of the transfer.

This project is a fork of the [cctp-rs](https://github.com/semiotic-ai/cctp-rs) [crate](https://crates.io/crates/cctp-rs)

## Example

```rust

mod common;

use {
alloy_chains::NamedChain,
alloy_provider::WalletProvider,
cctp_bridge::{Cctp, SolanSigners},
common::*,
solana_signer::Signer,
tracing::info,
};

#[tokio::main]
async fn main() -> anyhow::Result<()> {
dotenvy::dotenv().ok();
tracing_subscriber::fmt::init();
// Setup wallets
let base_sepolia_wallet_provider = evm_base_setup()?;
let (solana_keypair, rpc) = solana_setup()?;
info!(
"solana address {} sends to base address {}",
solana_keypair.pubkey(),
base_sepolia_wallet_provider.default_signer_address()
);

// Convenience wrapper for cctp_bridge::SolanaProvider trait
let rpc_wrapper: cctp_bridge::SolanaWrapper = rpc.into();
// Convenience wrapper for solana_signer::Signer for use of CCTP operations
let signers = SolanSigners::new(solana_keypair);

let bridge = Cctp::new_solana_evm(
rpc_wrapper,
base_sepolia_wallet_provider,
cctp_bridge::SOLANA_DEVNET, // source chain
NamedChain::BaseSepolia, // destination chain
);
// 0.000010 USDC to base sepolia
let result = bridge.bridge_sol_evm(10, signers, None, None, None).await?;
println!("Solana burn txHash {}", result.burn);
println!(
"Base Receive txHash {}",
alloy_primitives::hex::encode(result.recv)
);
Ok(())
}
```
Comment on lines +17 to +63
Copy link

Choose a reason for hiding this comment

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

Excellent Documentation Improvement ✓ The new example in the README is much more comprehensive than before. It includes:

  • Clear setup steps
  • Well-commented code
  • Actual output expectations
  • Context about what the code does

Minor Suggestions:

  1. Line 19: mod common; references a module that won't exist for users copying this example. Consider either:
    • Removing it and inlining the necessary setup
    • Adding a note: // Note: common module contains setup helpers - see examples/common for full code
  2. Line 55: Good comment about amount being 0.000010 USDC (10 units with 6 decimals)
  3. Consider adding error handling best practices in the example


## Development

Expand Down
2 changes: 1 addition & 1 deletion examples/common/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use {
};

#[allow(dead_code)]
pub fn evm_setup() -> anyhow::Result<impl WalletProvider + Provider + Clone> {
pub fn evm_base_setup() -> anyhow::Result<impl WalletProvider + Provider + Clone> {
let secret_key = env::var("EVM_SECRET_KEY").expect("EVM_SECRET_KEY not set");
let wallet = PrivateKeySigner::from_str(&secret_key).expect("Invalid private key");
let api_key = env::var("ALCHEMY_API_KEY").expect("ALCHEMY_API_KEY not set");
Expand Down
2 changes: 1 addition & 1 deletion examples/evm_sol.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use {
async fn main() -> anyhow::Result<()> {
dotenvy::dotenv().ok();
tracing_subscriber::fmt::init();
let base_provider = common::evm_setup()?;
let base_provider = common::evm_base_setup()?;
let (owner, rpc) = common::solana_setup()?;
info!(
"solana address {} sends to base address {}",
Expand Down
32 changes: 20 additions & 12 deletions examples/sol_evm.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@ mod common;
use {
alloy_chains::NamedChain,
alloy_provider::WalletProvider,
cctp_bridge::{Cctp, SolanSigners, SolanaWrapper},
cctp_bridge::{Cctp, SolanSigners},
common::*,
solana_signer::Signer,
tracing::info,
};
Expand All @@ -12,25 +13,32 @@ use {
async fn main() -> anyhow::Result<()> {
dotenvy::dotenv().ok();
tracing_subscriber::fmt::init();
let base_provider = common::evm_setup()?;
let (owner, rpc) = common::solana_setup()?;
// Setup wallets
let base_sepolia_wallet_provider = evm_base_setup()?;
let (solana_keypair, rpc) = solana_setup()?;
info!(
"solana address {} sends to base address {}",
owner.pubkey(),
base_provider.default_signer_address()
solana_keypair.pubkey(),
base_sepolia_wallet_provider.default_signer_address()
);

let rpc: SolanaWrapper = rpc.into();
// Convenience wrapper for cctp_bridge::SolanaProvider trait
let rpc_wrapper: cctp_bridge::SolanaWrapper = rpc.into();
// Convenience wrapper for solana_signer::Signer for use of CCTP operations
let signers = SolanSigners::new(solana_keypair);

let bridge = Cctp::new_solana_evm(
rpc,
base_provider,
rpc_wrapper,
base_sepolia_wallet_provider,
cctp_bridge::SOLANA_DEVNET,
NamedChain::BaseSepolia,
);
let result = bridge
.bridge_sol_evm(10, SolanSigners::new(owner), None, None, None)
.await?;
println!("success {result}");
// 0.000010 USDC to base sepolia
let result = bridge.bridge_sol_evm(10, signers, None, None, None).await?;
println!("Solana burn txHash {}", result.burn);
println!(
"Base Receive txHash {}",
alloy_primitives::hex::encode(result.recv)
);
Ok(())
}
2 changes: 1 addition & 1 deletion src/attestation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use {
};

/// To be passed to message transmitter to claim/mint
#[derive(Clone)]
#[derive(Clone, Eq, PartialEq)]
Copy link

Choose a reason for hiding this comment

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

API Change: Added trait derives

Adding Eq and PartialEq to Attestation is good for testing and comparison. However:

  1. Security consideration: Ensure that comparing attestations byte-by-byte is the correct semantic equality.
  2. Are there cases where attestations might be semantically equal but have different byte representations?
  3. Consider implementing Hash as well if you plan to use attestations in HashMaps/HashSets

Current implementation looks correct for direct byte comparison.

Copy link

Choose a reason for hiding this comment

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

Why is Eq and PartialEq needed?

This adds Eq and PartialEq derives to Attestation. While this isn't inherently problematic, it's worth understanding why:

  1. Are attestations being compared in the codebase?
  2. Is this for testing purposes?
  3. Is it being used in a HashMap or HashSet?

Recommendation: If this is for testing, consider adding a comment or only deriving these in test configuration:

#[derive(Clone)]
#[cfg_attr(test, derive(Eq, PartialEq))]

If it's needed in production code, that's fine, but it would be good to understand the use case.

Copy link

Choose a reason for hiding this comment

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

Derive Changes: Adding Eq and PartialEq to Attestation is good for testing and comparison.

Note: Since Attestation contains Vec<u8> fields, the derived Eq implementation will do byte-by-byte comparison, which is correct for attestation data. This is used in the test at tests/integration.rs:141 with assert_eq!(recv_attest, attest); - good addition!

pub struct Attestation {
pub attestation: Vec<u8>,
pub message: Vec<u8>,
Expand Down
60 changes: 47 additions & 13 deletions src/bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,22 @@ use {
CctpChain,
error::{Error, Result},
},
alloy_chains::{Chain, NamedChain},
alloy_chains::{Chain, ChainKind, NamedChain},
alloy_network::Ethereum,
alloy_primitives::{FixedBytes, TxHash, hex},
alloy_primitives::{
FixedBytes,
TxHash,
hex::{self, encode},
},
alloy_provider::Provider,
alloy_sol_types::SolEvent,
reqwest::{Client, Response},
solana_signature::Signature as SolanaSignature,
std::{
fmt::{Debug, Display},
thread::sleep,
time::Duration,
},
tokio::time::sleep,
tracing::{Level, debug, error, info, instrument, trace},
};

Expand Down Expand Up @@ -51,12 +55,15 @@ pub const CHAIN_CONFIRMATION_CONFIG: &[(NamedChain, u64, Duration)] = &[
];

/// Gets the chain-specific confirmation configuration
pub fn get_chain_confirmation_config(chain: &NamedChain) -> (u64, Duration) {
CHAIN_CONFIRMATION_CONFIG
.iter()
.find(|(ch, _, _)| ch == chain)
.map(|(_, confirmations, timeout)| (*confirmations, *timeout))
.unwrap_or((1, DEFAULT_CONFIRMATION_TIMEOUT))
pub fn get_chain_confirmation_config(chain: &Chain) -> (u64, Duration) {
match chain.kind() {
ChainKind::Named(n) => CHAIN_CONFIRMATION_CONFIG
.iter()
.find(|(ch, _, _)| ch == n)
.map(|(_, confirmations, timeout)| (*confirmations, *timeout))
.unwrap_or((1, DEFAULT_CONFIRMATION_TIMEOUT)),
ChainKind::Id(_) => (2, Duration::from_secs(4)), // TODO add specific timeout for id chain
Copy link

Choose a reason for hiding this comment

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

Hardcoded Default: The fallback timeout value of 4 seconds seems quite short for blockchain confirmation, especially for chains with variable block times. This could lead to premature timeouts under network congestion.

Recommendation: Consider:

  1. Documenting why 4 seconds was chosen
  2. Making this configurable rather than hardcoded
  3. Using a more conservative default (e.g., 30-60 seconds)

}
}

/// For solana reclaim accounts
Expand Down Expand Up @@ -114,7 +121,7 @@ impl Display for EvmBridgeResult {
}
}

#[derive(Clone, Debug)]
#[derive(Clone)]
pub struct Cctp<SrcProvider, DstProvider> {
source_provider: SrcProvider,
destination_provider: DstProvider,
Expand All @@ -124,6 +131,18 @@ pub struct Cctp<SrcProvider, DstProvider> {
client: Client,
}

impl<SrcProvider, DstProvider> Debug for Cctp<SrcProvider, DstProvider> {
Copy link

Choose a reason for hiding this comment

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

Code Quality: Good implementation of Debug trait

Nice custom Debug implementation that provides useful context without exposing sensitive provider details. The format CCTP[source(domain)->dest(domain)] is clear and informative.

Minor suggestion: Consider also including whether it's sandbox/mainnet:

write!(
    f,
    "CCTP[{}({})->{}({})][{}]",
    self.source_chain, src_domain, 
    self.destination_chain, dst_domain,
    if self.source_chain.sandbox() { "sandbox" } else { "mainnet" }
)

fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
let src_domain = self.source_chain.cctp_domain_id().unwrap_or(u32::MAX);
Copy link

Choose a reason for hiding this comment

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

Potential panic in Debug implementation

The unwrap_or(u32::MAX) is used when domain ID retrieval fails. While u32::MAX is better than panicking, it's misleading in debug output as it suggests a valid domain ID.

Recommendation: Consider indicating an error state more explicitly:

let src_domain = self.source_chain.cctp_domain_id()
    .map(|d| d.to_string())
    .unwrap_or_else(|_| "UNSUPPORTED".to_string());

Or just use ? syntax in the format string to show None for failed lookups.

let dst_domain = self.destination_chain.cctp_domain_id().unwrap_or(u32::MAX);
write!(
f,
"CCTP[{}({})->{}({})]",
self.source_chain, src_domain, self.destination_chain, dst_domain
)
}
Comment on lines +134 to +143
Copy link

Choose a reason for hiding this comment

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

Good Debug Implementation ✓ The custom Debug implementation for Cctp is much more informative than a derived implementation would be, showing the chain routing and domain IDs.

Observation: Using unwrap_or(u32::MAX) for unsupported chains is reasonable for debug output. This won't panic and clearly indicates an error state.

Minor Enhancement: Consider also showing the recipient address in debug output for complete context:

write!(
    f,
    "CCTP[{}({})->{}({}) recipient={}]",
    self.source_chain, src_domain, self.destination_chain, dst_domain, self.recipient
)

}

impl<SrcProvider, DstProvider> Cctp<SrcProvider, DstProvider> {
/// Returns the CCTP API URL for the current environment
pub fn api_url(&self) -> &'static str {
Expand Down Expand Up @@ -196,6 +215,21 @@ impl<SrcProvider, DstProvider> Cctp<SrcProvider, DstProvider> {
)
}

/// Wrapper call to [`get_attestation_with_retry`] for evm [`TxHash`]
Copy link

Choose a reason for hiding this comment

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

Code Quality: Good addition of convenience method

The get_attestation_evm wrapper is a nice ergonomic improvement. It properly handles the hex encoding that was previously scattered in calling code.

Minor suggestion: Consider adding a doc comment explaining when to use this vs get_attestation_with_retry:

/// Wrapper call to [`get_attestation_with_retry`] for EVM [`TxHash`].
/// 
/// Use this method when you have an EVM transaction hash. It handles
/// the hex encoding automatically. For non-EVM chains or raw message hashes,
/// use [`get_attestation_with_retry`] directly.

pub async fn get_attestation_evm(
Copy link

Choose a reason for hiding this comment

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

Missing documentation about error behavior

This new helper method is great for ergonomics, but the documentation doesn't mention what happens if the attestation retrieval fails (timeouts, network errors, etc.).

Recommendation: Add a # Errors section to the docstring explaining the possible error conditions:

/// Wrapper call to [`get_attestation_with_retry`] for evm [`TxHash`]
///
/// # Errors
/// Returns an error if:
/// - The attestation times out after max_attempts
/// - Network errors occur during API calls
/// - The message hash is not found or attestation fails

&self,
message_hash: TxHash,
max_attempts: Option<u32>,
poll_interval: Option<u64>,
) -> Result<Attestation> {
self.get_attestation_with_retry(
format!("0x{}", encode(message_hash)),
max_attempts,
poll_interval,
)
.await
}
Comment on lines +218 to +231
Copy link

Choose a reason for hiding this comment

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

Good API Addition ✓ The get_attestation_evm wrapper function provides a convenient, type-safe interface for EVM transactions. The hex encoding with 0x prefix matches expected API format.

This is a good example of providing specialized convenience methods while keeping the generic get_attestation_with_retry available for flexibility.


/// Gets the attestation for a message hash from the CCTP API
///
/// # Arguments
Expand Down Expand Up @@ -238,7 +272,7 @@ impl<SrcProvider, DstProvider> Cctp<SrcProvider, DstProvider> {
if response.status() == reqwest::StatusCode::TOO_MANY_REQUESTS {
let secs = 5 * 60;
debug!(sleep_secs = ?secs, "Rate limit exceeded, waiting before retrying");
sleep(Duration::from_secs(secs));
sleep(Duration::from_secs(secs)).await;
continue;
Comment on lines 272 to 276
Copy link

Choose a reason for hiding this comment

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

Hardcoded Sleep Duration: The 5-minute sleep on rate limiting is quite aggressive and blocks the entire async task. If multiple rate limits occur, this could lead to very long wait times.

Considerations:

  1. This is reasonable behavior for CCTP's rate limiting, but consider making it configurable
  2. Consider using exponential backoff instead of a fixed 5-minute wait
  3. Document this behavior in the function's documentation so users know what to expect

}

Expand All @@ -251,7 +285,7 @@ impl<SrcProvider, DstProvider> Cctp<SrcProvider, DstProvider> {
poll_interval = ?poll_interval,
"Attestation not found (404), waiting before retrying"
);
sleep(Duration::from_secs(poll_interval));
sleep(Duration::from_secs(poll_interval)).await;
continue;
}

Expand Down Expand Up @@ -322,7 +356,7 @@ impl<SrcProvider, DstProvider> Cctp<SrcProvider, DstProvider> {
poll_interval = ?poll_interval,
"Attestation pending, waiting before retrying"
);
sleep(Duration::from_secs(poll_interval));
sleep(Duration::from_secs(poll_interval)).await;
}
}
}
Expand Down
Loading
Loading