4FC-05 Commented out code

[ebma]

We need to clean up some commented-out code and TODO comments that were introduced by #385.

[ebma]

@pendulum-chain/product this is a follow-up to the changes for #257 and we should work on this very soon. We only split this out of the original ticket to progress faster with fixing the Spacewalk issues on testnet.

[prayagd]

Hey team! Please add your planning poker estimate with Zenhub @adelarja @b-yap @ebma @gianfra-t @TorstenStueber

[annatekl]

@ebma @b-yap is this relevant to pick it up for the current spacewalk launch?

[ebma]

No @annatekl. But wouldn't hurt to work on this soon.

[TorstenStueber]

@prayagd @annatekl Any reason this is not in Ready yet? Can we already move it there?

[b-yap]

This ticket also requires cleaning up "todos" and commented codes.
But here are the following todos carried over from Interlay:

• spacewalk/clients/runtime/src/rpc.rs

  Line 605 in 56b9601

  // TODO: check MinimumDeposit

  Interlay code here

• spacewalk/pallets/vault-registry/src/tests.rs

  Line 565 in 56b9601

  // TODO: emulate assert_called

  Interlay code here

• spacewalk/pallets/vault-registry/src/lib.rs

  Line 1085 in 56b9601

  // todo: do we need to do this for griefing as well?

  Interlay code here

• spacewalk/pallets/vault-registry/src/types.rs

  Line 627 in 56b9601

  // todo: clear replace collateral?

  Interlay code here

• spacewalk/pallets/issue/src/lib.rs

  Line 557 in 56b9601

  // TODO: update `issue.griefing_collateral`?

  Interlay code here

• spacewalk/pallets/issue/src/lib.rs

  Line 710 in 56b9601

  // TODO: update griefing collateral

  Interlay code here

• spacewalk/pallets/redeem/src/lib.rs

  Lines 593 to 594 in 56b9601

  	// todo: currently allowed to redeem from one currency to the other for free - decide if
  	// this is desirable

  Interlay code here

• spacewalk/pallets/staking/src/lib.rs

  Line 196 in 56b9601

  // TODO: conditionally compile this

  Interlay code here

• spacewalk/pallets/staking/src/lib.rs

  Line 619 in 56b9601

  // TODO: this can probably be saturated

  Interlay code here

• spacewalk/pallets/reward/src/lib.rs

  Lines 245 to 249 in 56b9601

  	// FIXME: this can easily overflow with large numbers
  	let stake_mul_reward_per_token =
  	stake.checked_mul(&reward_per_token).ok_or(ArithmeticError::Overflow)?;
  	let reward_tally = <RewardTally<T, I>>::get(currency_id, account_id);
  	// TODO: this can probably be saturated

  Interlay code here

We can keep them as is, or cherry pick any that we think we can do ourselves

[ebma]

Thanks for the nice overview @b-yap 🙏 The code samples of interlay's code you linked to are all still occurring on their latest main branch? If so, I think it's fine if we don't fix them for now.

[b-yap]

@ebma question:

#257 (comment)

one of the concerns of 4FC-05 Commented out code are

spacewalk/clients/vault/src/system.rs

Lines 718 to 720 in ad68c16

	// TODO
	// faucet::fund_and_register(&self.spacewalk_parachain, faucet_url, &vault_id)
	// .await?;

and

spacewalk/clients/vault/src/system.rs

Line 660 in ad68c16

// TODO fund account with faucet

Do you know what to do here, or is a separate ticket needed to address this?

You redirected to this ticket -> Add faucet to send test tokens to accounts #161

If I understand correctly, the deployed faucet is only for testnet:

if let Some(faucet_url) = &self.config.faucet_url {
      let is_public_network =
        self.spacewalk_parachain.is_public_network().await.unwrap_or_else(|error| {
          // Sometimes the fetch fails with 'StorageItemNotFound' error.
          // We assume public network by default
          tracing::warn!(
          "Failed to fetch public network status from parachain: {error}. Assuming public network."
        );
          true
        });

      if !is_public_network {
        // use account id of the spacewalk parachain
        let account_id = self.spacewalk_parachain.get_account_id().pretty_print();
        let url = format!("{faucet_url}?to={account_id}");
        let response = reqwest::get(url.clone()).await;
      }

    }

Is it correct to use the parachain's account_id?

The log:

Feb 13 22:54:25.198  INFO vault::system: THE FULL URL: https://faucet-service.pendulumchain.tech/fund?to=6izw2Zx6zcgA44G51ptCKWbsLeyDE4hgVcvKifAkfDr2tATc
Feb 13 22:54:27.928  INFO vault::system: THE RESPONSE: Ok(Response { url: Url { scheme: "https", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("faucet-service.pendulumchain.tech")), port: None, path: "/fund", query: Some("to=6izw2Zx6zcgA44G51ptCKWbsLeyDE4hgVcvKifAkfDr2tATc"), fragment: None }, status: 500, headers: {"date": "Tue, 13 Feb 2024 14:54:27 GMT", "content-type": "text/html; charset=utf-8", "content-length": "36", "connection": "keep-alive", "ratelimit-policy": "60;w=60", "ratelimit-limit": "60", "ratelimit-remaining": "59", "ratelimit-reset": "60", "content-security-policy": "default-src 'self';base-uri 'self';font-src 'self' https: data:;form-action 'self';frame-ancestors 'self';img-src 'self' data:;object-src 'none';script-src 'self';script-src-attr 'none';style-src 'self' https: 'unsafe-inline';upgrade-insecure-requests", "cross-origin-opener-policy": "same-origin", "cross-origin-resource-policy": "same-origin", "origin-agent-cluster": "?1", "referrer-policy": "no-referrer", "strict-transport-security": "max-age=15724800; includeSubDomains", "x-content-type-options": "nosniff", "x-dns-prefetch-control": "off", "x-download-options": "noopen", "x-frame-options": "SAMEORIGIN", "x-permitted-cross-domain-policies": "none", "x-xss-protection": "0", "access-control-allow-origin": "*", "etag": "W/\"24-EgLvasLBtN4AHHuzd2PfDR6mVDg\""} })

When I ran the url on browser, it shows
Server Error. Please try again later.

[gianfra-t]

@b-yap It seems that the faucet's account does not have enough funds. A slack message was sent to the devops incident channel. That must be it!

[b-yap]

thanks for the heads up, @gianfra-t !

[b-yap]

I started to work on

spacewalk/pallets/replace/src/benchmarking.rs

Line 154 in 35a4895

// TODO: check that an amount was actually withdrawn

And found out that Interlay made a few changes: chore: replace benchmarks #1025

I was following the changes but found some differences that made it too much, should I truly use thiers.
Example:

• Interlay use orml_tokens to deposit amounts, while we use orml_currencies.
• Interlay has additional storages like Aggregate that spacewalk doesn't have.
• Interlay's setup has BTC related code that we don't need.

Instead, I will only verify the changes directly:

withdraw_replace {
		...
		register_vault::<T>(vault_id.clone());
		...
		let vault : DefaultVault::<T> = VaultRegistry::<T>::get_vault_from_id(&vault_id).expect("should return a vault");
		let to_be_replaced_tokens = vault.to_be_replaced_tokens;
	}: _(RawOrigin::Signed(vault_id.account_id.clone()), vault_id.currencies.clone(), amount.amount())
	verify {
		let vault : DefaultVault::<T> = VaultRegistry::<T>::get_vault_from_id(&vault_id).expect("should return a vault");
		let updated_to_be_replaced_tokens = vault.to_be_replaced_tokens;

		assert!(to_be_replaced_tokens > updated_to_be_replaced_tokens);
	}

But currently have to deal with:

2024-02-14 20:49:23 panicked at 'Expected Ok(_). Got Err(
    Module(
        ModuleError {
            index: 19,
            error: [
                0,
                0,
                0,
                0,
            ],
            message: Some(
                "ParachainNotRunning",
            ),
        },
    ),
)', /Users/b.carlayap/projects/spacewalk/pallets/redeem/src/benchmarking.rs:156:9    
Error: Input("Error executing and verifying runtime benchmark: Execution aborted due to trap: wasm trap: wasm `unreachable` instruction executed\nWASM backtrace:\nerror while executing at wasm backtrace:\n    0: 0x3532d - <unknown>!rust_begin_unwind\n    1: 0x980a - <unknown>!core::panicking::panic_fmt::h77cb6fbeac3ac4ae\n    2: 0xbdf48 - <unknown>!<redeem::benchmarking::SelectedBenchmark as frame_benchmarking::utils::BenchmarkingSetup<T>>::instance::h674cb72c20b90907\n    3: 0x156fb1 - <unknown>!redeem::benchmarking::<impl frame_benchmarking::utils::Benchmarking for redeem::pallet::Pallet<T>>::run_benchmark::h74c840d51638c276\n    4: 0x12f2f6 - <unknown>!<spacewalk_runtime_standalone::Runtime as frame_benchmarking::utils::runtime_decl_for_benchmark::BenchmarkV1<sp_runtime::generic::block::Block<sp_runtime::generic::header::Header<u32,sp_runtime::traits::BlakeTwo256>,sp_runtime::generic::unchecked_extrinsic::UncheckedExtrinsic<sp_runtime::multiaddress::MultiAddress<<<sp_runtime::MultiSignature as sp_runtime::traits::Verify>::Signer as sp_runtime::traits::IdentifyAccount>::AccountId,()>,spacewalk_runtime_standalone::RuntimeCall,sp_runtime::MultiSignature,(frame_system::extensions::check_spec_version::CheckSpecVersion<spacewalk_runtime_standalone::Runtime>,frame_system::extensions::check_tx_version::CheckTxVersion<spacewalk_runtime_standalone::Runtime>,frame_system::extensions::check_genesis::CheckGenesis<spacewalk_runtime_standalone::Runtime>,frame_system::extensions::check_mortality::CheckMortality<spacewalk_runtime_standalone::Runtime>,frame_system::extensions::check_nonce::CheckNonce<spacewalk_runtime_standalone::Runtime>,frame_system::extensions::check_weight::CheckWeight<spacewalk_runtime_standalone::Runtime>,pallet_transaction_payment::ChargeTransactionPayment<spacewalk_runtime_standalone::Runtime>)>>>>::dispatch_benchmark::h6066f53bffd4da82\n    5: 0x1177ec - <unknown>!Benchmark_dispatch_benchmark")

after running:

./target/release/spacewalk-standalone benchmark pallet \
    --chain dev \
    --execution=wasm \
    --wasm-execution=compiled \
    --pallet "*" \
    --extrinsic "*" \
    --steps 50 \
    --repeat 20 \
    --output pallets/all-weight.rs

[ebma]

The 'ParachainNotRunning' error indicates that the oracle pallet does not have all the prices available that it needs. The 'need' is defined by the currencies added to the 'oracle_keys' of the oracle pallet which you can check in the chain_spec. Did we change something about it? I don't think so 🤔

[b-yap]

@ebma
Not sure what the actual fix is; I rearranged the code of both benchmarks of Replace and Redeem pallets, following the benchmark of the Issue pallet since it worked fine.

[ebma]

Hmm okay weird, not sure what it fixed it in your comment.