-
Notifications
You must be signed in to change notification settings - Fork 74
Conversation
Can you mention a bit what's needed to make it production ready? |
It's mostly because I wrote some of the smart contract myself and they obviously need to be audited (even if it's quite straightforward and nothing complicated). Also it's missing quite a few steps/check to make it fully usable, like monitoring blocks to check for contract validity. For now a contract will be created in a There are a couple of Plus, this will need to be heavily tested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty straightforward. Nice internal API design. 💯
I'm submitting my elixir-related comments first. For the solidity ones, I'll try compile them and see if we have the same result (I hope they are reproducible builds).
apps/ewallet_db/priv/repo/migrations/20190715072907_add_blockchain_fields_1_to_token.exs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! 🕺
@@ -11,6 +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, | |||
default_contract_creation_gas_limit: 1_500_000, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably move all those values in the confg system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I think this can be done in a new PR with the blockchain config system.
|> Base.encode16(case: :lower) | ||
|
||
contract_binary = | ||
:eth_blockchain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could prob extract this to another function, or even another module.
) do | ||
constructor_attributes = | ||
[{name, symbol, decimals, initial_amount}] | ||
|> TypeEncoder.encode(%FunctionSelector{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't another module, like ABI, take care of the encoding?
Returns {:ok, tx_hash, contract_address} if success, | ||
{:error, code} || {:error, code, message} otherwise | ||
""" | ||
def create_contract(%{from: from, contract_data: init} = attrs, adapter, pid) do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is just sending a transaction to create it, but would it make more sense in a Contract
module?
expected_contract_uuid = "3681491a-e8d0-4219-a40a-53d9a47fe64a" | ||
|
||
expected_contract_binary = | ||
:eth_blockchain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is extract, as suggested above, the new function could be used here instead of duplicating the code.
|
||
expected_contract_attributes = | ||
[{"OMGToken", "OMG", 18, 100}] | ||
|> TypeEncoder.encode(%FunctionSelector{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same same!
def deploy_erc20(conn, attrs) do | ||
with attrs <- Map.put(attrs, "account_uuid", Account.get_master_account().uuid), | ||
{:ok, _} <- authorize(:deploy_erc20, conn.assigns, attrs), | ||
{:ok, attrs} <- TokenGate.deploy_erc20(attrs), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In a future PR, Token will have a status and the gate should handle the original insert, deployment to Ethereum, tracking of the deployment and finally the update of the token to confirmed.
Also, could you link the documentations back to somewhere within |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I can confirm here that the current solc code in this PR, except for slightly reorganized dir path, is the same as https://github.com/OpenZeppelin/openzeppelin-contracts
- I'm getting the same compiled code as in this PR when compiling the solc files myself.
- Some usage suggestions below
@@ -0,0 +1,43 @@ | |||
pragma solidity ^0.5.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep these roles code in the same structure as openzepplin's ones? (i.e. contracts/access/Roles.sol
and contracts/access/roles/...
instead of current contracts/roles/...
) just so it's easy to trace back.
It'll also help that our solc code is the exact copy without having to change the import path.
Issue/Task Number: 703
Closes #703
Overview
This PR adds the ability to deploy 2 types of ERC20 tokens via a new endpoint
token.deploy_erc20
.The 2 types are:
mint
function which can be called untilfinishMinting
is called.This implementation does not require the solidity compiler to work. Indeed the 2 contracts are already pre-compiled and their binary/ABI has been extracted (source files can be found under
apps/eth_blockchain/contracts/ERC20
and compiled files underapps/eth_blockchain/contracts/compiled
).This feature is not production ready
Changes
token.deploy_erc20
Implementation Details
Contract sources were taken from https://github.com/OpenZeppelin/openzeppelin-contracts/tree/master/contracts and were not modified except for:
ERC20EWalletMintable.sol
ERC20EWallet.sol
ERC20MintableLocked.sol
Contracts were compiled using solidity compiler
0.5.0+commit.1d4f565a
, more info here.Usage
The easiest way to test is by using the updated swagger spec. To test the creation of a mintable token, the param
locked
needs to befalse
andtrue
for a non-mintable token.Impact
Run migrations.