[master] v1 metamask connect to isolated server + testnet #2820

n-hutton · 2022-06-24T13:33:41Z

MVP getting metamask to connect to the testnet and move funds about.

TL;DR: by using the old zil api you can transfer to a metamask account. You can then transfer between these accounts. The mechanism for tracking mined TX is not yet in place so metamask thinks they are still pending:

This required adding a library for ECDSA public key recovery (that is, given a message and a signature, derive the public key), which was forked from bitcoin-core. I do not know how esteemed/well tested this library is. It was used by Aleth.

Details:

The main difficulty is getting the details of the raw transaction submission correct - it is detailed here: https://eips.ethereum.org/EIPS/eip-155

The current solution I think only works if metamask thinks your chain height is over 2,675,000 - otherwise, it would use a different signing scheme. The solution I go with might be the code just checks for both.

In order to add a raw eth transaction, you do the following:
Parse all the fields from the transaction using ethereum serialization format 'RLP'
Replace the signature with 0 and hash the transaction to determine the hash that was created client side
Put it back into RLP format and hash it
Use this hash to recreate the public key

There are some dummy values returned for the queries that metamask likes to see that need to be filled in - getting blocks by hash etc.

Note: get balance for eth returns a hex value, whereas zil returns a decimal string. So there is potential for massively incorrect balances to be shown if it is not used correctly.

Adds the following API calls:

eth_blockNumber
net_version
eth_getBalance
eth_getBlockByNumber
eth_gasPrice
eth_getCode
eth_estimateGas
eth_getTransactionCount
eth_sendRawTransaction
eth_getTransactionReceipt - needs to be added to lookup server (!)

constants.xml

Steve-White-UK · 2022-07-21T13:14:34Z

src/libServer/LookupServer.cpp

@@ -792,6 +847,187 @@ Json::Value LookupServer::CreateTransaction(
  }
 }

+Json::Value LookupServer::CreateTransactionEth(EthFields const& fields, bytes const& pubKey, const unsigned int num_shards,


Is it neccassary to have this code in both isolated server and lookup server, could it not be common to both? just asking :-) or is this just temporary ?

the code that can be common is common - so the isolated server calls the lookup server's implementation when it is simple (like getting the chain id)

Steve-White-UK

Fairly understandble, just a couple of comments will seek clarification tommorw.

codecov-commenter · 2022-07-23T10:34:51Z

Codecov Report

Merging #2820 (109df60) into master (2316603) will increase coverage by 0.55%.
The diff coverage is 60.27%.

@@            Coverage Diff             @@
##           master    #2820      +/-   ##
==========================================
+ Coverage   32.76%   33.32%   +0.55%     
==========================================
  Files         274      278       +4     
  Lines       42694    43231     +537     
==========================================
+ Hits        13987    14405     +418     
- Misses      28707    28826     +119

Impacted Files	Coverage Δ
src/common/Constants.h	`100.00% <ø> (ø)`
src/libData/AccountData/Transaction.cpp	`75.86% <0.00%> (-2.71%)`	⬇️
src/libData/AccountData/Transaction.h	`100.00% <ø> (ø)`
src/libNode/DSBlockProcessing.cpp	`0.00% <ø> (ø)`
src/libValidator/Validator.cpp	`2.76% <0.00%> (+<0.01%)`	⬆️
src/libServer/LookupServer.cpp	`18.26% <21.16%> (+2.19%)`	⬆️
src/libCrypto/EthCrypto.cpp	`28.46% <50.00%> (+28.46%)`	⬆️
src/libUtils/DataConversion.cpp	`44.11% <61.11%> (-8.02%)`	⬇️
src/libServer/LookupServer.h	`32.71% <63.15%> (+9.32%)`	⬆️
src/libEth/Eth.cpp	`76.92% <76.92%> (ø)`
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2316603...109df60. Read the comment docs.

bzawisto

Well done Nathan! There are a couple of nitpick comments so if you don't want to address them that's still fine!

src/libCrypto/EthCrypto.cpp

src/libData/AccountData/Transaction.cpp

bzawisto · 2022-07-27T19:07:12Z

src/libServer/IsolatedServer.cpp

@@ -491,6 +549,166 @@ Json::Value IsolatedServer::CreateTransaction(const Json::Value& _json) {
  }
 }

+Json::Value IsolatedServer::CreateTransactionEth(EthFields const& fields,


Presumably challenging, but do you think it's feasible to split this function into smaller entities (now it's > 150 loc).

yes, I can do this - I was basically copying the code from the scilla way of adding TX so can refactor both in a seperate PR if you like?

bzawisto · 2022-07-27T19:07:51Z

src/libServer/IsolatedServer.h

@@ -49,6 +50,24 @@ class IsolatedServer : public LookupServer,
    response = this->CreateTransaction(request[0u]);
  }

+  inline virtual void GetEthSendRawTransactionI(const Json::Value& request,
+                                                Json::Value& response) {
+    (void)request;


If we don't use request, should we enclose it with / * */ in the arg list?

Oh. Request is used. The (void) was a copy/paste error in this instance. I don't mind refactoring to use /* */ instead of a void cast in a future PR if you like?

bzawisto · 2022-07-27T19:08:17Z

src/libServer/IsolatedServer.h

+      rawTx.erase(0, 2);
+    }
+
+    auto pubKey = RecoverECDSAPubSig(rawTx, ETH_CHAINID_INT);


Nitpick: immutable vars could be marked as const.

bzawisto · 2022-07-27T19:12:19Z

src/libEth/Eth.cpp

+}
+
+// Given a RLP message, parse out the fields and return a EthFields object
+EthFields parseRawTxFields(std::string const& message) {


It's a bit of a subtle piece of code, can we write some unit test just to have a mean of checking if it still works in case someone wants to change it one day?

sounds great - done

bzawisto · 2022-07-27T19:13:14Z

src/libCrypto/EthCrypto.cpp

+// is in line with EIP-155.
+// message shall not contain '0x'
+bytes RecoverECDSAPubSig(std::string const& message, int chain_id) {


It'd be nice to have a unit test for some predefined input and known output (just to ensure it works in case someone needs to change it).

sounds great - done

v1 metamask connect - try testnet, too

00e5606

github-actions bot changed the title ~~v1 metamask connect - try testnet, too~~ [master] v1 metamask connect - try testnet, too Jun 24, 2022

n-hutton and others added 27 commits June 24, 2022 15:24

fixup

26f2440

bump travis/jenkins

676d6a9

tidying

ef23f54

add debug for testnet

31cf230

add killer rpc call for debug purpose

86ec18e

hacks

cd3b4af

Merge branch 'master' into feature/evm_rpc

b8639f8

fix unfortunate error

2d2faf0

WIP

3052f3b

testnet

afa4c22

fixme

878768d

about to switch TX way

f6dc7d3

fix versioning

3d23a94

fixes

05c9349

fixes

7f19390

bump and turn off travis

b4bc76c

try more stuff

56b53af

fix balance

f75267b

fix balance querying

abbf7bd

WIP

e7af88a

thing thing

989125c

WORKING FIRST VERSION!

46af9a6

seems to be working... try for another testnet deployment

7ca3ecb

another testnet deployment

18ca008

bump

f78ebec

add missing cmake file

f09aaa8

add other file

ecac377

n-hutton added 9 commits July 20, 2022 00:14

fix build...

e2d02df

sigh

6d062d7

format fix

a080c5c

include linting

9b25bac

fix build error...

cb97c00

clang tidy fix

525bddd

sigh

1d9c7f2

fix test

7601a21

fix another test

2904777

n-hutton marked this pull request as ready for review July 21, 2022 11:03

Steve-White-UK reviewed Jul 21, 2022

View reviewed changes

constants.xml Outdated Show resolved Hide resolved

Steve-White-UK reviewed Jul 21, 2022

View reviewed changes

constants.xml Outdated Show resolved Hide resolved

Steve-White-UK reviewed Jul 21, 2022

View reviewed changes

constants.xml Outdated Show resolved Hide resolved

Steve-White-UK reviewed Jul 21, 2022

View reviewed changes

n-hutton added 2 commits July 22, 2022 23:36

merge to master

21a5a34

fix build

1139ff3

n-hutton requested review from bzawisto and art-gor July 26, 2022 13:40

bzawisto approved these changes Jul 27, 2022

View reviewed changes

n-hutton and others added 5 commits July 28, 2022 08:59

Merge branch 'master' into feature/evm_rpc

b019d75

pr feedback except for unit tests

152d26e

add unit tests

af1b70d

style...

dd8a036

more style

109df60

n-hutton merged commit 50441f1 into master Jul 29, 2022

n-hutton deleted the feature/evm_rpc branch July 29, 2022 11:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[master] v1 metamask connect to isolated server + testnet #2820

[master] v1 metamask connect to isolated server + testnet #2820

n-hutton commented Jun 24, 2022 •

edited

Loading

Steve-White-UK Jul 21, 2022

n-hutton Jul 23, 2022

Steve-White-UK left a comment

codecov-commenter commented Jul 23, 2022 •

edited

Loading

bzawisto left a comment

bzawisto Jul 27, 2022

n-hutton Jul 28, 2022

bzawisto Jul 27, 2022

n-hutton Jul 28, 2022

bzawisto Jul 27, 2022

n-hutton Jul 28, 2022

bzawisto Jul 27, 2022

n-hutton Jul 28, 2022

bzawisto Jul 27, 2022

n-hutton Jul 28, 2022

[master] v1 metamask connect to isolated server + testnet #2820

[master] v1 metamask connect to isolated server + testnet #2820

Conversation

n-hutton commented Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Steve-White-UK left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 23, 2022 • edited Loading

Codecov Report

bzawisto 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n-hutton commented Jun 24, 2022 •

edited

Loading

codecov-commenter commented Jul 23, 2022 •

edited

Loading