feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins by dimxy · Pull Request #1930 · GLEECBTC/komodo-defi-framework

dimxy · 2023-08-03T08:18:07Z

Adds new sign_raw_transaction rpc method for UTXO coins.
Currently supports P2PKH and P2WPKH spent outputs.
Closes #1407

TODO:
support more previous output types
support several signing keys
update docs to add rpc desc
signing with trezor

borngraced

Great work! some suggestions from my side

borngraced · 2023-08-09T03:29:48Z

+        hex::decode(args.tx_hex.as_bytes()).map_to_mm(|e| RawTransactionError::InvalidParam(e.to_string()))?;
+    let tx: UtxoTx =
+        deserialize(tx_bytes.as_slice()).map_to_mm(|e| RawTransactionError::InvalidParam(e.to_string()))?;


I think these should be TxByteParseError or DecodeError

borngraced · 2023-08-09T03:30:25Z

+                        .map_to_mm(|e| RawTransactionError::InvalidParam(e.to_string()))?,
+                );
+            }
+
+            let prev_hash = hex::decode(prev_utxo.tx_hash.as_bytes())
+                .map_err(|e| RawTransactionError::InvalidParam(e.to_string()))?;


same as above

borngraced · 2023-08-09T03:31:31Z

+        .filter(|input| !unspents.iter().any(|u| u.outpoint.hash == input.previous_output.hash))
+        .map(|input| input.previous_output.hash.into())


what about using filter_map here once?

borngraced · 2023-08-09T03:33:27Z

+        signature_version,
+        coin.as_ref().conf.fork_id,
+    )
+    .map_err(|err| RawTransactionError::SigningError(err.get_inner().to_string()))?;


why don't we just call err.to_string() so we get the error trace too

borngraced · 2023-08-09T03:34:15Z

        let secret = SecretKey::from_slice(&*self.secret)?;
        let message = SecpMessage::from_slice(&**message)?;
-        let signature = SECP_SIGN.sign(&message, &secret);
+        let signature = SECP_SIGN.sign_low_r(&message, &secret); // use low R signing from bitcoin which reduces signature malleability


// use low R signing from bitcoin which reduces signature malleability let signature = SECP_SIGN.sign_low_r(&message, &secret);

shamardy · 2023-08-10T20:25:46Z

@dimxy please fix clippy and fmt warnings, you should use cargo clippy --all-targets --all-features -- --D warnings to check clippy warnings and cargo +nightly fmt for formatting the code.

shamardy

Thanks a lot for the PR! First review iteration!

dimxy · 2023-08-11T15:05:34Z

@dimxy please fix clippy and fmt warnings, you should use cargo clippy --all-targets --all-features -- --D warnings to check clippy warnings and cargo +nightly fmt for formatting the code.

okay thank you, I used clippy but apparently with different params that I found in the contrib guide

shamardy · 2023-08-11T15:08:51Z

I used clippy but apparently with different params that I found in the contrib guide

Contribution guide needs lots of updates apparently.

dimxy · 2023-08-14T12:35:39Z

thank you all for your reviews, added new commits following them: used async_trait, fixed formatting, clippy errs, refactored sign_raw_tx (advised function added), improved RawTransactionError

shamardy

Thanks a lot for the fixes! Next review iteration!

dimxy · 2023-09-06T08:04:40Z

I rebased on the latest dev. I believe I have made all fixes requested by the reviewers

shamardy

Next review iteration! Will do one final review round once these comments are fixed.

laruh · 2023-09-14T04:53:34Z

@dimxy please note, that this PR started to have conflicts

dimxy · 2023-09-14T07:50:57Z

@dimxy please note, that this PR started to have conflicts

Oh, thank you. I have already fixed several conflicts recently but more of them came

dimxy · 2023-11-28T10:46:05Z

just rebased on the latest dev

smk762 · 2023-11-28T18:45:14Z

just rebased on the latest dev

Thanks! I'm, still a little confused about the ETH/EVM units. Seems the 0x prefix from examples is optional, which is nice. Units appear to be in wei? I created and broadcast a few transactions on polygon, and I'm not sure how the transaction value is being determined. Ideally this would be in ETH rather than gwei or wei (and consistent with other withdraw methods).

            "value": "0x1",
            "gas_limit": "210000"
            https://polygonscan.com/tx/0x6f7e1573b490d1a2ac197b5cf4cb85b98a4a6c5794cbe8d2d53da9717860e70c            
            Value: 1 wei (< $0.000001)
            Max Txn Cost/Fee: 0.273444490862789 MATIC ($0.21)
            Gas Price: 126.437327466 Gwei (0.000000126437327466 MATIC)

            "value": "1",
            "gas_limit": "210000"
            https://polygonscan.com/tx/0x6c266a3cc299ed091e3d5e11d75b10e9921c4a5ef8b21b0173371a3651866e11
            Value: 1 wei
            Transaction Fee: 0.002522350168632 MATIC ($0.00)
            Gas Price: 120.111912792 Gwei (0.000000120111912792 MATIC)

            "value": "1000000000",
            "gas_limit": "21000"
            https://polygonscan.com/tx/0x383b3c343bae089cad35d037d84ac2aed69fca26e1676f2835a62bffea595f53
            Value: 0.000000068719476736 MATIC (< $0.000001)
            Transaction Fee: 0.002931836276166 MATIC ($0.00)
            Gas Price: 139.611251246 Gwei (0.000000139611251246 MATIC)
          
            "value": "1000000000000000",
            "gas_limit": "21000"
            https://polygonscan.com/tx/0xe52cf506aa0f7932158388fad80af058cb242a50494896c8ab8baba7dd453852
            Value: 1.152921504606846976 MATIC ($0.87)
            Transaction Fee: 0.003759189279783 MATIC ($0.00)
            Gas Price: 179.009013323 Gwei (0.000000179009013323 MATIC)

            "value": "0x1000000000000000",
            "gas_limit": "21000"
            https://polygonscan.com/tx/0x137175ea2ccf4d4819669ae59a53ecc782ebc1dcfa0b05763d5016c607ca2ee5
            Value: 1.152921504606846976 MATIC ($0.87)
            Transaction Fee: 0.002742404163051 MATIC ($0.00)
            Gas Price: 130.590674431 Gwei (0.000000130590674431 MATIC)

smk762 · 2023-11-28T21:03:30Z

Docs draft at https://edbbd891.komodo-docs.pages.dev/en/docs/atomicdex/api/v20/sign_raw_transaction/
Everything outside of ETH units confusion working as expected

dimxy · 2023-11-29T07:49:23Z

Units appear to be in wei?

Yes it is in wei and to be directly converted into U256 'value' (as it was supposed to be the 'raw transaction').
let's make it 'Eth' for consistency with other rpcs

…saction for ETH type

smk762

LGTM, thanks!

shamardy

Latest changes approved! Will merge this after 2.0.0-beta is released.

Alrighttt · 2023-12-20T16:48:50Z

 }

 impl From<&'static str> for Address {
-    fn from(s: &'static str) -> Self { s.parse().unwrap() }


Ideally this should be conditionally compiled for tests only.

Yes, here is a PR for refactoring this struct and a discussion on this unwrap in it: #1960 (comment)

* dev: (22 commits) chore(config): remove vscode launchjson (GLEECBTC#2040) feat(trading-proto-upgrade): wasm DB, kickstart, refund states, v2 RPCs (GLEECBTC#2015) feat(UTXO): balance event streaming for Electrum clients (GLEECBTC#2013) feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins (GLEECBTC#1930) fix(p2p): handle encode_and_sign errors (GLEECBTC#2038) chore(release): add changelog entries for v2.0.0-beta (GLEECBTC#2037) chore(network): write network information to stdout (GLEECBTC#2034) fix(price_endpoints): add cached url (GLEECBTC#2032) deps(network): sync with upstream yamux (GLEECBTC#2030) fix(config): accept a string as rpcport value (GLEECBTC#2026) feat(nft): move db lock, add tx fee and confirmations (GLEECBTC#1989) chore(network): update seednodes for netid 8762 (GLEECBTC#2024) chore(network): add todo on peer storage behaviour (GLEECBTC#2025) chore(network): exclude `168.119.236.249` from the seednode list (GLEECBTC#2021) feat(network): deprecate 7777 network (GLEECBTC#2020) chore(release): bump mm2 version to 2.0.0-beta (GLEECBTC#2018) feat(UTXO swaps): kmd burn plan impl (GLEECBTC#2006) chore(docs): fix the link to simple market maker in README.md (GLEECBTC#2011) refactor(cli): cli dependency updates and warn on bad config perm (GLEECBTC#1956) chore(containers and docs): update docs and container images (GLEECBTC#2003) ... # Conflicts: # mm2src/mm2_main/tests/mm2_tests/mm2_tests_inner.rs # mm2src/mm2_test_helpers/src/for_tests.rs

* dev: (24 commits) chore(release): bump mm2 version to 2.1.0-beta (GLEECBTC#2044) feat(trezor): add segwit support for withdraw with trezor (GLEECBTC#1984) chore(config): remove vscode launchjson (GLEECBTC#2040) feat(trading-proto-upgrade): wasm DB, kickstart, refund states, v2 RPCs (GLEECBTC#2015) feat(UTXO): balance event streaming for Electrum clients (GLEECBTC#2013) feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins (GLEECBTC#1930) fix(p2p): handle encode_and_sign errors (GLEECBTC#2038) chore(release): add changelog entries for v2.0.0-beta (GLEECBTC#2037) chore(network): write network information to stdout (GLEECBTC#2034) fix(price_endpoints): add cached url (GLEECBTC#2032) deps(network): sync with upstream yamux (GLEECBTC#2030) fix(config): accept a string as rpcport value (GLEECBTC#2026) feat(nft): move db lock, add tx fee and confirmations (GLEECBTC#1989) chore(network): update seednodes for netid 8762 (GLEECBTC#2024) chore(network): add todo on peer storage behaviour (GLEECBTC#2025) chore(network): exclude `168.119.236.249` from the seednode list (GLEECBTC#2021) feat(network): deprecate 7777 network (GLEECBTC#2020) chore(release): bump mm2 version to 2.0.0-beta (GLEECBTC#2018) feat(UTXO swaps): kmd burn plan impl (GLEECBTC#2006) chore(docs): fix the link to simple market maker in README.md (GLEECBTC#2011) ...

* evm-hd-wallet: (27 commits) Fix todo comments Fix HDAddressOps::Address trait bounds fix(indexeddb): fix IDB cursor.continue_() call after drop (GLEECBTC#2028) security bump for `h2` (GLEECBTC#2062) fix(makerbot): allow more than one prices url in makerbot (GLEECBTC#2027) fix(wasm worker env): refactor direct usage of `window` (GLEECBTC#1953) feat(nft): nft abi in withdraw_nft RPC, clear_nft_db RPC (GLEECBTC#2039) refactor(utxo): refactor utxo output script creation (GLEECBTC#1960) feat(ETH): balance event streaming for ETH (GLEECBTC#2041) chore(release): bump mm2 version to 2.1.0-beta (GLEECBTC#2044) feat(trezor): add segwit support for withdraw with trezor (GLEECBTC#1984) chore(config): remove vscode launchjson (GLEECBTC#2040) feat(trading-proto-upgrade): wasm DB, kickstart, refund states, v2 RPCs (GLEECBTC#2015) feat(UTXO): balance event streaming for Electrum clients (GLEECBTC#2013) feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins (GLEECBTC#1930) fix(p2p): handle encode_and_sign errors (GLEECBTC#2038) chore(release): add changelog entries for v2.0.0-beta (GLEECBTC#2037) chore(network): write network information to stdout (GLEECBTC#2034) fix(price_endpoints): add cached url (GLEECBTC#2032) deps(network): sync with upstream yamux (GLEECBTC#2030) ...

dimxy assigned shamardy Aug 3, 2023

dimxy changed the title ~~[R2R] feat(rpc): add new sign_raw_transaction rpc for UTXO coins~~ feat(rpc): add new sign_raw_transaction rpc for UTXO coins Aug 3, 2023

dimxy added the under review label Aug 3, 2023

shamardy requested review from borngraced, rozhkovdmitrii and shamardy August 3, 2023 22:55

shamardy added the 1.0.7-beta label Aug 3, 2023

borngraced suggested changes Aug 9, 2023

View reviewed changes

borngraced reviewed Aug 9, 2023

View reviewed changes

Comment thread mm2src/coins/tendermint/tendermint_token.rs

shamardy requested changes Aug 10, 2023

View reviewed changes

Comment thread mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs Outdated

Comment thread mm2src/coins/lp_coins.rs Outdated

Comment thread mm2src/coins/lp_coins.rs Outdated

Comment thread mm2src/coins/lp_coins.rs Outdated

Comment thread mm2src/coins/utxo/utxo_common.rs

dimxy assigned dimxy and unassigned shamardy Aug 16, 2023

shamardy requested changes Aug 18, 2023

View reviewed changes

shamardy added in progress and removed under review labels Aug 23, 2023

shamardy added 1.2.0-beta and removed 1.0.7-beta labels Sep 4, 2023

dimxy force-pushed the feature-sign-raw-tx branch from 5d422ab to 7f01291 Compare September 6, 2023 07:27

dimxy added under review and removed in progress labels Sep 8, 2023

shamardy changed the title ~~feat(rpc): add new sign_raw_transaction rpc for UTXO coins~~ feat(rpc): add new sign_raw_transaction rpc for UTXO and EVM coins Sep 13, 2023

shamardy reviewed Sep 13, 2023

View reviewed changes

Comment thread mm2src/coins/lp_coins.rs

Comment thread mm2src/coins/eth.rs Outdated

Comment thread mm2src/coins/eth.rs Outdated

Comment thread mm2src/coins/lp_coins.rs

Comment thread mm2src/coins/utxo/utxo_common.rs

Comment thread mm2src/coins/utxo/utxo_common.rs

fix after rebase

a699167

dimxy dismissed stale reviews from ca333 and shamardy via a699167 November 28, 2023 09:51

dimxy force-pushed the feature-sign-raw-tx branch from f5afcf8 to a699167 Compare November 28, 2023 09:51

change value units to eth, fix parse eth data as hex in sign_raw_tran…

eb0dfee

…saction for ETH type

dimxy force-pushed the feature-sign-raw-tx branch from 9cb21d2 to eb0dfee Compare November 29, 2023 12:07

smk762 previously approved these changes Dec 2, 2023

View reviewed changes

shamardy added 2.1.0-beta and removed 2.0.0-beta labels Dec 6, 2023

shamardy previously approved these changes Dec 12, 2023

View reviewed changes

fix value in wasm_test_sign_eth_tx

078385f

dimxy dismissed stale reviews from shamardy and smk762 via 078385f December 19, 2023 19:32

shamardy approved these changes Dec 19, 2023

View reviewed changes

shamardy removed QA labels Dec 19, 2023

shamardy changed the title ~~feat(rpc): add new sign_raw_transaction rpc for UTXO and EVM coins~~ feat(tx): add new sign_raw_transaction rpc for UTXO and EVM coins Dec 19, 2023

shamardy merged commit 7382588 into dev Dec 19, 2023

shamardy deleted the feature-sign-raw-tx branch December 19, 2023 20:02

Alrighttt reviewed Dec 20, 2023

View reviewed changes

Alrighttt self-assigned this Dec 20, 2023

harsenyan3 mentioned this pull request Jul 16, 2024

Edit sign_raw_transaction RPC method documentation GLEECBTC/komodo-docs-mdx#276

Closed

shamardy mentioned this pull request Aug 1, 2024

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

Merged

		.filter(\|input\| !unspents.iter().any(\|u\| u.outpoint.hash == input.previous_output.hash))
		.map(\|input\| input.previous_output.hash.into())

Conversation

dimxy commented Aug 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borngraced left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

borngraced Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borngraced Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

borngraced Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

borngraced Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

borngraced Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shamardy commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimxy commented Aug 11, 2023

Uh oh!

shamardy commented Aug 11, 2023

Uh oh!

dimxy commented Aug 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dimxy commented Sep 6, 2023

Uh oh!

shamardy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

laruh commented Sep 14, 2023

Uh oh!

dimxy commented Sep 14, 2023

Uh oh!

dimxy commented Nov 28, 2023

Uh oh!

smk762 commented Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smk762 commented Nov 28, 2023

Uh oh!

dimxy commented Nov 29, 2023

Uh oh!

smk762 left a comment

Choose a reason for hiding this comment

Uh oh!

dimxy commented Aug 3, 2023 •

edited

Loading

borngraced left a comment •

edited

Loading

borngraced Aug 9, 2023 •

edited

Loading

shamardy commented Aug 10, 2023 •

edited

Loading

dimxy commented Aug 14, 2023 •

edited

Loading

smk762 commented Nov 28, 2023 •

edited

Loading

shamardy left a comment •

edited

Loading