feat(tendermint): pubkey-only activation and unsigned tx #2088

onur-ozkan · 2024-03-13T13:12:54Z

This PR implements pubkey-only mode for the Tendermint protocol, which means we can use any external wallet for wallet and swap operations on Tendermint.

Additionally, ibc_withdraw RPC is removed and withdraw is refactored for Tendermint to support IBC transfers by automatically finding IBC channels whenever possible.

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy

Great work! This is an initial review since the PR is only in progress but I wanted to start reviewing it early due to the high priority of this PR.

mm2src/coins/tendermint/tendermint_coin.rs

shamardy · 2024-03-27T00:51:30Z

mm2src/coins/tendermint/tendermint_coin.rs

+                #[cfg(target_arch = "wasm32")]
+                PrivKeyPolicy::Metamask(_) => unreachable!(),


Metamask was supposed to be for any similar wallet once we start integrating others, you can see that it uses EthMetamaskPolicy which includes the public key

komodo-defi-framework/mm2src/coins/lp_coins.rs

Lines 3601 to 3610 in c1428fe

#[cfg(target_arch = "wasm32")]

Metamask(EthMetamaskPolicy),

}

#[cfg(target_arch = "wasm32")]

#[derive(Clone, Debug)]

pub struct EthMetamaskPolicy {

pub(crate) public_key: EthH264,

pub(crate) public_key_uncompressed: EthH520,

}

We have 2 options:
1 - Either use generics and rename metamask to DappCompatibleWallet or something similar
2 - keep metamask but we should move it to EthPrivKeyPolicy instead in the future.

I think this is outside this PR scope anyway but I will leave this comment as a reminder for whoever will get to refactor this.

fwiw, initially I also designed this to be a central/generic implementation in mm2, but considering the time we have, I couldn't afford the time allocation to do that.

mm2src/coins/tendermint/tendermint_coin.rs

borngraced

Really great work!, a question and couple changes after first review

mm2src/coins/tx_history_storage/wasm/tx_history_storage_v2.rs

mm2src/coins/tendermint/tendermint_token.rs

borngraced · 2024-03-27T05:26:38Z

mm2src/coins/tendermint/tendermint_tx_history_v2.rs

+                            tx: TransactionData::Signed {
+                                tx_hash: tx_hash.to_string(),
+                                tx_hex: msg.into(),
+                            },


I see that we are constructing TransactionData::Signed a lot of time. what about creating an helper function e.g

fn construct_signed_tx_from_hash_hex<Hash: ToString>(hash: Hash, hex: BytesJson) -> TransactionData {}

just some thoughts 💭

What about TransactionData::new_signed(tx_hash, tx_hex) ? The construct_signed_tx_from_hash_hex name makes it even more complicated than before in my opinion.

onur-ozkan · 2024-03-27T07:36:40Z

Thanks for reviews. As this is an early-stage PR, things can be very different depending on the other blocker tasks I am working on. I will be working on the other/blocker tasks and come back here once I finish other parts.

… into keplr-and-external-sign-swap

Signed-off-by: onur-ozkan <work@onurozkan.dev>

… into keplr-and-external-sign-swap

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy

Amazing Work! Only some minor comments, I will do another recheck of tendermint_coin.rs and tendermint_token.rs early next week before approving the PR!

mm2src/coins/lp_coins.rs

mm2src/coins/rpc_command/tendermint/ibc_transfer_channels.rs

mm2src/coins/rpc_command/tendermint/mod.rs

mm2src/mm2_core/src/mm_ctx.rs

shamardy · 2024-05-10T13:01:38Z

mm2src/coins_activation/src/tendermint_with_assets_activation.rs

+            if ctx.is_watcher() || ctx.use_watchers() {
+                return MmError::err(TendermintInitError {
+                    ticker: ticker.clone(),
+                    kind: TendermintInitErrorKind::CantUseWatchersWithPubkeyPolicy,
+                });


Watchers are not implemented for tendermint yet but I believe they can be implemented for external wallets too. It's up to you if you want to remove this check at this stage or not.

Can you give a little bit more context about watchers using external wallet?

It's just another signed transaction that gets sent to the watcher where they can add the secret once revealed by the maker, so it will be one more transaction signed by keplr. Maybe for Tendermint watchers support, we will need some additions to nucleus codebase, I need to check this but it's not a priority currently.

If I remember correctly, we made watchers so people don't have to keep the mm2 open for the swaps. If user has to be online/active on the keplr window, how does using watcher make sense? I think we should not allow doing that as it doesn't seem beneficial for users and adds tons of complexity on the codebase to maintain.

If I remember correctly, we made watchers so people don't have to keep the mm2 open for the swaps. If user has to be online/active on the keplr window, how does using watcher makes sense?

The main reason we need watchers is not offline swaps but to fix some security issues, if the taker goes offline after sending their payment for a long time the maker can get both payments. Watchers start their work after the taker sends their payment, so for keplr the taker will sign 2 transactions, one for the taker payment and another to send to the watchers, then if anything happens and they went offline, the watcher can complete the swap or refund it before the maker refunds theirs.

P.S. if a swap fails, the taker has to wait some time before they can refund it. If they didn't get back online after some time to refund it, and the maker is malicious, they can lose their payment.

shamardy · 2024-05-10T14:34:21Z

mm2src/coins/tendermint/tendermint_coin.rs

+    let metadata_list = git_controller
+        .client
+        .get_file_metadata_list(
+            CHAIN_REGISTRY_REPO_OWNER,
+            CHAIN_REGISTRY_REPO_NAME,
+            CHAIN_REGISTRY_BRANCH,
+            CHAIN_REGISTRY_IBC_DIR_NAME,
+        )
+        .await
+        .map_err(|e| IBCTransferChannelsRequestError::Transport(format!("{:?}", e)))?;


I know it's our repo, but is it completely secure to rely on this repo for ibc channels data? We have to consider supply chain attacks, what would happen if a wrong channel id is used for a transfer?
P.S. the alternative would be to load a file like the coins file and it will be updated with each new GUI release while relying on the user to enter custom channel ids for new IBC channels similar to custom token imports. What do you think @onur-ozkan?

c.c. @ca333

I know it's our repo, but is it completely secure to rely on this repo for ibc channels data? We have to consider supply chain attacks, what would happen if a wrong channel id is used for a transfer?

Either our repo or user's hostname configuration must be hacked (which is already ..... situation) in order to provide incorrect channels. But that's already more dangerous than providing incorrect channels.

The things which may happen when using wrong channel:

By default, if using a channel id that doesn't exists, funds will be recovered (except the tx fee part).

If channel id belongs to a relayer that is customized by malicious person, then anything can happen depending on what this person changed on his/her relayer.

P.S. the alternative would be to load a file like the coins file and it will be updated with each new GUI release while relying on the user to enter custom channel ids for new IBC channels similar to custom token imports. What do you think @onur-ozkan?

Channels can be changed very frequently, I don't think we can keep up with it by releasing komodo-wallet for each change on the channels.

Either our repo or user's hostname configuration must be hacked (which is already ..... situation) in order to provide incorrect channels. But that's already more dangerous than providing incorrect channels.

Only one dev with write auth getting hacked is enough to write incorrect channels info to this repo and it will be used straight away by komodo wallet and the defi framework release. We can't lock this to a certain commit hash too to have auto update.

Channels can be changed very frequently, I don't think we can keep up with it by releasing komodo-wallet for each change on the channels.

We can leave it as is for now but I wanted to raise this issue for the sec review team to see their opinion.
One more question, do channels change for already running and established chains or is this needed for new chains to be always available in komodo wallet?

Only one dev with write auth getting hacked is enough to write incorrect channels info to this repo and it will be used straight away by komodo wallet and the defi framework release. We can't lock this to a certain commit hash too.

We can but that kills the whole idea. I would just put the json file in the mm2 source code instead of using the locked hash, but as I mentioned above we can not make mm2 release each time a channel info changes in the repo. Giving the write access to someone irrelevant is not an easy accident and that's already pretty bad situation in my opinion.

We can leave it as is for now but I wanted to raise this issue for the sec review team to see their opinion.
One more question, do channels change for already running and established chains or is this needed for new chains to be always available in komodo wallet?

They can be removed/updated for any new/old chain.

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy

🔥

borngraced

Great work and well done!

* dev: feat(tendermint): pubkey-only activation and unsigned tx (KomodoPlatform#2088) fix(tests): set txfee for some tbtc tests (KomodoPlatform#2116)

* dev: feat(tendermint): pubkey-only activation and unsigned tx (KomodoPlatform#2088) fix(tests): set txfee for some tbtc tests (KomodoPlatform#2116) fix(eth): remove my_address from sign_and_send_transaction_with_keypair (KomodoPlatform#2115) fix(utxo-swap): apply events occurred while taker down (KomodoPlatform#2114) refactor(memory): memory usage improvements (KomodoPlatform#2098) feat(app-dir): implement root application dir `.kdf` (KomodoPlatform#2102) fix tendermint fee calculation (KomodoPlatform#2106) update dockerfile (KomodoPlatform#2104)

add activation mode into TendermintCoinImpl type

e6d5b7f

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan added enhancement New feature or request in progress Changes will be made from the author labels Mar 13, 2024

onur-ozkan force-pushed the keplr-and-external-sign-swap branch from f014bcd to c1428fe Compare March 13, 2024 13:16

onur-ozkan added 5 commits March 13, 2024 16:17

tendermint unsigned withdraw dirty P.o.C

c1428fe

Signed-off-by: onur-ozkan <work@onurozkan.dev>

optimize unwindings and left some TODO notes

2685be6

Signed-off-by: onur-ozkan <work@onurozkan.dev>

fix wasm compatibility

10b9449

Signed-off-by: onur-ozkan <work@onurozkan.dev>

return sign_doc

115f29d

Signed-off-by: onur-ozkan <work@onurozkan.dev>

update solana tx results

b88e312

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan changed the title ~~feat(tendermint): pubkey login and unsigned transactions~~ feat(tendermint): pubkey-only activation and unsigned tx Mar 14, 2024

onur-ozkan added 9 commits March 14, 2024 15:39

apply P.o.C patch on tendermint withdraw

40e3598

Signed-off-by: onur-ozkan <work@onurozkan.dev>

deserialize raw pubkey

72e977c

Signed-off-by: onur-ozkan <work@onurozkan.dev>

create TendermintActivationPolicy and use the actual addr

b4f4d6e

Signed-off-by: onur-ozkan <work@onurozkan.dev>

save dev state

3636789

Signed-off-by: onur-ozkan <work@onurozkan.dev>

refactor privkey policy to be activation policy

9c4824d

Signed-off-by: onur-ozkan <work@onurozkan.dev>

refactor how keys are handled for tendermint

e0d0180

Signed-off-by: onur-ozkan <work@onurozkan.dev>

simplify account id derivation for pubkey-only activation

fbccddc

Signed-off-by: onur-ozkan <work@onurozkan.dev>

create TendermintActivationPolicy::generate_account_id

1c01748

Signed-off-by: onur-ozkan <work@onurozkan.dev>

fix missing field error on tendermint activation

d8b20fc

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the keplr-and-external-sign-swap branch from 696181e to f833b52 Compare March 20, 2024 11:24

onur-ozkan added 5 commits March 20, 2024 14:25

write unit test for TendermintActivationPolicy::generate_account_id

f833b52

Signed-off-by: onur-ozkan <work@onurozkan.dev>

handle panics

3ca9365

Signed-off-by: onur-ozkan <work@onurozkan.dev>

handle fee calculations for pubkey-only mode

cb99206

Signed-off-by: onur-ozkan <work@onurozkan.dev>

implement signed/unsigned tendermint withdraws(including ibc)

26df383

Signed-off-by: onur-ozkan <work@onurozkan.dev>

improve gas estimation for pubkey-only activations

fedff61

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy requested review from shamardy and borngraced March 22, 2024 22:03

shamardy reviewed Mar 27, 2024

View reviewed changes

borngraced requested changes Mar 27, 2024

View reviewed changes

Merge branch 'dev' of github.com:KomodoPlatform/komodo-defi-framework…

99707f2

… into keplr-and-external-sign-swap

onur-ozkan force-pushed the keplr-and-external-sign-swap branch from 163bbb7 to 06e2073 Compare April 29, 2024 11:12

only match the tx content

7d87d32

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the keplr-and-external-sign-swap branch from 06e2073 to 7d87d32 Compare April 29, 2024 11:32

properly derive pubkey for tendermint tokens

9802160

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy requested review from shamardy and borngraced May 6, 2024 14:57

WIP: ibc refactoring

f776f38

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan force-pushed the keplr-and-external-sign-swap branch from b431563 to f776f38 Compare May 8, 2024 05:42

onur-ozkan added 7 commits May 8, 2024 12:49

update tendermint validate_address

1467df2

Signed-off-by: onur-ozkan <work@onurozkan.dev>

fix WASM error for IBCs

033101c

Signed-off-by: onur-ozkan <work@onurozkan.dev>

deduplicate IBC channel detection

33ca1da

Signed-off-by: onur-ozkan <work@onurozkan.dev>

Merge branch 'dev' of github.com:KomodoPlatform/komodo-defi-framework…

d1ce39d

… into keplr-and-external-sign-swap

fix sia

46f912f

Signed-off-by: onur-ozkan <work@onurozkan.dev>

fix tests

e84e615

Signed-off-by: onur-ozkan <work@onurozkan.dev>

increase htlc test timeout

7744207

Signed-off-by: onur-ozkan <work@onurozkan.dev>

onur-ozkan added under review and removed in progress Changes will be made from the author labels May 10, 2024

onur-ozkan and others added 2 commits May 10, 2024 10:46

update data_asker test (can't do multi-thread tests on WASM)

c48bde8

Signed-off-by: onur-ozkan <work@onurozkan.dev>

deserialize COIN_BALANCE to EventName::CoinBalance

0b8b0cb

shamardy reviewed May 10, 2024

View reviewed changes

chore: add doc-comment, fix typo

358ad7b

Signed-off-by: onur-ozkan <work@onurozkan.dev>

shamardy approved these changes May 13, 2024

View reviewed changes

borngraced approved these changes May 13, 2024

View reviewed changes

shamardy merged commit 52326c4 into dev May 14, 2024
21 of 25 checks passed

shamardy deleted the keplr-and-external-sign-swap branch May 14, 2024 02:26

harsenyan3 mentioned this pull request Jul 17, 2024

ibc transfers possible in withdraw method KomodoPlatform/komodo-docs-mdx#283

Open

shamardy mentioned this pull request Aug 1, 2024

chore(release): v2.1.0-beta #2043

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(tendermint): pubkey-only activation and unsigned tx #2088

feat(tendermint): pubkey-only activation and unsigned tx #2088

onur-ozkan commented Mar 13, 2024 •

edited

Loading

shamardy left a comment

shamardy Mar 27, 2024

onur-ozkan Apr 8, 2024

borngraced left a comment

borngraced Mar 27, 2024

onur-ozkan Apr 8, 2024 •

edited

Loading

borngraced May 6, 2024

onur-ozkan commented Mar 27, 2024

shamardy left a comment

shamardy May 10, 2024

onur-ozkan May 10, 2024

shamardy May 13, 2024

onur-ozkan May 13, 2024 •

edited

Loading

shamardy May 13, 2024

shamardy May 13, 2024

shamardy May 10, 2024

onur-ozkan May 10, 2024 •

edited

Loading

shamardy May 13, 2024 •

edited

Loading

onur-ozkan May 13, 2024

shamardy left a comment

borngraced left a comment

		#[cfg(target_arch = "wasm32")]
		PrivKeyPolicy::Metamask(_) => unreachable!(),

	#[cfg(target_arch = "wasm32")]
	Metamask(EthMetamaskPolicy),
	}

	#[cfg(target_arch = "wasm32")]
	#[derive(Clone, Debug)]
	pub struct EthMetamaskPolicy {
	pub(crate) public_key: EthH264,
	pub(crate) public_key_uncompressed: EthH520,
	}

feat(tendermint): pubkey-only activation and unsigned tx #2088

feat(tendermint): pubkey-only activation and unsigned tx #2088

Conversation

onur-ozkan commented Mar 13, 2024 • edited Loading

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan commented Mar 27, 2024

shamardy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

onur-ozkan May 10, 2024 • edited Loading

Choose a reason for hiding this comment

shamardy May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shamardy left a comment

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

onur-ozkan commented Mar 13, 2024 •

edited

Loading

onur-ozkan Apr 8, 2024 •

edited

Loading

onur-ozkan May 13, 2024 •

edited

Loading

onur-ozkan May 10, 2024 •

edited

Loading

shamardy May 13, 2024 •

edited

Loading