Refactor: vault_swap should take TransactionInId #5409

Janislav · 2024-11-11T12:57:48Z

Pull Request

Closes: PRO-1760

Checklist

Please conduct a thorough self-review before opening the PR.

I am confident that the code works.
I have written sufficient tests.
I have written and tested required migrations.
I have updated documentation where appropriate.

Summary

Replaces the TransactionHash with TransactionInId
Did all the necessary refactor to handle the convert between the usage of generic and none-generic pallets
Fixed all tests and benchmarks
Change the engine where necessary

Non-Breaking changes

This is a breaking change, but I don't think we touched anything that is live yet. That means we should check with the frontends. The following things have chanded:

The vault_swap_request extrinsic in the ingress egress pallet -> We are changed the name of tx_hash to tx_id
The SwapRequested event in the swapping pallet -> tx_hash is now tx_id and the SwapOrigin is SwapOrigin now

In terms of migration, I think we are good. I didn't see any usage SwapOrigin in or former tx_hash in storage, which means there is no need for a migration.

codecov · 2024-11-11T13:08:27Z

Codecov Report

Attention: Patch coverage is 71.60494% with 23 lines in your changes missing coverage. Please review.

Project coverage is 71%. Comparing base (c22b70a) to head (248016f).

Files with missing lines	Patch %	Lines
state-chain/chains/src/lib.rs	68%	3 Missing and 3 partials ⚠️
state-chain/pallets/cf-ingress-egress/src/lib.rs	60%	4 Missing and 2 partials ⚠️
engine/src/witness/evm/vault.rs	0%	4 Missing ⚠️
engine/src/witness/arb.rs	0%	3 Missing ⚠️
engine/src/witness/eth.rs	0%	3 Missing ⚠️
state-chain/pallets/cf-swapping/src/lib.rs	50%	1 Missing ⚠️

Additional details and impacted files

@@          Coverage Diff           @@
##            main   #5409    +/-   ##
======================================
- Coverage     71%     71%    -0%     
======================================
  Files        489     489            
  Lines      86667   86563   -104     
  Branches   86667   86563   -104     
======================================
- Hits       61958   61794   -164     
- Misses     21798   21853    +55     
- Partials    2911    2916     +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

albert-llimos · 2024-11-11T17:22:27Z

@Janislav I know this is a draft but I've seen that you've used SolHash for Solana, which seems to be the obvious option. However, due to witnessing shortcomings, we will need to use (SolAddress, u64) for Solana as AccountAddress and slot number. I wrote it in the Linear comments but I can see how it was not too clear.
https://linear.app/chainflip/issue/PRO-1760/vault-swaps-should-take-transactioninid-instead-of-transactionhash#comment-26f67a54
Let me know if anything is unclear.

Janislav · 2024-11-11T17:36:12Z

Mhhh... We will and can use whatever we decide the TransactionInId for Solana should be, but I think this should be not part of this PR, right? I assume changing this has also some other side effects and deserves it's own PR/Ticket I guess.

albert-llimos · 2024-11-11T17:47:54Z

Mhhh... We will and can use whatever we decide the TransactionInId for Solana should be, but I think this should be not part of this PR, right? I assume changing this has also some other side effects and deserves it's own PR/Ticket I guess.

I was thinking that you can already declare it as the correct type for Solana, as the SolHash is not what we want. The work on the engine to report the correct value for that will of course be in another PR.

Janislav · 2024-11-11T19:35:46Z

Okay, since we are really not using TransactionInId for anything for Solana right now, we might get away with just changing the type. I was worried it could be more complex than that (migrations, tests, benchmark, engine). In this case we can do it straight in this PR I think.

Janislav · 2024-11-12T08:59:10Z

bouncer/shared/utils.ts

-          (typeof id === 'string' && 'Vault' in data.origin && data.origin.Vault.txHash === id);
+          (typeof id === 'string' &&
+            'Vault' in data.origin &&
+            data.origin.Vault.txHash.ByteHash === id);


FYI: For now, it's safe to assume that we always have to deal with EVM or BTC, so it will be always a H256 hash string. If this should change, we have to handle this somehow.

We have the Solana Vault swaps PR in the pipeline that will definitely break that. I'll make sure to update that after this is complete. Thanks for pointing that out.

- Added a struct that can convert from a generic TxID to a static TxID representation - Integrated the converter into the runtime - Fixed the witnessing

dandanlen

I don't think we need a generic SwapOrigin: The only reason we ever construct a SwapOrigin<TxInId> is to convert it to a SwapOrigin<TxInIdAnyChain>. Instead of building what we don't want and then converting it to what we need, we could just build what we need. We just need a way to wrap the chain-specific id to the the 'any chain' enum.

dandanlen · 2024-11-13T12:23:52Z

state-chain/chains/src/lib.rs

+use cf_primitives::TxId;
+use sp_core::H256;


nit: please group these with the rest of the imports.

dandanlen · 2024-11-13T12:30:41Z

state-chain/chains/src/lib.rs

+pub trait ConvertTransactionInIdToAnyChain<TransactionInId> {
+	fn convert(origin: SwapOrigin<TransactionInId>) -> SwapOrigin<TransactionInIdForAnyChain>;
+}


Do we really need a new trait? Can we not just add a constraint to TransactionInId : Into<TransactionInIdForAnyChain, and a map method to SwapOrigin?

dandanlen · 2024-11-13T12:49:43Z

state-chain/chains/src/lib.rs

+#[derive(Debug, Clone, TypeInfo, Encode, Decode, PartialEq, Eq)]
+pub enum TransactionInIdForAnyChain {
+	// Used by Ethereum, Arbitrum and BTC.
+	ByteHash(H256),


I think it makes sense to split this up per ChainCrypto, and also use scoped types for clarity:

Bitcoin(<BitcoinCrypto as ChainCrypto>::TransactionInId), Evm(<EvmCrypto as ChainCrypto>::TransactionInId), // etc.

For example we might want to use bitcoin's backward-serialized tx hash type without impacting what we use for Eth/Arb.

dandanlen · 2024-11-13T12:56:02Z

state-chain/chains/src/lib.rs

 	DepositChannel {
 		deposit_address: address::EncodedAddress,
 		channel_id: ChannelId,
 		deposit_block_height: u64,
 	},
 	Vault {
-		tx_hash: TransactionHash,
+		tx_id: TransactionInId,


It's bit odd that SwapOrigin::DepositChannel uses EncodedAddress (ie. it's not chain-specific) but SwapOrigin::Vault is a chain-specific variant... 🤔

Do we need SwapOrigin to be generic at all? Can this variant not just be concrete?

Vault { tx_id: TransactionInIdForAnyChain }

Janislav commented Nov 12, 2024

View reviewed changes

Janislav force-pushed the refactor/pro-1760/use-txinid-in-vault-swaps branch from cabf11d to 615f472 Compare November 12, 2024 10:42

Janislav marked this pull request as ready for review November 12, 2024 12:33

Janislav requested review from kylezs, dandanlen and martin-chainflip as code owners November 12, 2024 12:33

Janislav added 9 commits November 13, 2024 09:52

chore: added type change

e492197

feature: Added Generic TxID converter

7a19bc1

- Added a struct that can convert from a generic TxID to a static TxID representation - Integrated the converter into the runtime - Fixed the witnessing

chore: Fixed tests and clippy

ea830af

chore: renaming

0ca168a

tests: Expecting ByteHash in bouncer tetsts

2e3dfc6

chore: Changed TransactionInId for Solana

3109b80

refactor: Change naming from tx_hash to tx_id

a119d95

refactor: Changed txHash to txId also in bouncer

2633e23

fix: fixed broken tests after reabse

248016f

Janislav force-pushed the refactor/pro-1760/use-txinid-in-vault-swaps branch from 615f472 to 248016f Compare November 13, 2024 10:27

dandanlen reviewed Nov 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: vault_swap should take TransactionInId #5409

Refactor: vault_swap should take TransactionInId #5409

Janislav commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 11, 2024 •

edited

Loading

albert-llimos commented Nov 11, 2024

Janislav commented Nov 11, 2024

albert-llimos commented Nov 11, 2024

Janislav commented Nov 11, 2024

Janislav Nov 12, 2024

albert-llimos Nov 12, 2024

dandanlen left a comment •

edited

Loading

dandanlen Nov 13, 2024

dandanlen Nov 13, 2024

dandanlen Nov 13, 2024 •

edited

Loading

dandanlen Nov 13, 2024

Refactor: vault_swap should take TransactionInId #5409

Are you sure you want to change the base?

Refactor: vault_swap should take TransactionInId #5409

Conversation

Janislav commented Nov 11, 2024 • edited Loading

Pull Request

Checklist

Summary

Non-Breaking changes

codecov bot commented Nov 11, 2024 • edited Loading

Codecov Report

albert-llimos commented Nov 11, 2024

Janislav commented Nov 11, 2024

albert-llimos commented Nov 11, 2024

Janislav commented Nov 11, 2024

Janislav Nov 12, 2024

Choose a reason for hiding this comment

albert-llimos Nov 12, 2024

Choose a reason for hiding this comment

dandanlen left a comment • edited Loading

Choose a reason for hiding this comment

dandanlen Nov 13, 2024

Choose a reason for hiding this comment

dandanlen Nov 13, 2024

Choose a reason for hiding this comment

dandanlen Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

dandanlen Nov 13, 2024

Choose a reason for hiding this comment

Janislav commented Nov 11, 2024 •

edited

Loading

codecov bot commented Nov 11, 2024 •

edited

Loading

dandanlen left a comment •

edited

Loading

dandanlen Nov 13, 2024 •

edited

Loading