Eth blockchain transaction #1028

T-Dnzt · 2019-05-28T09:51:54Z

Issue/Task Number: 699
Closes #699

Overview

This PR adds 2 new blockchain actions to allow sending Eth or Tokens from a hot wallet.

Changes

Add EthBlockchain.Transaction.send(%{from: "0xsender", to: "0xreceiver", amount: amount})
Add EthBlockchain.Transaction.send(%{from:" 0xsender", to: "0xreceiver", amount: amount, contract_address: "0xcontract_address"})
Fixed wallet generation and moved it to the Keychain app.

Implementation Details

We use the send_raw_transaction method from geth to submit an offline signed and encoded transaction to the network.
The transaction is generated in the EthBlockchain app, then signed in the Keychain app, so the private key doesn't leave the Keychain app.
A few settings have been added to set default value to some parameters:

default_gas_price: 20_000_000_000,
default_eth_transaction_gas_limit: 21_000,
default_contract_transaction_gas_limit: 90_000,
chain_id: 1

This settings are arbitrarily set and may/should/will need to be updated in the future.
There is a limitation to this implementation where you need to wait until a transaction gets included in a block before making a new one from the same account. (due to the nonce that needs to be incremented and equal to the current number of mined transaction for the account).

Usage

There is a new folder at the root of the project named local_geth_testnet that contains a README file to setup a local geth testnet with some initial data.

Follow this guide then you will be able to test the following by replacing the different addresses with the one obtained:

Send ETH:

EthBlockchain.Transaction.send(%{from: "0xprimary_address", to: "0xrecipient_address", amount: 123})

Send OMG:

EthBlockchain.Transaction.send({from: "0xprimary_address", to: "0xrecipient_address", amount: 123, contract_address: "0xcontract_address"})

You can also check the balance of an address with:

EthBlockchain.Balance.get(%{address: "0xprimary_address", contract_addresses: ["0x0000000000000000000000000000000000000000", "0xcontract_address"]})

which will output the balance of ETH and OMG for the specified address.

.gitignore

apps/eth_blockchain/lib/eth_blockchain/abi.ex

apps/eth_blockchain/lib/eth_blockchain/transaction.ex

apps/keychain/lib/keychain/signature.ex

unnawut

Tried running this on my local machine. Works like magic!

iex> EthBlockchain.Transaction.send({"0x33dbe8d0cddd178bc2aad4b194855d2b912dbfe1", "0x3230f3a6bc7c5fbec52eb533e762722bb339a140", 123, "0x90fa40efd96489b98a73d15aa226c2d64099a3d3"})
[debug] QUERY OK source="keychain" db=3.2ms queue=0.1ms
SELECT k0."encrypted_private_key" FROM "keychain" AS k0 WHERE (k0."wallet_id" = $1) ["0x33dbe8d0cddd178bc2aad4b194855d2b912dbfe1"]
{:ok, "0x5e22da6ead930b248b46034020f0b1ac9754527f8571a2e43479f970ece3ca4a"}

> omgtoken.balanceOf(eth.accounts[0])
99999999999999999877

> omgtoken.balanceOf(eth.accounts[1])
123

tools/local_geth_testnet/console.sh

sirn

Mostly nitpicking, otherwise looks ok to me.

Also, do we want to reject 0x000...000 from being inserted? (Or was this already a thing? I can’t remember)

sirn · 2019-06-19T11:31:42Z

tools/local_geth_testnet/genesis.json

+  "parentHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
+  "gasLimit": "0xffffffff",
+  "config": {
+    "chainId": 1,


Should change chainId to something else other than 1. This ID collides with mainnet. We used to use 90325 for our internal ETH testnet a while ago (it reads 0 = O, 3 = Mi, 2 = Se(cond), 5 = Go).

sirn · 2019-06-19T11:32:41Z

tools/local_geth_testnet/console.sh

+fi
+
+echo "Starting geth console..."
+geth --identity "local-geth" --rpc  --rpccorsdomain "*" --datadir "${DATA_DIR}" --nodiscover --rpcapi "db,eth,net,web3" --nat "any" console


Might be better to run this with --dev if we’re running private local chain.

sirn · 2019-06-19T11:49:07Z

apps/eth_blockchain/lib/eth_blockchain/abi_encoder.ex

+
+  import Utils.Helpers.Encoding
+
+  def balance_of(address) when byte_size(address) == 42 do


Should we use something like balance_of(0x <> _ = address) instead?

sirn · 2019-06-19T11:52:51Z

apps/eth_blockchain/lib/eth_blockchain/adapter.ex

@@ -77,6 +77,14 @@ defmodule EthBlockchain.Adapter do
    GenServer.stop(pid)
  end

+  @spec adapter_or_default(atom() | nil) :: atom()
+  def adapter_or_default(adapter \\ nil) do


This function seems specific to adapter and adapter_or_default/0 doesn’t seems to get used, should you change this to call(nil, func_spec, pid) that did resolve adapter from default adapter instead?

sirn · 2019-06-19T11:58:08Z

apps/eth_blockchain/lib/eth_blockchain/transaction.ex

+    gas_price = Application.get_env(:eth_blockchain, :default_gas_price)
+
+    send(
+      {from_address, to_address, amount, "0x0000000000000000000000000000000000000000", gas_price},


I think it might help readability if we put this to @eth_address 😅

sirn · 2019-06-19T12:04:40Z

apps/eth_blockchain/lib/eth_blockchain/transaction.ex

+  def send({from_address, to_address, amount, contract_address}, adapter, pid)
+      when is_binary(contract_address) do
+    gas_price = Application.get_env(:eth_blockchain, :default_gas_price)
+    send({from_address, to_address, amount, contract_address, gas_price}, adapter, pid)


Might be able to simplify default gas price retrieving with (for both eth/contract):

def send({_, _, _, _} = t, adapter, pid) do gas_price = Application.get_env(:eth_blockchain, :default_gas_price) t2 = Tuple.append(t, gas_price) send(t2, adapter, pid) end

Alternatively, it might be easier if we use map instead of tuple.

sirn · 2019-06-19T12:10:47Z

apps/eth_blockchain/lib/eth_blockchain/transaction.ex

+      s
+    ] = rlp
+
+    {init, data} = if to == <<>>, do: {init_or_data, <<>>}, else: {<<>>, init_or_data}


🚨👮 THIS IS if POLICE 👮‍♀ 🚔
YOU MUST SHOW YOUR ERLANG LICENSE

Please use case 😢

{init, data} = case to do <<>> -> {init_or_data, <<>>} _ -> {<<>>, init_or_data} end

unnawut · 2019-06-24T07:44:51Z

apps/eth_blockchain/config/config.exs

@@ -11,7 +11,7 @@ config :eth_blockchain,
  default_gas_price: 20_000_000_000,
  default_eth_transaction_gas_limit: 21_000,
  default_contract_transaction_gas_limit: 90_000,
-  chain_id: 90325
+  chain_id: 90_325


Can you also add a code comment noting where 90_325 number came from?

unnawut · 2019-06-24T07:49:14Z

apps/eth_blockchain/lib/eth_blockchain/balance.ex

@@ -55,4 +65,6 @@ defmodule EthBlockchain.Balance do
        error
    end
  end
+
+  defp do_get(_, _, _), do: {:error, :invalid_parameters}


Invalid parameter checking should be done closer to where the data enters the system, i.e. controller and gates. Data should be safe by the time it reaches this module and throws an exception about unmatched function.

cc: @sirn for a second opinion?

unnawut · 2019-06-24T08:21:30Z

apps/admin_api/test/admin_api/v1/controllers/blockchain_balance_controller_test.exs

      assert Enum.member?(balances, {"0x0000000000000000000000000000000000000002", 123})
    end

    test_with_auths "returns an error when given a non-existing address" do
      response =
        request("/blockchain_wallet.get_balances", %{
-          "address" => "0x00000000000000000000000000000000000000000"
+          "address" => EthBlockchain.eth_address()


This attribute is for a non-existing wallet address, so I think it's better to be a random address than EthBlockchain.eth_address()

mederic-p force-pushed the 699-blockchain-transaction branch from b7308ee to b94edd5 Compare June 6, 2019 01:52

unnawut added this to the v2.0 milestone Jun 11, 2019

unnawut assigned T-Dnzt Jun 11, 2019

mederic-p requested review from unnawut and ripzery June 14, 2019 02:16

mederic-p added the flag/ethereum integration label Jun 14, 2019

mederic-p changed the title ~~[WIP] Blockchain transaction~~ Eth blockchain transaction Jun 14, 2019

mederic-p self-assigned this Jun 14, 2019

unnawut reviewed Jun 14, 2019

View reviewed changes

mederic-p changed the title ~~Eth blockchain transaction~~ [WIP] Eth blockchain transaction Jun 17, 2019

mederic-p force-pushed the 699-blockchain-transaction branch from 399a529 to 5d50b03 Compare June 17, 2019 08:39

mederic-p changed the title ~~[WIP] Eth blockchain transaction~~ Eth blockchain transaction Jun 17, 2019

mederic-p requested a review from unnawut June 17, 2019 09:55

unnawut reviewed Jun 17, 2019

View reviewed changes

tools/local_geth_testnet/console.sh Outdated Show resolved Hide resolved

unnawut added this to Review in eWallet via automation Jun 18, 2019

unnawut approved these changes Jun 18, 2019

View reviewed changes

ripzery approved these changes Jun 19, 2019

View reviewed changes

T-Dnzt requested a review from sirn June 19, 2019 07:29

sirn reviewed Jun 19, 2019

View reviewed changes

mederic-p requested a review from unnawut June 24, 2019 03:32

unnawut reviewed Jun 24, 2019

View reviewed changes

mederic added 8 commits June 24, 2019 16:36

Move wallet generation to keychain app

ac82978

tmp

d99cf04

Add chain_id back

6081e16

Fix wallet generation

ff85288

Fix send transaction

3a2a3c4

Fix ABI encoding

d9e2ae1

credo & format

18200f5

Fix token transfer

8b39285

mederic added 23 commits June 24, 2019 16:36

Move some var to config

c36f4c2

Add missing utils dep

c8ebf85

Fix existing tests

ef7cd69

tamrof xim

af3d0fe

Handle error and add tests

84add64

Add tests

a7eb000

Add scripts and README to facilitate local testnet setup

bb2cd93

Fix typo

9438c50

Increase gas limit to prevent failed transaction

b29f781

Small refactor

c0a793c

Add byzantium block to genesis

dae94c3

Rename ABI to ABIEncoder

109f4c1

Moved local_geth_testnet to tools

44154f5

Refactor transaction send and tests

b0ab817

Fix wrong module name

0275d12

Add doc / spec where missing

df774b6

Merge init.sh and console.sh

6c2515f

Change chain_id to 90325

b106673

Update guard close

6ae022e

Refactor

25e0594

Refactor params to maps instead of tuples

f149224

Remove unneeded checks

4253fb4

Use different address in test

c61c682

mederic-p force-pushed the 699-blockchain-transaction branch from e355f57 to c61c682 Compare June 24, 2019 09:38

mederic-p approved these changes Jun 24, 2019

View reviewed changes

unnawut approved these changes Jun 24, 2019

View reviewed changes

mederic-p merged commit 5e99fe6 into master Jun 24, 2019

eWallet automation moved this from Review to Done Jun 24, 2019

mederic-p deleted the 699-blockchain-transaction branch June 24, 2019 10:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Eth blockchain transaction #1028

Eth blockchain transaction #1028

T-Dnzt commented May 28, 2019 •

edited by mederic-p

Loading

unnawut left a comment

sirn left a comment

sirn Jun 19, 2019

sirn Jun 19, 2019

sirn Jun 19, 2019

sirn Jun 19, 2019

sirn Jun 19, 2019

sirn Jun 19, 2019

sirn Jun 19, 2019

sirn Jun 19, 2019

unnawut Jun 24, 2019

unnawut Jun 24, 2019

unnawut Jun 24, 2019


		import Utils.Helpers.Encoding

		def balance_of(address) when byte_size(address) == 42 do

Eth blockchain transaction #1028

Eth blockchain transaction #1028

Conversation

T-Dnzt commented May 28, 2019 • edited by mederic-p Loading

Overview

Changes

Implementation Details

Usage

unnawut left a comment

Choose a reason for hiding this comment

sirn 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

Choose a reason for hiding this comment

T-Dnzt commented May 28, 2019 •

edited by mederic-p

Loading