417 Fix 4FC-05 commented out code

[b-yap]

closes #417 and #277

Please review pendulum-chain/substrate-stellar-sdk#25 first.

Aside from the following todos that will not be included in this PR, 2 big things need more info:

• The benchmark changes can be reviewed easily with this commit: efc306c .

---

For

spacewalk/clients/wallet/src/resubmissions.rs

Lines 250 to 251 in bda384b

	/// TODO: This operation is very costly and we should try to optimize it in the future.
	async fn is_transaction_already_submitted(&self, tx: &Transaction) -> bool {

• Firstly get the iterator:

  let mut iter = match self.get_all_transactions_iter().await {
   		Ok(iter) => iter,
   		Err(e) => {
   			tracing::warn!("is_transaction_already_submitted(): failed to get iterator: {e:?}");
   			return false
   		},
   	};

  of datatype TransactionsResponseIter containing a list of transactions (field records).

• The records is sorted in DESCENDING order, which means the sequence number of the transaction will be smaller the further we iterate. The loop goes through the records in this order:

  • top of the list
  • middle of the list
  • end of the list

• Compare the target transaction to the transaction of the list in this order:

   fn _check_transaction_match(
    tx: &Transaction,
    tx_resp: &TransactionResponse,
    public_key: &PublicKey,
   ) -> Result<bool, Option<SequenceNumber>> {
    // Make sure that we are the sender and not the receiver because otherwise an
    // attacker could send a transaction to us with the target memo and we'd wrongly
    // assume that we already submitted this transaction.
    if !is_source_account_match(public_key, &tx_resp) {
  	return Err(None)
    }
  
    let Ok(source_account_sequence)  = tx_resp.source_account_sequence() else {
  	tracing::warn!("_check_transaction_match(): cannot extract sequence number of transaction response: 
    {tx_resp:?}");
  	return Err(None)
    };
  
    // check if the sequence number is the same as this response
    if tx.seq_num == source_account_sequence {
  	// Check if the transaction contains the memo we want to send
  	return Ok(is_memo_match(tx, &tx_resp))
    }
  
    Err(Some(source_account_sequence))
  }

  a description can be found on top of this function.

• If the transaction at the middle of the list is not a match, use its sequence number to determine where a match is possible. Remove half of that list to narrow down the search.

  match _check_transaction_match(tx, &response, public_key) {
   	Ok(res) => return Some(res),
   	Err(Some(source_account_sequence)) => {
   		// if the sequence number is GREATER than this response,
   		// then a match must be in the first half of the list.
   		if tx_sequence_num > source_account_sequence {
   			iter.remove_last_half_records();
   		}
   		// a match must be in the last half of the list.
   		else {
   			iter.remove_first_half_records();
   		}
   	},
   	_ => {},
   }

• If the transaction at the end of the list is not a match, use its sequence number to determine if a match can be found in the current records list, or on the next page.

    match _check_transaction_match(tx, &response, public_key) {
    	Ok(res) => return Some(res),
    	Err(Some(source_account_sequence)) => {
    		// if the sequence number is LESSER than this response,
    		// then a match is possible on the NEXT page
    		if tx_sequence_num < source_account_sequence {
    			if let None = iter.jump_to_next_page().await {
    				// there's no pages left, meaning there's no other transactions to compare
    				return Some(false)
    			}
    		}
    	},
    	_ => {},  

[b-yap]

addresses:

spacewalk/testchain/runtime/src/lib.rs

Lines 451 to 460 in bda384b

	// todo: this code results in Execution error:
	// todo: \"Unable to get required collateral for amount\":
	// todo: Module(ModuleError { index: 19, error: [0, 0, 0, 0], message: None })", data: None
	// } cfg_if::cfg_if! {
	// if #[cfg(not(feature = "testing-utils"))] {
	// if token_symbol == 0 {
	// return Some((b"Kusama".to_vec(), b"KSM".to_vec()))
	// }
	// }
	// }

testing ONE intregation test case will work.

[ebma]

What do you mean with 'testing ONE integration test case will work'? Do these 'testing-utils' import solve the problem you linked to?

[b-yap]

when we wanted to test only 1 case:

cargo test test_redeem_succeeds_on_mainnet -p vault --features standalone-metadata,integration

It goes inside the

if #[cfg(not(feature = "testing-utils"))] {

block of code, of testchain-runtime.

For

cargo test --all --all-features

it doesn't.

My theory is: when running a singular test case in the vault, it doesn't make use of the runtime's dev-dependencies. But the runtime clearly specifies to use the testing-utils feature of testchain-runtime.

spacewalk/clients/runtime/Cargo.toml

Line 71 in bda384b

testchain-runtime = { package = "spacewalk-runtime-standalone", path = "../../testchain/runtime", features = ["testing-utils"] }

The vault's dev-dependencies calls for the testing-utils feature of the runtime. But as you can see in runtime, its testing-utils feature is using the defaults of testchain-runtime and oracle.
That's why it got confusing why the cfg-if didn't work in testchain-runtime.

[b-yap]

again, this is to address

spacewalk/clients/vault/tests/helper/mod.rs

Line 33 in bda384b

// TODO clean this up by extending the `get_test_secret_key()` function

[ebma]

I think for these functions, we also need two different secret keys for the mainnet tests. I think we need one for the 'source' account and another one for the 'destination' at least in the integration tests.

[b-yap]

I followed how Interlay did it.

[ebma]

The link is broken :x

[b-yap]

here: interlay/interbtc-clients@622f36b

[b-yap]

Slot is available and can be used:

spacewalk/clients/wallet/src/types.rs

Line 6 in bda384b

pub type Slot = u32;

so there's really no need for type Ledger.

[ebma]

We have an iceboxed ticket that is about this Ledger vs Slot naming right? Can we maybe even just close it then?

[ebma]

Great changes @b-yap, thanks for taking the time to clean this all up 👍
I left just a few minor comments. As far as I understand you improved that resubmission check with some kind of variation of a binary search which should speed it up indeed.

[ebma]

This will change when merging the PR for the mainnet integration tests into this one. Then we'll need to do what I mentioned in my other comment, ie. also create this function for mainnet.

[b-yap]

i updated the function from 2 to only 1:

pub fn get_secret_key(with_currency: bool, is_mainnet: bool) -> String {
	let suffix = if with_currency { "_with_currency" } else { "" };
	let directory = if is_mainnet { "mainnet" } else { "testnet" };

	let path = format!("./resources/secretkey/{directory}/stellar_secretkey_{directory}{suffix}");
	std::fs::read_to_string(path).expect("should return a string")
}

[ebma]

We have an iceboxed ticket that is about this Ledger vs Slot naming right? Can we maybe even just close it then?

[ebma]

This cannot be reached with the previous if statement. I think we should switch the order of the two if statements.

[b-yap]

i updated the code so it does not "assume" the last element; I MADE it check the last element.

[ebma]

Some checks in this function are duplicated with is_transaction_already_submitted(). Wouldn't it be enough to have these checks in one of the two functions?

[b-yap]

got it in 1 function: fn _check_transaction_match(...) though the return datatype is quite confusing, so I added comments on the function;
including the ff functions:

• fn check_last_transaction_match(...)
• check_middle_transaction_match(...)

It is is some kind of binary search; but the checking of the last element plays a role: whether the transaction can be found on the next page or not.

[ebma]

The return types of the helper functions are not ideal but alternatively we'd have to create enum types for the return values which is overkill just for these helpers so I'm fine if we keep it like this. The doc comments you put are great and should be good enough for this.

Can we close this ticket once this PR is merged? I saw you changed some things about the Slot after all.

[b-yap]

I had to cancel the CI because Gianni's PR is still running.
I'll get it to run after the other one finishes.

[ebma]

The ubuntu CI passed so we can already merge this one though, can't we @b-yap?