hyperlane-aptos: clean up use of client

Don't wrap the aptos SDK's client with a newtype and Deref, this is an antipattern and serves no useful purpose. Use the Url type in the APIs rather than converting to and from strings. Have a private constructor for AptosHpProvider to indicate silly clones enforced by hyperlane-core API.
movementlabsxyz · May 23, 2024 · de3dde2 · de3dde2
1 parent 12925be
commit de3dde2
Show file tree

Hide file tree

Showing 8 changed files with 55 additions and 64 deletions.
diff --git a/rust/chains/hyperlane-aptos/src/client.rs b/rust/chains/hyperlane-aptos/src/client.rs
diff --git a/rust/chains/hyperlane-aptos/src/interchain_gas.rs b/rust/chains/hyperlane-aptos/src/interchain_gas.rs
@@ -3,30 +3,35 @@
 use std::ops::RangeInclusive;
 
 use async_trait::async_trait;
-use hyperlane_core::{
-    ChainCommunicationError, ChainResult, ContractLocator, HyperlaneChain, HyperlaneContract, HyperlaneDomain, HyperlaneProvider, Indexed, Indexer, InterchainGasPaymaster, InterchainGasPayment, LogMeta, SequenceAwareIndexer, H256
-};
 use tracing::{info, instrument};
+use url::Url;
 
-use crate::{get_filtered_events, AptosHpProvider, ConnectionConf, GasPaymentEventData};
+use hyperlane_core::{
+    ChainCommunicationError, ChainResult, ContractLocator, HyperlaneChain, HyperlaneContract,
+    HyperlaneDomain, HyperlaneProvider, Indexed, Indexer, InterchainGasPaymaster,
+    InterchainGasPayment, LogMeta, SequenceAwareIndexer, H256,
+};
 
-use crate::AptosClient;
 use aptos_sdk::types::account_address::AccountAddress;
 
+use crate::{
+    get_filtered_events, AptosClient, AptosHpProvider, ConnectionConf, GasPaymentEventData,
+};
+
 /// A reference to an IGP contract on some Aptos chain
 #[derive(Debug)]
 pub struct AptosInterchainGasPaymaster {
     domain: HyperlaneDomain,
     package_address: AccountAddress,
-    aptos_client_url: String,
+    aptos_client_url: Url,
 }
 
 impl AptosInterchainGasPaymaster {
     /// Create a new Aptos IGP.
     pub fn new(conf: &ConnectionConf, locator: &ContractLocator) -> Self {
         let package_address =
             AccountAddress::from_bytes(<[u8; 32]>::from(locator.address)).unwrap();
-        let aptos_client_url = conf.url.to_string();
+        let aptos_client_url = conf.url.clone();
         Self {
             package_address,
             domain: locator.domain.clone(),
@@ -68,7 +73,7 @@ impl AptosInterchainGasPaymasterIndexer {
     pub fn new(conf: &ConnectionConf, locator: ContractLocator) -> Self {
         let package_address =
             AccountAddress::from_bytes(<[u8; 32]>::from(locator.address)).unwrap();
-        let aptos_client = AptosClient::new(conf.url.to_string());
+        let aptos_client = AptosClient::new(conf.url.clone());
         Self {
             aptos_client,
             package_address,
@@ -106,7 +111,7 @@ impl Indexer<InterchainGasPayment> for AptosInterchainGasPaymasterIndexer {
 
 #[async_trait]
 impl SequenceAwareIndexer<InterchainGasPayment> for AptosInterchainGasPaymasterIndexer {
-   async fn latest_sequence_count_and_tip(&self) -> ChainResult<(Option<u32>, u32)> {
+    async fn latest_sequence_count_and_tip(&self) -> ChainResult<(Option<u32>, u32)> {
         let chain_state = self
             .aptos_client
             .get_ledger_information()
@@ -115,5 +120,5 @@ impl SequenceAwareIndexer<InterchainGasPayment> for AptosInterchainGasPaymasterI
             .unwrap()
             .into_inner();
         Ok((None, chain_state.block_height as u32))
-   }
+    }
 }
diff --git a/rust/chains/hyperlane-aptos/src/interchain_security_module.rs b/rust/chains/hyperlane-aptos/src/interchain_security_module.rs
@@ -8,9 +8,7 @@ use hyperlane_core::{
     HyperlaneMessage, InterchainSecurityModule, ModuleType, H256, U256,
 };
 
-use crate::utils;
-use crate::AptosClient;
-use crate::ConnectionConf;
+use crate::{utils, AptosClient, AptosHpProvider, ConnectionConf};
 
 use aptos_sdk::types::account_address::AccountAddress;
 
@@ -27,7 +25,7 @@ pub struct AptosInterchainSecurityModule {
 impl AptosInterchainSecurityModule {
     /// Create a new sealevel InterchainSecurityModule
     pub fn new(conf: &ConnectionConf, locator: ContractLocator, payer: Option<Keypair>) -> Self {
-        let aptos_client = AptosClient::new(conf.url.to_string());
+        let aptos_client = AptosClient::new(conf.url.clone());
         let package_address =
             AccountAddress::from_bytes(<[u8; 32]>::from(locator.address)).unwrap();
         Self {
@@ -51,9 +49,9 @@ impl HyperlaneChain for AptosInterchainSecurityModule {
     }
 
     fn provider(&self) -> Box<dyn hyperlane_core::HyperlaneProvider> {
-        Box::new(crate::AptosHpProvider::new(
+        Box::new(AptosHpProvider::with_client(
             self.domain.clone(),
-            self.aptos_client.path_prefix_string(),
+            self.aptos_client.clone(),
         ))
     }
 }

diff --git a/rust/chains/hyperlane-aptos/src/lib.rs b/rust/chains/hyperlane-aptos/src/lib.rs
@@ -5,7 +5,6 @@
 #![deny(warnings)]
 
 pub use crate::multisig_ism::*;
-pub use client::AptosClient;
 pub use interchain_gas::*;
 pub use interchain_security_module::*;
 pub use mailbox::*;
@@ -17,7 +16,8 @@ pub use types::*;
 pub use utils::*;
 pub use validator_announce::*;
 
-mod client;
+use aptos_sdk::rest_client::Client as AptosClient;
+
 mod interchain_gas;
 mod interchain_security_module;
 mod mailbox;

diff --git a/rust/chains/hyperlane-aptos/src/mailbox.rs b/rust/chains/hyperlane-aptos/src/mailbox.rs
@@ -66,7 +66,7 @@ impl AptosMailbox {
         let domain = locator.domain.id();
         let package_address =
             AccountAddress::from_bytes(<[u8; 32]>::from(locator.address)).unwrap();
-        let aptos_client = AptosClient::new(conf.url.to_string());
+        let aptos_client = AptosClient::new(conf.url.clone());
 
         Ok(AptosMailbox {
             domain: locator.domain.clone(),
@@ -113,9 +113,9 @@ impl HyperlaneChain for AptosMailbox {
     }
 
     fn provider(&self) -> Box<dyn HyperlaneProvider> {
-        Box::new(AptosHpProvider::new(
+        Box::new(AptosHpProvider::with_client(
             self.domain.clone(),
-            self.aptos_client.path_prefix_string(),
+            self.aptos_client.clone(),
         ))
     }
 }
@@ -289,7 +289,7 @@ pub struct AptosMailboxIndexer {
 
 impl AptosMailboxIndexer {
     pub fn new(conf: &ConnectionConf, locator: ContractLocator) -> ChainResult<Self> {
-        let aptos_client = AptosClient::new(conf.url.to_string());
+        let aptos_client = AptosClient::new(conf.url.clone());
         let package_address =
             AccountAddress::from_bytes(<[u8; 32]>::from(locator.address)).unwrap();
         let mailbox = AptosMailbox::new(conf, locator, None)?;
@@ -340,7 +340,7 @@ impl Indexer<HyperlaneMessage> for AptosMailboxIndexer {
 #[async_trait]
 impl Indexer<H256> for AptosMailboxIndexer {
     async fn fetch_logs(
-        &self, 
+        &self,
         range: RangeInclusive<u32>
     ) -> ChainResult<Vec<(Indexed<H256>, LogMeta)>> {
         get_filtered_events::<H256, MsgProcessEventData>(

diff --git a/rust/chains/hyperlane-aptos/src/multisig_ism.rs b/rust/chains/hyperlane-aptos/src/multisig_ism.rs
@@ -48,7 +48,7 @@ impl AptosMultisigISM {
     pub fn new(conf: &ConnectionConf, locator: ContractLocator, payer: Option<Keypair>) -> Self {
         let package_address =
             AccountAddress::from_bytes(<[u8; 32]>::from(locator.address)).unwrap();
-        let aptos_client = AptosClient::new(conf.url.to_string());
+        let aptos_client = AptosClient::new(conf.url.clone());
 
         Self {
             payer,
@@ -70,9 +70,9 @@ impl HyperlaneChain for AptosMultisigISM {
         &self.domain
     }
     fn provider(&self) -> Box<dyn HyperlaneProvider> {
-        Box::new(AptosHpProvider::new(
+        Box::new(AptosHpProvider::with_client(
             self.domain.clone(),
-            self.aptos_client.path_prefix_string(),
+            self.aptos_client.clone(),
         ))
     }
 }

diff --git a/rust/chains/hyperlane-aptos/src/provider.rs b/rust/chains/hyperlane-aptos/src/provider.rs
@@ -2,6 +2,7 @@ use aptos_sdk::crypto::HashValue;
 use aptos_sdk::rest_client::aptos_api_types::Transaction;
 
 use async_trait::async_trait;
+use url::Url;
 
 use hyperlane_core::{
     BlockInfo, ChainInfo, ChainResult, HyperlaneChain, HyperlaneDomain, HyperlaneProvider, TxnInfo,
@@ -19,24 +20,35 @@ pub struct AptosHpProvider {
 
 impl AptosHpProvider {
     /// Create a new Aptos provider.
-    pub fn new(domain: HyperlaneDomain, rest_url: String) -> Self {
+    pub fn new(domain: HyperlaneDomain, rest_url: Url) -> Self {
         let aptos_client = AptosClient::new(rest_url);
         AptosHpProvider {
             domain,
             aptos_client,
         }
     }
+
+    // The design of hyperlane-core traits forces us to clone client instances.
+    // This kinky private constructor is used to indicate all such cases.
+    pub(crate) fn with_client(domain: HyperlaneDomain, aptos_client: AptosClient) -> Self {
+        AptosHpProvider {
+            domain,
+            aptos_client,
+        }
+    }
 }
 
 impl HyperlaneChain for AptosHpProvider {
     fn domain(&self) -> &HyperlaneDomain {
         &self.domain
     }
 
+    // Investigate the trait design: why does this method need to return
+    // a boxed provider, while HyperlaneProvider is also a supertrait?
     fn provider(&self) -> Box<dyn HyperlaneProvider> {
-        Box::new(AptosHpProvider::new(
+        Box::new(AptosHpProvider::with_client(
             self.domain.clone(),
-            self.aptos_client.path_prefix_string(),
+            self.aptos_client.clone(),
         ))
     }
 }

diff --git a/rust/chains/hyperlane-aptos/src/validator_announce.rs b/rust/chains/hyperlane-aptos/src/validator_announce.rs
@@ -11,7 +11,9 @@ use crate::utils::{self, send_aptos_transaction};
 use crate::{convert_hex_string_to_h256, convert_keypair_to_aptos_account, AptosClient};
 use crate::{simulate_aptos_transaction, ConnectionConf};
 use hyperlane_core::{
-    Announcement, ChainCommunicationError, ChainResult, ContractLocator, FixedPointNumber, HyperlaneChain, HyperlaneContract, HyperlaneDomain, SignedType, TxOutcome, ValidatorAnnounce, H256, H512, U256
+    Announcement, ChainCommunicationError, ChainResult, ContractLocator, FixedPointNumber,
+    HyperlaneChain, HyperlaneContract, HyperlaneDomain, SignedType, TxOutcome, ValidatorAnnounce,
+    H256, H512, U256,
 };
 
 use aptos_sdk::{
@@ -48,7 +50,7 @@ pub struct AptosValidatorAnnounce {
 impl AptosValidatorAnnounce {
     /// Create a new Aptos ValidatorAnnounce
     pub fn new(conf: &ConnectionConf, locator: ContractLocator, payer: Option<Keypair>) -> Self {
-        let aptos_client = AptosClient::new(conf.url.to_string());
+        let aptos_client = AptosClient::new(conf.url.clone());
         let package_address =
             AccountAddress::from_bytes(<[u8; 32]>::from(locator.address)).unwrap();
         Self {
@@ -117,9 +119,9 @@ impl HyperlaneChain for AptosValidatorAnnounce {
     }
 
     fn provider(&self) -> Box<dyn hyperlane_core::HyperlaneProvider> {
-        Box::new(crate::AptosHpProvider::new(
+        Box::new(crate::AptosHpProvider::with_client(
             self.domain.clone(),
-            self.aptos_client.path_prefix_string(),
+            self.aptos_client.clone(),
         ))
     }
 }
@@ -178,11 +180,11 @@ impl ValidatorAnnounce for AptosValidatorAnnounce {
 
         let (tx_hash, is_success) = self
             .announce_contract_call(announcement)
-            .await
-            .map_err(|e| {
-                println!("tx error {}", e.to_string());
-                ChainCommunicationError::TransactionTimeout()
-            })?;
+                .await
+                .map_err(|e| {
+                    println!("tx error {}", e.to_string());
+                    ChainCommunicationError::TransactionTimeout()
+                })?;
 
         Ok(TxOutcome {
             transaction_id: H512::from(convert_hex_string_to_h256(&tx_hash).unwrap()),