[master] v1 metamask connect to isolated server + testnet#2820
[master] v1 metamask connect to isolated server + testnet#2820
Conversation
github-actions
bot
changed the title
src/libServer/LookupServer.cpp
Outdated
| } | ||
| } | ||
|
|
||
| Json::Value LookupServer::CreateTransactionEth(EthFields const& fields, bytes const& pubKey, const unsigned int num_shards, |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
left a comment
Steve-White-UK
left a comment
There was a problem hiding this comment.
Fairly understandble, just a couple of comments will seek clarification tommorw.
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
bzawisto
left a comment
bzawisto
left a comment
There was a problem hiding this comment.
Well done Nathan! There are a couple of nitpick comments so if you don't want to address them that's still fine!
| } | ||
| } | ||
|
|
||
| Json::Value IsolatedServer::CreateTransactionEth(EthFields const& fields, |
There was a problem hiding this comment.
Presumably challenging, but do you think it's feasible to split this function into smaller entities (now it's > 150 loc).
There was a problem hiding this comment.
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?
src/libServer/IsolatedServer.h
Outdated
|
|
||
| inline virtual void GetEthSendRawTransactionI(const Json::Value& request, | ||
| Json::Value& response) { | ||
| (void)request; |
There was a problem hiding this comment.
If we don't use request, should we enclose it with / * */ in the arg list?
There was a problem hiding this comment.
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?
src/libServer/IsolatedServer.h
Outdated
| rawTx.erase(0, 2); | ||
| } | ||
|
|
||
| auto pubKey = RecoverECDSAPubSig(rawTx, ETH_CHAINID_INT); |
There was a problem hiding this comment.
Nitpick: immutable vars could be marked as const.
| } | ||
|
|
||
| // Given a RLP message, parse out the fields and return a EthFields object | ||
| EthFields parseRawTxFields(std::string const& message) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
sounds great - done
| // EIP-155 : assume the chain height is high enough that the signing scheme | ||
| // is in line with EIP-155. | ||
| // message shall not contain '0x' | ||
| bytes RecoverECDSAPubSig(std::string const& message, int chain_id) { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
sounds great - done
4 participants
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 (!)