Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

499 make vault retry stellar transaction submission for tx insufficient fee error #500

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
2 changes: 1 addition & 1 deletion clients/wallet/src/horizon/horizon.rs
Original file line number Diff line number Diff line change
Expand Up @@ -267,7 +267,7 @@ impl<C: HorizonClient + Clone> HorizonFetcher<C> {
future::join(issue_map.read(), memos_to_issue_ids.read()).await;

if issue_map.is_empty() || memos_to_issue_ids.is_empty() {
tracing::info!("fetch_horizon_and_process_new_transactions(): nothing to traverse");
tracing::debug!("fetch_horizon_and_process_new_transactions(): nothing to traverse");
return Ok(last_cursor)
}
let mut txs_iter = self.fetch_transactions_iter(last_cursor).await?;
Expand Down
6 changes: 3 additions & 3 deletions clients/wallet/src/horizon/responses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -181,8 +181,8 @@ pub struct TransactionResponse {
pub fee_account: Vec<u8>,
#[serde(deserialize_with = "de_str_to_u64")]
pub fee_charged: u64,
#[serde(deserialize_with = "de_str_to_bytes")]
pub max_fee: Vec<u8>,
#[serde(deserialize_with = "de_str_to_u64")]
pub max_fee: u64,
operation_count: u32,
#[serde(deserialize_with = "de_str_to_bytes")]
pub envelope_xdr: Vec<u8>,
Expand Down Expand Up @@ -219,7 +219,7 @@ impl Debug for TransactionResponse {
.field("source_account_sequence", &debug_str_or_vec_u8!(&self.source_account_sequence))
.field("fee_account", &debug_str_or_vec_u8!(&self.fee_account))
.field("fee_charged", &self.fee_charged)
.field("max_fee", &debug_str_or_vec_u8!(&self.max_fee))
.field("max_fee", &self.max_fee)
.field("operation_count", &self.operation_count)
.field("envelope_xdr", &debug_str_or_vec_u8!(&self.envelope_xdr))
.field("result_xdr", &debug_str_or_vec_u8!(&self.result_xdr))
Expand Down
117 changes: 117 additions & 0 deletions clients/wallet/src/resubmissions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ use primitives::stellar::{types::SequenceNumber, PublicKey};
use reqwest::Client;

pub const RESUBMISSION_INTERVAL_IN_SECS: u64 = 1800;
// The maximum fee we want to charge for a transaction
const MAXIMUM_TX_FEE: u32 = 10_000_000; // 1 XLM

#[cfg_attr(test, mockable)]
impl StellarWallet {
Expand Down Expand Up @@ -150,6 +152,8 @@ impl StellarWallet {
return self.handle_tx_bad_seq_error_with_xdr(envelope_xdr).await.map(Some),
"tx_internal_error" =>
return self.handle_tx_internal_error(envelope_xdr).await.map(Some),
"tx_insufficient_fee" =>
return self.handle_tx_insufficient_fee_error(envelope_xdr).await.map(Some),
_ => {
if let Ok(env) = decode_to_envelope(envelope_xdr) {
self.remove_tx_envelope_from_cache(&env);
Expand Down Expand Up @@ -191,6 +195,36 @@ impl StellarWallet {

self.submit_transaction(envelope).await
}

// We encountered an insufficient fee error and try submitting the transaction again with a
// higher fee. We'll bump the fee by 10x the original fee. We don't use a FeeBumpTransaction
// because this operation is not supported by the stellar-relay pallet yet.
async fn handle_tx_insufficient_fee_error(
&self,
envelope_xdr_as_str_opt: &Option<String>,
) -> Result<TransactionResponse, Error> {
let tx_envelope = decode_to_envelope(envelope_xdr_as_str_opt)?;
let mut tx = tx_envelope.get_transaction().ok_or(DecodeError)?;

// Check if we already submitted this transaction
if !self.is_transaction_already_submitted(&tx).await {
// Remove original transaction.
// The same envelope will be saved again using a different sequence number
self.remove_tx_envelope_from_cache(&tx_envelope);

// Bump the fee by 10x
tx.fee = tx.fee * 10;
if tx.fee > MAXIMUM_TX_FEE {
tx.fee = MAXIMUM_TX_FEE;
}

return self.bump_sequence_number_and_submit(tx).await
}

tracing::error!("handle_tx_insufficient_fee_error(): Similar transaction already submitted. Skipping {:?}", tx);

Err(ResubmissionError("Transaction already submitted".to_string()))
}
}

fn is_source_account_match(public_key: &PublicKey, tx: &TransactionResponse) -> bool {
Expand Down Expand Up @@ -680,6 +714,47 @@ mod test {
wallet.remove_cache_dir();
}

#[tokio::test]
#[serial]
async fn check_handle_tx_insufficient_fee_error_with_envelope() {
let wallet =
wallet_with_storage("resources/check_handle_tx_insufficient_fee_error_with_envelope")
.expect("should return a wallet")
.clone();
let wallet = wallet.write().await;

// This is the fee we will bump by 10x
let base_fee = 99;
// This is the new maximum fee we expect to be charged
let bumped_fee = base_fee * 10;

let sequence = wallet.get_sequence().await.expect("return a sequence");
let envelope = wallet
.create_payment_envelope(
default_destination(),
StellarAsset::native(),
10,
rand::random(),
base_fee,
sequence + 1,
)
.expect("should return an envelope");

let envelope_xdr = envelope.to_base64_xdr();
// Convert vec to string (because the HorizonSubmissionError always returns a string)
let envelope_xdr =
Some(String::from_utf8(envelope_xdr).expect("should create string from vec"));

let result = wallet.handle_tx_insufficient_fee_error(&envelope_xdr).await;

assert!(result.is_ok());
let response = result.unwrap();
assert!(response.successful);
assert_eq!(response.max_fee, bumped_fee as u64);

wallet.remove_cache_dir();
}

#[tokio::test]
#[serial]
async fn check_handle_error() {
Expand Down Expand Up @@ -730,6 +805,48 @@ mod test {
}
}

// tx_insufficient_fee test
{
let envelope = wallet
.create_dummy_envelope_no_signature(19)
.await
.expect("returns an envelope");
let envelope_xdr = envelope.to_base64_xdr();
let envelope_xdr = String::from_utf8(envelope_xdr).ok();

// result is success
{
let error = Error::HorizonSubmissionError {
title: "title".to_string(),
status: 400,
reason: "tx_insufficient_fee".to_string(),
result_code_op: vec![],
envelope_xdr,
};

StellarWallet::is_transaction_already_submitted
.mock_safe(move |_, _| MockResult::Return(Box::pin(async move { false })));

assert!(wallet.handle_error(error).await.is_ok());
}

// result is error
{
let error = Error::HorizonSubmissionError {
title: "title".to_string(),
status: 400,
reason: "tx_insufficient_fee".to_string(),
result_code_op: vec![],
envelope_xdr: None,
};

match wallet.handle_error(error).await {
Err(Error::ResubmissionError(_)) => assert!(true),
other => panic!("expecting Error::ResubmissionError, found: {other:?}"),
};
}
}

// tx_internal_error test
{
let envelope = wallet
Expand Down
4 changes: 2 additions & 2 deletions pallets/oracle/rpc/runtime-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@

#![cfg_attr(not(feature = "std"), no_std)]
use codec::{Codec, Decode, Encode};
use frame_support::{dispatch::DispatchError};
use frame_support::dispatch::DispatchError;
use primitives::UnsignedFixedPoint;
use scale_info::TypeInfo;
#[cfg(feature = "std")]
use serde::{Deserialize, Deserializer, Serialize, Serializer};
use scale_info::TypeInfo;
#[derive(Eq, PartialEq, Encode, Decode, Default, TypeInfo)]
#[cfg_attr(feature = "std", derive(Debug, Serialize, Deserialize))]
#[cfg_attr(feature = "std", serde(rename_all = "camelCase"))]
Expand Down
12 changes: 6 additions & 6 deletions testchain/node/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ use futures::StreamExt;
use sc_client_api::BlockBackend;
use sc_consensus_aura::{ImportQueueParams, SlotProportion, StartAuraParams};
use sc_consensus_grandpa::SharedVoterState;
use sc_executor::{NativeElseWasmExecutor, WasmExecutor, DEFAULT_HEAP_ALLOC_STRATEGY, HeapAllocStrategy};
use sc_executor::{
HeapAllocStrategy, NativeElseWasmExecutor, WasmExecutor, DEFAULT_HEAP_ALLOC_STRATEGY,
};
use sc_service::{
error::Error as ServiceError, Configuration, RpcHandlers, TFullBackend, TFullClient,
TaskManager,
Expand Down Expand Up @@ -115,7 +117,7 @@ pub fn new_partial_mainnet(
.with_max_runtime_instances(config.max_runtime_instances)
.with_runtime_cache_size(config.runtime_cache_size)
.build();

let executor = NativeElseWasmExecutor::<MainnetExecutor>::new_with_wasm_executor(wasm);

let (client, backend, keystore_container, task_manager) =
Expand Down Expand Up @@ -219,7 +221,6 @@ pub fn new_partial_testnet(
>,
ServiceError,
> {

let telemetry = config
.telemetry_endpoints
.clone()
Expand All @@ -242,7 +243,7 @@ pub fn new_partial_testnet(
.with_max_runtime_instances(config.max_runtime_instances)
.with_runtime_cache_size(config.runtime_cache_size)
.build();

let executor = NativeElseWasmExecutor::<TestnetExecutor>::new_with_wasm_executor(wasm);

let (client, backend, keystore_container, task_manager) =
Expand Down Expand Up @@ -461,8 +462,7 @@ pub fn new_full(mut config: Configuration) -> Result<(TaskManager, RpcHandlers),

// if the node isn't actively participating in consensus then it doesn't
// need a keystore, regardless of which protocol we use below.
let keystore =
if role.is_authority() { Some(keystore_container.keystore()) } else { None };
let keystore = if role.is_authority() { Some(keystore_container.keystore()) } else { None };

let grandpa_config = sc_consensus_grandpa::Config {
// FIXME #1578 make this available through chainspec
Expand Down
Loading