Skip to content

Commit

Permalink
263 add optimizations audit recommendations (#392)
Browse files Browse the repository at this point in the history 
* SYS-04 Double Iterations can be merged into a single one

* SRL-02/SRW-02 limit parameter type optimization. get_latest_transactions was removed so no need to change there, makes use of get_transactions

* RPC-01/ Closure usage could simplify the codebase

* PRO-02/PRF-02 Return Type Could be an Option

* LIV-01 Duplicated Helper Function Call

* LIP-04/LI5-04 Potential Unnecessary Computations

* LIP-03/LI5-03 Redundant Condition Check

* Rollback on LIV-01, try_deposit_collateral is also used in other pallets, cannot modify like proposed.

* LII-02/LBS-01  Duplicated Condition Check

* 4FC-06/9B2-06  Loops optimizations with iterators

* refactor for RPC-01

* Fix weird indentation within macro

* Replace `get_old_vault_replace_requests_filter`

* Fix mock type declaration issues

* Change signature of `get_vault_redeem_requests()`

* RPC-01 fixed mock issue by adding constrained trait object, functions are left as generic so to avoid the need of a second map and filter/transform can be done in one iterator

* Fix (potential) bug introduced before in 4FC-06/9B2-06. mut requests passed to drain expired() does hold active requests after execution now.

* Refactor RPC-01. Remove the need for 2 into_iter() calls

* Refactor PRO-02/PRF-02. Also return None when building fails.

* FIX issue introduced by LII-02/LBS-01. Second check is needed.

* Refactor on LIP-04/LI5-04.

* Update pallets/vault-registry/src/lib.rs

remove empty line

Co-authored-by: Marcel Ebert <mail@marcel-ebert.de>

* Update pallets/vault-registry/src/lib.rs

remove empty line

Co-authored-by: Marcel Ebert <mail@marcel-ebert.de>

* format

* clippy changes

---------

Co-authored-by: Marcel Ebert <mail@marcel-ebert.de>
  • Loading branch information
@gianfra-t @ebma
gianfra-t and ebma authored Sep 8, 2023
1 parent 5fc74e0 commit 09eabb6
Show file tree
Hide file tree
Showing 12 changed files with 181 additions and 119 deletions.
73 changes: 53 additions & 20 deletions clients/runtime/src/rpc.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1109,10 +1109,11 @@ pub trait RedeemPallet {
async fn get_redeem_request(&self, redeem_id: H256) -> Result<SpacewalkRedeemRequest, Error>;

/// Get all redeem requests requested of the given vault
async fn get_vault_redeem_requests(
async fn get_vault_redeem_requests<T>(
&self,
account_id: AccountId,
) -> Result<Vec<(H256, SpacewalkRedeemRequest)>, Error>;
filter: Box<dyn Fn((H256, SpacewalkRedeemRequest)) -> Option<T> + Send>,
) -> Result<Vec<T>, Error>;

async fn get_redeem_period(&self) -> Result<BlockNumber, Error>;
}
Expand Down Expand Up @@ -1165,24 +1166,38 @@ impl RedeemPallet for SpacewalkParachain {
.await
}

async fn get_vault_redeem_requests(
async fn get_vault_redeem_requests<T>(
&self,
account_id: AccountId,
) -> Result<Vec<(H256, SpacewalkRedeemRequest)>, Error> {
filter: Box<dyn Fn((H256, SpacewalkRedeemRequest)) -> Option<T> + Send>,
) -> Result<Vec<T>, Error> {
let head = self.get_finalized_block_hash().await?;
let result: Vec<H256> = self
.api
.rpc()
.request("redeem_getVaultRedeemRequests", rpc_params![account_id, head])
.await?;
join_all(
result.into_iter().map(|key| async move {
self.get_redeem_request(key).await.map(|value| (key, value))
}),
)
.await
.into_iter()
.collect()

let redeem_requests = join_all(result.into_iter().map(|key| async move {
self.get_redeem_request(key).await.map(|value| (key, value))
}))
.await;

let mut some_error: Option<Error> = None;
let filtered_results: Vec<_> = redeem_requests
.into_iter()
.filter_map(|item_maybe| match item_maybe {
Ok(item) => filter(item),
Err(e) => {
some_error.get_or_insert(e);
None
},
})
.collect();
match some_error {
Some(err) => Err(err),
None => Ok(filtered_results),
}
}

async fn get_redeem_period(&self) -> Result<BlockNumber, Error> {
Expand Down Expand Up @@ -1256,10 +1271,11 @@ pub trait ReplacePallet {
) -> Result<Vec<(H256, SpacewalkReplaceRequest)>, Error>;

/// Get all replace requests made by the given vault
async fn get_old_vault_replace_requests(
async fn get_old_vault_replace_requests<T: 'static>(
&self,
account_id: AccountId,
) -> Result<Vec<(H256, SpacewalkReplaceRequest)>, Error>;
filter: Box<dyn Fn((H256, SpacewalkReplaceRequest)) -> Option<T> + Send>,
) -> Result<Vec<T>, Error>;

/// Get the time difference in number of blocks between when a replace
/// request is created and required completion time by a vault
Expand Down Expand Up @@ -1352,22 +1368,39 @@ impl ReplacePallet for SpacewalkParachain {
}

/// Get all replace requests made by the given vault
async fn get_old_vault_replace_requests(
async fn get_old_vault_replace_requests<T>(
&self,
account_id: AccountId,
) -> Result<Vec<(H256, SpacewalkReplaceRequest)>, Error> {
filter: Box<dyn Fn((H256, SpacewalkReplaceRequest)) -> Option<T> + Send>,
) -> Result<Vec<T>, Error> {
let head = self.get_finalized_block_hash().await?;
let result: Vec<H256> = self
.api
.rpc()
.request("replace_getOldVaultReplaceRequests", rpc_params![account_id, head])
.await?;
join_all(result.into_iter().map(|key| async move {

let replace_requests = join_all(result.into_iter().map(|key| async move {
self.get_replace_request(key).await.map(|value| (key, value))
}))
.await
.into_iter()
.collect()
.await;

let mut some_error: Option<Error> = None;
let filtered_results: Vec<_> = replace_requests
.into_iter()
.filter_map(|item_maybe| match item_maybe {
Ok(item) => filter(item),
Err(e) => {
some_error.get_or_insert(e);
None
},
})
.collect();

match some_error {
Some(err) => Err(err),
None => Ok(filtered_results),
}
}

async fn get_replace_period(&self) -> Result<u32, Error> {
Expand Down
16 changes: 8 additions & 8 deletions clients/vault/src/cancellation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,16 +169,16 @@ impl<P: IssuePallet + ReplacePallet + Send + Sync> Canceller<P> for ReplaceCance
// verbose drain_filter
fn drain_expired(requests: &mut Vec<ActiveRequest>, current_height: u32) -> Vec<ActiveRequest> {
let mut expired = Vec::new();
let has_expired = |request: &ActiveRequest| current_height > request.parachain_deadline_height;
let mut i = 0;
while i != requests.len() {
if has_expired(&requests[i]) {
let req = requests.remove(i);
expired.push(req);

requests.retain(|req| {
if current_height > req.parachain_deadline_height {
expired.push(*req);
false
} else {
i += 1;
true
}
}
});

expired
}

Expand Down
45 changes: 27 additions & 18 deletions clients/vault/src/execution.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,29 +374,38 @@ pub async fn execute_open_requests(
let parachain_rpc = &parachain_rpc;
let vault_id = parachain_rpc.get_account_id().clone();

// get all redeem and replace requests
let (redeem_requests, replace_requests) = try_join!(
parachain_rpc.get_vault_redeem_requests(vault_id.clone()),
parachain_rpc.get_old_vault_replace_requests(vault_id.clone()),
)?;

let open_redeems = redeem_requests
.into_iter()
.filter(|(_, request)| request.status == RedeemRequestStatus::Pending)
.filter_map(|(hash, request)| {
//closure to filter and transform redeem_requests
let filter_redeem_reqs = move |(hash, request): (H256, SpacewalkRedeemRequest)| {
if request.status == RedeemRequestStatus::Pending {
Request::from_redeem_request(hash, request, payment_margin).ok()
});
} else {
None
}
};

let open_replaces = replace_requests
.into_iter()
.filter(|(_, request)| request.status == ReplaceRequestStatus::Pending)
.filter_map(|(hash, request)| {
//closure to filter and transform replace_requests
let filter_replace_reqs = move |(hash, request): (H256, SpacewalkReplaceRequest)| {
if request.status == ReplaceRequestStatus::Pending {
Request::from_replace_request(hash, request, payment_margin).ok()
});
} else {
None
}
};

// get all redeem and replace requests
let (open_redeems, open_replaces) = try_join!(
parachain_rpc
.get_vault_redeem_requests::<Request>(vault_id.clone(), Box::new(filter_redeem_reqs)),
parachain_rpc.get_old_vault_replace_requests::<Request>(
vault_id.clone(),
Box::new(filter_replace_reqs)
),
)?;

// collect all requests into a hashmap, indexed by their id
let mut open_requests = open_redeems
.chain(open_replaces)
.into_iter()
.chain(open_replaces.into_iter())
.map(|x| (derive_shortened_request_id(&x.hash.0), x))
.collect::<HashMap<_, _>>();

Expand Down Expand Up @@ -461,7 +470,7 @@ pub async fn execute_open_requests(

// All requests remaining in the hashmap did not have a Stellar payment yet, so pay
// and execute all of these
for (_, request) in open_requests {
for (_, request) in open_requests.into_iter() {
// there are potentially a large number of open requests - pay and execute each
// in a separate task to ensure that awaiting confirmations does not significantly
// delay other requests
Expand Down
10 changes: 8 additions & 2 deletions clients/vault/src/metrics.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ use runtime::{
types::currency_id::CurrencyIdExt,
AggregateUpdatedEvent, CollateralBalancesPallet, CurrencyId, Error as RuntimeError, FixedU128,
IssuePallet, IssueRequestStatus, OracleKey, RedeemPallet, RedeemRequestStatus, SecurityPallet,
SpacewalkParachain, UtilFuncs, VaultId, VaultRegistryPallet, H256,
SpacewalkParachain, SpacewalkRedeemRequest, UtilFuncs, VaultId, VaultRegistryPallet, H256,
};
use service::{
warp::{Rejection, Reply},
Expand Down Expand Up @@ -469,8 +469,14 @@ pub async fn poll_metrics<
loop {
publish_native_currency_balance(parachain_rpc).await?;
publish_issue_count(parachain_rpc, vault_id_manager).await;

let pass_all_filter = |item: (H256, SpacewalkRedeemRequest)| Some(item);

if let Ok(redeems) = parachain_rpc
.get_vault_redeem_requests(parachain_rpc.get_account_id().clone())
.get_vault_redeem_requests::<(H256, SpacewalkRedeemRequest)>(
parachain_rpc.get_account_id().clone(),
Box::new(pass_all_filter),
)
.await
{
publish_redeem_count(vault_id_manager, &redeems).await;
Expand Down
20 changes: 9 additions & 11 deletions clients/vault/src/oracle/collector/proof_builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,24 +114,22 @@ impl ScpMessageCollector {
&self,
slot: Slot,
sender: &StellarMessageSender,
) -> UnlimitedVarArray<ScpEnvelope> {
let empty = UnlimitedVarArray::new_empty();

) -> Option<UnlimitedVarArray<ScpEnvelope>> {
if let Some(envelopes) = self.envelopes_map().get(&slot) {
// lacking envelopes
if envelopes.len() < get_min_externalized_messages(self.is_public()) {
tracing::debug!(
"Proof Building for slot {slot}: not enough envelopes to build proof "
);
} else {
return UnlimitedVarArray::new(envelopes.clone()).unwrap_or(empty)
return UnlimitedVarArray::new(envelopes.clone()).ok()
}
}

// forcefully retrieve envelopes
self.fetch_missing_envelopes(slot, sender).await;

empty
return None
}

/// Returns either a TransactionSet or a ProofStatus saying it failed to retrieve the set.
Expand Down Expand Up @@ -179,15 +177,15 @@ impl ScpMessageCollector {
/// * `slot` - the slot where the txset is to get.
/// * `sender` - used to send messages to Stellar Node
pub async fn build_proof(&self, slot: Slot, sender: &StellarMessageSender) -> Option<Proof> {
let envelopes = self.get_envelopes(slot, sender).await;
let envelopes_maybe = self.get_envelopes(slot, sender).await;
// return early if we don't have enough envelopes or the tx_set
if envelopes.len() == 0 {
tracing::debug!("Couldn't built proof for slot {slot} due to missing envelopes");
if let Some(envelopes) = envelopes_maybe {
let tx_set = self.get_txset(slot, sender).await?;
return Some(Proof { slot, envelopes, tx_set })
} else {
tracing::debug!("Couldn't build proof for slot {slot} due to missing envelopes");
return None
}

let tx_set = self.get_txset(slot, sender).await?;
Some(Proof { slot, envelopes, tx_set })
}

/// Insert envelopes fetched from the archive to the map
Expand Down
83 changes: 45 additions & 38 deletions clients/vault/src/replace.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -248,47 +248,54 @@ mod tests {
}

mockall::mock! {
Provider {}

#[async_trait]
pub trait VaultRegistryPallet {
async fn get_vault(&self, vault_id: &VaultId) -> Result<SpacewalkVault, RuntimeError>;
async fn get_vaults_by_account_id(&self, account_id: &AccountId) -> Result<Vec<VaultId>, RuntimeError>;
async fn get_all_vaults(&self) -> Result<Vec<SpacewalkVault>, RuntimeError>;
async fn register_vault(&self, vault_id: &VaultId, collateral: u128) -> Result<(), RuntimeError>;
async fn deposit_collateral(&self, vault_id: &VaultId, amount: u128) -> Result<(), RuntimeError>;
async fn withdraw_collateral(&self, vault_id: &VaultId, amount: u128) -> Result<(), RuntimeError>;
async fn get_public_key(&self) -> Result<Option<StellarPublicKeyRaw>, RuntimeError>;
async fn register_public_key(&self, public_key: StellarPublicKeyRaw) -> Result<(), RuntimeError>;
async fn get_required_collateral_for_wrapped(&self, amount: u128, wrapped_currency: CurrencyId, collateral_currency: CurrencyId) -> Result<u128, RuntimeError>;
async fn get_required_collateral_for_vault(&self, vault_id: VaultId) -> Result<u128, RuntimeError>;
async fn get_vault_total_collateral(&self, vault_id: VaultId) -> Result<u128, RuntimeError>;
async fn get_collateralization_from_vault(&self, vault_id: VaultId, only_issued: bool) -> Result<u128, RuntimeError>;
}
Provider {}

#[async_trait]
pub trait VaultRegistryPallet {
async fn get_vault(&self, vault_id: &VaultId) -> Result<SpacewalkVault, RuntimeError>;
async fn get_vaults_by_account_id(&self, account_id: &AccountId) -> Result<Vec<VaultId>, RuntimeError>;
async fn get_all_vaults(&self) -> Result<Vec<SpacewalkVault>, RuntimeError>;
async fn register_vault(&self, vault_id: &VaultId, collateral: u128) -> Result<(), RuntimeError>;
async fn deposit_collateral(&self, vault_id: &VaultId, amount: u128) -> Result<(), RuntimeError>;
async fn withdraw_collateral(&self, vault_id: &VaultId, amount: u128) -> Result<(), RuntimeError>;
async fn get_public_key(&self) -> Result<Option<StellarPublicKeyRaw>, RuntimeError>;
async fn register_public_key(&self, public_key: StellarPublicKeyRaw) -> Result<(), RuntimeError>;
async fn get_required_collateral_for_wrapped(&self, amount: u128, wrapped_currency: CurrencyId, collateral_currency: CurrencyId) -> Result<u128, RuntimeError>;
async fn get_required_collateral_for_vault(&self, vault_id: VaultId) -> Result<u128, RuntimeError>;
async fn get_vault_total_collateral(&self, vault_id: VaultId) -> Result<u128, RuntimeError>;
async fn get_collateralization_from_vault(&self, vault_id: VaultId, only_issued: bool) -> Result<u128, RuntimeError>;
}

#[async_trait]
pub trait ReplacePallet {
async fn request_replace(&self, vault_id: &VaultId, amount: u128) -> Result<(), RuntimeError>;
async fn withdraw_replace(&self, vault_id: &VaultId, amount: u128) -> Result<(), RuntimeError>;
async fn accept_replace(&self, new_vault: &VaultId, old_vault: &VaultId, amount: u128, collateral: u128, stellar_address: StellarPublicKeyRaw) -> Result<(), RuntimeError>;
async fn execute_replace(&self, replace_id: H256, tx_env: &[u8], scp_envs: &[u8], tx_set: &[u8]) -> Result<(), RuntimeError>;
async fn cancel_replace(&self, replace_id: H256) -> Result<(), RuntimeError>;
async fn get_new_vault_replace_requests(&self, account_id: AccountId) -> Result<Vec<(H256, SpacewalkReplaceRequest)>, RuntimeError>;
async fn get_old_vault_replace_requests(&self, account_id: AccountId) -> Result<Vec<(H256, SpacewalkReplaceRequest)>, RuntimeError>;
async fn get_replace_period(&self) -> Result<u32, RuntimeError>;
async fn get_replace_request(&self, replace_id: H256) -> Result<SpacewalkReplaceRequest, RuntimeError>;
async fn get_replace_dust_amount(&self) -> Result<u128, RuntimeError>;
}


#[async_trait]
pub trait CollateralBalancesPallet {
async fn get_free_balance(&self, currency_id: CurrencyId) -> Result<Balance, RuntimeError>;
async fn get_native_balance_for_id(&self, id: &AccountId) -> Result<Balance, RuntimeError>;
async fn get_free_balance_for_id(&self, id: AccountId, currency_id: CurrencyId) -> Result<Balance, RuntimeError>;
async fn get_reserved_balance(&self, currency_id: CurrencyId) -> Result<Balance, RuntimeError>;
async fn get_reserved_balance_for_id(&self, id: AccountId, currency_id: CurrencyId) -> Result<Balance, RuntimeError>;
async fn transfer_to(&self, recipient: &AccountId, amount: u128, currency_id: CurrencyId) -> Result<(), RuntimeError>; }
#[async_trait]
pub trait ReplacePallet {
async fn request_replace(&self, vault_id: &VaultId, amount: u128) -> Result<(), RuntimeError>;
async fn withdraw_replace(&self, vault_id: &VaultId, amount: u128) -> Result<(), RuntimeError>;
async fn accept_replace(&self, new_vault: &VaultId, old_vault: &VaultId, amount: u128, collateral: u128, stellar_address: StellarPublicKeyRaw) -> Result<(), RuntimeError>;
async fn execute_replace(&self, replace_id: H256, tx_env: &[u8], scp_envs: &[u8], tx_set: &[u8]) -> Result<(), RuntimeError>;
async fn cancel_replace(&self, replace_id: H256) -> Result<(), RuntimeError>;
async fn get_new_vault_replace_requests(&self, account_id: AccountId) -> Result<Vec<(H256, SpacewalkReplaceRequest)>, RuntimeError>;
async fn get_old_vault_replace_requests<T: 'static>(
&self,
account_id: AccountId,
filter: Box<dyn Fn((H256, SpacewalkReplaceRequest)) -> Option<T> + Send>,
) -> Result<Vec<T>, RuntimeError>;
async fn get_replace_period(&self) -> Result<u32, RuntimeError>;
async fn get_replace_request(&self, replace_id: H256) -> Result<SpacewalkReplaceRequest, RuntimeError>;
async fn get_replace_dust_amount(&self) -> Result<u128, RuntimeError>;
}


#[async_trait]
pub trait CollateralBalancesPallet {
async fn get_free_balance(&self, currency_id: CurrencyId) -> Result<Balance, RuntimeError>;
async fn get_native_balance_for_id(&self, id: &AccountId) -> Result<Balance, RuntimeError>;
async fn get_free_balance_for_id(&self, id: AccountId, currency_id: CurrencyId) -> Result<Balance, RuntimeError>;
async fn get_reserved_balance(&self, currency_id: CurrencyId) -> Result<Balance, RuntimeError>;
async fn get_reserved_balance_for_id(&self, id: AccountId, currency_id: CurrencyId) -> Result<Balance, RuntimeError>;
async fn transfer_to(&self, recipient: &AccountId, amount: u128, currency_id: CurrencyId) -> Result<(), RuntimeError>;
}
}

impl Clone for MockProvider {
Expand Down
Loading

0 comments on commit 09eabb6

Please sign in to comment.