257 Fix informational issues part 3 (#390)

* IML-01 | Same Behavior Defined For Different Conditions

* LI7-05 | Mismatch in Variable Name and Pallet Name

* 9B2-02 | Inconsistent Comments

* PRF-01 | Unhandled Error

* LIH-03 | Values Length Not Validated in `feed_values` Function

* CLI-01 | Confusing Function Naming

* CLI-03 | Incorrect Error Type Thrown partial

* CLI-03 | Incorrect Error Type Thrown

* #390 (comment),
#390 (comment),
#390 (comment),
#390 (comment),
#390 (comment),
https://github.com/pendulum-chain/spacewalk/actions/runs/6048709746/job/16414585252?pr=390#step:12:38

* https://github.com/pendulum-chain/spacewalk/actions/runs/6069946336/job/16465120232?pr=390#step:11:1573

* fix rustfmt error: https://github.com/pendulum-chain/spacewalk/actions/runs/6072497871/job/16472578544?pr=390

clients/runtime/src/error.rs

@@ -22,6 +22,8 @@ use prometheus::Error as PrometheusError;
 pub enum Error {
 	#[error("Could not get exchange rate info")]
 	ExchangeRateInfo,
+	#[error("The list is empty. At least one element is required.")]
+	FeedingEmptyList,
 	#[error("Could not get issue id")]
 	RequestIssueIDNotFound,
 	#[error("Could not get redeem id")]

clients/runtime/src/rpc.rs

@@ -870,6 +870,10 @@ impl OraclePallet for SpacewalkParachain {
 	/// # Arguments
 	/// * `value` - the current exchange rate
 	async fn feed_values(&self, values: Vec<((Vec<u8>, Vec<u8>), FixedU128)>) -> Result<(), Error> {
+		if values.is_empty() {
+			return Err(Error::FeedingEmptyList)
+		}
+
 		use crate::metadata::runtime_types::dia_oracle::dia::CoinInfo;
 
 		let now = std::time::SystemTime::now();

clients/vault/src/error.rs

@@ -33,8 +33,8 @@ pub enum Error {
 	#[error("StellarWalletError: {0}")]
 	StellarWalletError(#[from] WalletError),
 
-	#[error("Error returned when fetching remote info")]
-	HttpFetchingError,
+	#[error("Error fetching remote info from a Stellar Horizon server")]
+	HorizonResponseError,
 	#[error("Lookup Error")]
 	LookupError,
 	#[error("Stellar SDK Error")]

clients/vault/src/issue.rs

@@ -52,9 +52,10 @@ pub(crate) async fn initialize_issue_set(
 /// # Arguments
 ///
 /// * `parachain_rpc` - the parachain RPC handle
-/// * `vault_secret_key` - The secret key of this vault
+/// * `vault_public_key` - The public key of this vault
 /// * `event_channel` - the channel over which to signal events
 /// * `issues` - a map to save all the new issue requests
+/// * `memos_to_issue_ids` - map of issue memo to issue id
 pub async fn listen_for_issue_requests(
 	parachain_rpc: SpacewalkParachain,
 	vault_public_key: PublicKey,

clients/vault/src/oracle/agent.rs

@@ -128,7 +128,6 @@ pub async fn start_oracle_agent(
 impl OracleAgent {
 	/// This method returns the proof for a given slot or an error if the proof cannot be provided.
 	/// The agent will try every possible way to get the proof before returning an error.
-	/// Set timeout to 60 seconds; 10 seconds interval.
 	pub async fn get_proof(&self, slot: Slot) -> Result<Proof, Error> {
 		let sender = self
 			.message_sender
@@ -151,6 +150,7 @@ impl OracleAgent {
 					None => {
 						tracing::warn!("Failed to build proof for slot {slot}.");
 						drop(collector);
+						// give 10 seconds interval for every retry
 						sleep(Duration::from_secs(10)).await;
 						continue
 					},

clients/vault/src/oracle/collector/proof_builder.rs

@@ -84,7 +84,12 @@ impl ScpMessageCollector {
 	/// fetches envelopes from the stellar node
 	async fn ask_node_for_envelopes(&self, slot: Slot, sender: &StellarMessageSender) {
 		// for this slot to be processed, we must put this in our watch list.
-		let _ = sender.send(StellarMessage::GetScpState(slot.try_into().unwrap())).await;
+		if let Err(e) = sender.send(StellarMessage::GetScpState(slot.try_into().unwrap())).await {
+			tracing::error!(
+				"Proof Building for slot {slot}: failed to send `GetScpState` message: {e:?}"
+			);
+			return
+		}
 		tracing::debug!(
 			"Proof Building for slot {slot}: requesting to StellarNode for messages..."
 		);
@@ -134,7 +139,7 @@ impl ScpMessageCollector {
 		empty
 	}
 
-	/// Returns either a TransactionSet or a ProofStatus saying it failed to retrieve the set.
+	/// Returns a TransactionSet if a txset is found; None if the slot does not have a txset
 	///
 	/// # Arguments
 	///
@@ -196,18 +201,15 @@ impl ScpMessageCollector {
 	///
 	/// * `envelopes_map_lock` - the map to insert the envelopes to.
 	/// * `slot` - the slot where the envelopes belong to
-	fn get_envelopes_from_horizon_archive(
-		&self,
-		slot: Slot,
-	) -> impl Future<Output = Result<(), String>> {
+	fn get_envelopes_from_horizon_archive(&self, slot: Slot) -> impl Future<Output = ()> {
 		tracing::debug!("Fetching SCP envelopes from horizon archive for slot {slot}...");
 		let envelopes_map_arc = self.envelopes_map_clone();
 
 		let archive_urls = self.stellar_history_archive_urls();
 		async move {
 			if archive_urls.is_empty() {
-				tracing::debug!("Cannot get envelopes from horizon archive for slot {slot}: no archive URLs configured");
-				return Err("No archive URLs configured".to_string())
+				tracing::error!("Cannot get envelopes from horizon archive for slot {slot}: no archive URLs configured");
+				return
 			}
 
 			// We try to get the SCPArchive from each archive URL until we succeed or run out of
@@ -217,7 +219,7 @@ impl ScpMessageCollector {
 				let scp_archive_result = scp_archive_storage.get_archive(slot).await;
 				if let Err(e) = scp_archive_result {
 					tracing::error!(
-						"Could not get SCPArchive for slot {slot:?} from Horizon Archive: {e:?}"
+						"Could not get SCPArchive for slot {slot} from Horizon Archive: {e:?}"
 					);
 					continue
 				}
@@ -268,18 +270,18 @@ impl ScpMessageCollector {
 
 						if envelopes_map.get(&slot).is_none() {
 							tracing::debug!(
-							"Adding {} archived SCP envelopes for slot {slot} to envelopes map. {} are externalized",
-							relevant_envelopes.len(),
-							externalized_envelopes_count
-						);
+								"Adding {} archived SCP envelopes for slot {slot} to envelopes map. {} are externalized",
+								relevant_envelopes.len(),
+								externalized_envelopes_count
+							);
 							envelopes_map.insert(slot, relevant_envelopes);
 							break
 						}
 					}
+				} else {
+					tracing::warn!("Could not get ScpHistory entry from archive for slot {slot}");
 				}
 			}
-			// If we get here, we failed to get the envelopes from any archive
-			return Err("Could not get envelopes from any archive".to_string())
 		}
 	}
 
@@ -318,6 +320,10 @@ impl ScpMessageCollector {
 					let mut tx_set_map = txset_map_arc.write();
 					tx_set_map.insert(slot, target_history_entry.tx_set.clone());
 					break
+				} else {
+					tracing::warn!(
+						"Could not get TransactionHistory entry from archive for slot {slot}"
+					);
 				}
 			}
 		}

clients/vault/src/oracle/storage/impls.rs

@@ -55,11 +55,12 @@ impl FileHandlerExt<EnvelopesMap> for EnvelopesFileHandler {
 		let len = data.len();
 
 		for (idx, (key, value)) in data.iter().enumerate() {
+			// writes the first slot as the beginning of the filename
 			if idx == 0 {
 				let _ = write!(filename, "{}_", key);
 			}
-
-			if idx == (len - 1) {
+			// writes the last slot as the ending of the filename
+			else if idx == (len - 1) {
 				let _ = write!(filename, "{}", key);
 			}
 
@@ -106,11 +107,12 @@ impl FileHandlerExt<TxSetMap> for TxSetsFileHandler {
 		let len = data.len();
 
 		for (idx, (key, set)) in data.iter().enumerate() {
+			// writes the first slot as the beginning of the filename
 			if idx == 0 {
 				let _ = write!(filename, "{}_", key);
 			}
-
-			if idx == (len - 1) {
+			// writes the last slot as the ending of the filename
+			else if idx == (len - 1) {
 				let _ = write!(filename, "{}", key);
 			}
 
@@ -220,7 +222,6 @@ mod test {
 		{
 			let slot = 578490;
 			let expected_name = format!("{}_{}", slot - *M_SLOTS_FILE, slot);
-
 			let file_name =
 				EnvelopesFileHandler::find_file_by_slot(slot).expect("should return a file");
 			assert_eq!(&file_name, &expected_name);
@@ -360,6 +361,7 @@ mod test {
 			let mut path = PathBuf::new();
 			path.push("./resources/test/tx_sets_for_testing");
 			path.push(&format!("{}_{}", first_slot, last_slot));
+			println!("find file: {:?}", path);
 
 			let mut file = File::open(path).expect("file should exist");
 			let mut bytes: Vec<u8> = vec![];

clients/vault/src/system.rs

@@ -117,17 +117,14 @@ impl VaultIdManager {
 			.get_vaults_by_account_id(self.spacewalk_parachain.get_account_id())
 			.await?
 		{
-			match is_vault_registered(&self.spacewalk_parachain, &vault_id).await {
-				Err(Error::RuntimeError(RuntimeError::VaultLiquidated)) => {
-					tracing::error!(
-						"[{}] Vault is liquidated -- not going to process events for this vault.",
-						vault_id.pretty_print()
-					);
-				},
-				Ok(_) => {
-					self.add_vault_id(vault_id.clone()).await?;
-				},
-				Err(x) => return Err(x),
+			// check if vault is registered
+			match self.spacewalk_parachain.get_vault(&vault_id).await {
+				Ok(_) => self.add_vault_id(vault_id.clone()).await?,
+				Err(RuntimeError::VaultLiquidated) => tracing::error!(
+					"[{}] Vault is liquidated -- not going to process events for this vault.",
+					vault_id.pretty_print()
+				),
+				Err(e) => return Err(e.into()),
 			}
 		}
 		Ok(())
@@ -745,18 +742,18 @@ impl VaultService {
 
 		self.maintain_connection()?;
 
-		self.maybe_register_public_key().await?;
+		self.register_public_key_if_not_present().await?;
 
 		join_all(parsed_auto_register.iter().map(
 			|(collateral_currency, wrapped_currency, amount)| {
-				self.maybe_register_vault(collateral_currency, wrapped_currency, amount)
+				self.register_vault_if_not_present(collateral_currency, wrapped_currency, amount)
 			},
 		))
 		.await
 		.into_iter()
 		.collect::<Result<_, Error>>()?;
 
-		// purposefully _after_ maybe_register_vault and _before_ other calls
+		// purposefully _after_ register_vault_if_not_present and _before_ other calls
 		self.vault_id_manager.fetch_vault_ids().await?;
 
 		let wallet = self.stellar_wallet.write().await;
@@ -801,7 +798,7 @@ impl VaultService {
 		run_and_monitor_tasks(self.shutdown.clone(), tasks).await
 	}
 
-	async fn maybe_register_public_key(&mut self) -> Result<(), Error> {
+	async fn register_public_key_if_not_present(&mut self) -> Result<(), Error> {
 		if let Some(_faucet_url) = &self.config.faucet_url {
 			// TODO fund account with faucet
 		}
@@ -821,55 +818,72 @@ impl VaultService {
 		Ok(())
 	}
 
-	async fn maybe_register_vault(
+	async fn register_vault_with_collateral(
+		&self,
+		vault_id: VaultId,
+		collateral_amount: &Option<u128>,
+	) -> Result<(), Error> {
+		if let Some(collateral) = collateral_amount {
+			tracing::info!("[{}] Automatically registering...", vault_id.pretty_print());
+			let free_balance = self
+				.spacewalk_parachain
+				.get_free_balance(vault_id.collateral_currency())
+				.await?;
+			return self
+				.spacewalk_parachain
+				.register_vault(
+					&vault_id,
+					if collateral.gt(&free_balance) {
+						tracing::warn!(
+							"Cannot register with {}, using the available free balance: {}",
+							collateral,
+							free_balance
+						);
+						free_balance
+					} else {
+						*collateral
+					},
+				)
+				.await
+				.map_err(|e| Error::RuntimeError(e))
+		} else if let Some(_faucet_url) = &self.config.faucet_url {
+			tracing::info!("[{}] Automatically registering...", vault_id.pretty_print());
+			// TODO
+			// faucet::fund_and_register(&self.spacewalk_parachain, faucet_url, &vault_id)
+			// 	.await?;
+			Ok(())
+		} else {
+			tracing::error!(
+				"[{}] Cannot register a vault: no collateral and no faucet url",
+				vault_id.pretty_print()
+			);
+			Err(Error::FaucetUrlNotSet)
+		}
+	}
+
+	async fn register_vault_if_not_present(
 		&self,
 		collateral_currency: &CurrencyId,
 		wrapped_currency: &CurrencyId,
 		maybe_collateral_amount: &Option<u128>,
 	) -> Result<(), Error> {
 		let vault_id = self.get_vault_id(*collateral_currency, *wrapped_currency);
 
-		match is_vault_registered(&self.spacewalk_parachain, &vault_id).await {
-			Err(Error::RuntimeError(RuntimeError::VaultLiquidated)) | Ok(true) => {
+		// check if a vault is registered
+		match self.spacewalk_parachain.get_vault(&vault_id).await {
+			Ok(_) | Err(RuntimeError::VaultLiquidated) => {
 				tracing::info!(
 					"[{}] Not registering vault -- already registered",
 					vault_id.pretty_print()
 				);
+				Ok(())
 			},
-			Ok(false) => {
+			Err(RuntimeError::VaultNotFound) => {
 				tracing::info!("[{}] Not registered", vault_id.pretty_print());
-				if let Some(collateral) = maybe_collateral_amount {
-					tracing::info!("[{}] Automatically registering...", vault_id.pretty_print());
-					let free_balance = self
-						.spacewalk_parachain
-						.get_free_balance(vault_id.collateral_currency())
-						.await?;
-					self.spacewalk_parachain
-						.register_vault(
-							&vault_id,
-							if collateral.gt(&free_balance) {
-								tracing::warn!(
-									"Cannot register with {}, using the available free balance: {}",
-									collateral,
-									free_balance
-								);
-								free_balance
-							} else {
-								*collateral
-							},
-						)
-						.await?;
-				} else if let Some(_faucet_url) = &self.config.faucet_url {
-					tracing::info!("[{}] Automatically registering...", vault_id.pretty_print());
-					// TODO
-					// faucet::fund_and_register(&self.spacewalk_parachain, faucet_url, &vault_id)
-					// 	.await?;
-				}
+				self.register_vault_with_collateral(vault_id, maybe_collateral_amount).await
 			},
-			Err(x) => return Err(x),
+			Err(e) => Err(Error::RuntimeError(e)),
 		}
-
-		Ok(())
 	}
 
 	async fn await_parachain_block(&self) -> Result<u32, Error> {
@@ -883,14 +897,3 @@ impl VaultService {
 		Ok(startup_height)
 	}
 }
-
-pub(crate) async fn is_vault_registered(
-	parachain_rpc: &SpacewalkParachain,
-	vault_id: &VaultId,
-) -> Result<bool, Error> {
-	match parachain_rpc.get_vault(vault_id).await {
-		Ok(_) => Ok(true),
-		Err(RuntimeError::VaultNotFound) => Ok(false),
-		Err(err) => Err(err.into()),
-	}
-}