Add Constructor Function #595

dangell7 · 2018-01-28T03:36:50Z

There was no easy way to deploy a contract with a signed raw transaction.

How was it fixed?

By adding the deploy_data function. It can be used like so....

`

greeter_compiled_sol = compile_files(['contracts/Greeter.sol'])
greeter_contract_interface = greeter_compiled_sol['contracts/Greeter.sol:Greeter']
greeter_contract = w3.eth.contract(abi=greeter_contract_interface['abi'], bytecode=greeter_contract_interface['bin'])

contract_data = greeter_contract.deploy_data()

print(contract_data)

key = decrypt_account(w3.eth.coinbase, 'Coinbase-Master')

gas_price = w3.eth.gasPrice
gas_limit = 7000000

nonce = w3.eth.getTransactionCount(w3.eth.coinbase)

transaction = {
    'nonce': nonce,
    'gasPrice': gas_price,
    'gas': gas_limit,
    'to': '',
    'value': 1000,
    'data': contract_data,
    'chainId': None
}


signed = w3.eth.account.signTransaction(transaction, key)

raw_tx_hex = Web3.toHex(signed.rawTransaction)

print('Raw Tx Hex: %s' % raw_tx_hex)

tx_hash = w3.eth.sendRawTransaction(raw_tx_hex)

while w3.eth.getTransactionReceipt(tx_hash) == None:
    time.sleep(5)
    print('Waiting on Tranasaction')

tx_receipt = w3.eth.getTransactionReceipt(tx_hash)

contract_address = tx_receipt['contractAddress']

print('Greeter Contract Address: {}'.format(contract_address))

`

Cute Animal Picture

pipermerriam · 2018-01-28T03:41:04Z

@harpangell7 this feature is captured here in #577 which defines an alternate API for this functionality which is more inline with the existing API for contract functions and events.

Please note that @zheli has claimed that issue, however, there is no open pull request at this time. I would encourage you to reach out to @zheli prior to investing time in converting this PR to match #577 (assuming you are interested in claiming the bounty).

pipermerriam · 2018-01-28T03:43:14Z

Also, for contributions to be something we can merge, this would need to have tests and documentation written to cover the new functionality.

dangell7 · 2018-01-28T03:55:56Z

Honestly man, I'm not sure I feel comfortable asking for money lol. I just did it because I needed the functionality. I would hate to claim a bounty and not be able to produce. I will keep your tests and documentation in mind though. And I really do appreciate all the work that YOU do!! I'll just use the new API when its released, until then, I have my fork. lol

pipermerriam · 2018-01-28T04:01:22Z

I only mention the bounty to be sure there's as little contention as possible. You are welcome to contribute the feature of your own altruistic/whatever motivation. Either way works.

dangell7 · 2018-01-28T05:43:57Z

Well I needed to learn the tests anyways. So I added them and the documentation. But now my "check" with Travis it fails. :( Aslo, the tests dont assert the deploying to the chain, only that they can sign and return the correct signed transaction details. I couldn't find the tests on the sendRawTransaction().

zheli · 2018-01-28T09:19:06Z

@harpangell7 I have only done some research on current library interface and how it can be done, there is no serious code have been written yet. I would be happy to let you claim this issue (#577) if you want to.

dangell7 · 2018-01-28T17:31:00Z

Yeah I'd love to give it a try!! Give me 2 days, and if I cant figure it out, then I'll need some help.

zheli · 2018-01-28T19:39:06Z

@harpangell7 I don't know how to say this, but I actually made some progress on my pull request today... maybe we can work on it together and share the bounty? 😂

dangell7 · 2018-01-28T20:20:12Z

Thats fine, I'm working on the constructor api now. So if you want to tackle the fallback and we can meet in the middle?

zheli · 2018-01-28T21:35:16Z

@harpangell7 I think one of us should branch off from the other's branch to make it easier to track changes:) Maybe you can also tell me briefly how do you want the new constructor to work?

Right now I just need to ask @pipermerriam or @carver what do they mean by deprecating contract.constructor().deploy :)

dangell7 · 2018-01-28T22:00:33Z

@zheli go ahead and take this one. I was working on it but got busy and you claimed it first. But to answer your question I think they just mean deprecating contract.deloy() and making the contract.constructor().transact() the new deploy method. I'm curious to see how it turns out though!

dangell7 · 2018-01-29T01:51:44Z

@zheli I made a push to show you how I would do it.

zheli · 2018-01-29T08:00:45Z

@harpangell7 thx man! Regarding to constructor(*,**), I am not sure if they mean to create a new constructor method in Contract class. As constructor is also a concept in object-oriented programming, in this case it could mean __init__(). At least that's how I understand it.

dangell7 · 2018-01-29T08:09:25Z

Oh interesting, I was thinking they meant the literal constructor, as contract(abi, bytecode) already init()'s the class, therefore, calling the constructor on the class made more sense to me. See the language is def lost. lol I also read the fallback as literally contract.fallback(*args, **kwargs).estimateGas(). This would eliminate the need to check for an actually function called fallback, because that function would be called using contract.functions.fallback(*args, **kwargs).estimateGas(). But its really up to them. lol

pipermerriam

Lets see if I can answer all the questions.

We do actually mean a function named constructor so the literal API would be contract_instance.constructor(*args, **kwargs).transact({...})

I'm fine with you two working collaboratively on this. I'd prefer that the two of you determine how you want to split the bounty if you work on this collaboratively. I'd prefer to not be tasked with making that subjective decision.

@harpangell7 RE: the signing and sendRawTransaction testing.

I think your tests which assert things about the signed data are not really testing the correct thing, or at least, they are opaque in terms of what they are testing. Instead, an approach that I think will produce better tests would be:

use c.constructor(...).buildTransaction() to build the txn dict.
send normal deploy transaction with c.constructor(...).transact()
compare built transaction with the transact() transaction and see that their data fields match.

pipermerriam · 2018-01-29T17:44:14Z

docs/contracts.rst

+        'nonce': w3.eth.getTransactionCount(w3.eth.coinbase),
+        'chainId': None
+        }
+        >>> contract_data = token_contract.constructor(web3.eth.accounts[1], 12345).buildTransaction(transaction)


This code example seems wrong. The return value from token_contract.constructor(...).build_transaction(...) should be a transaction dictionary. Then, that returned dictionary should be what is passed into web3.eth.account.signTransaction.

Make sense?

Yes, that makes sense and I will add it now

pipermerriam · 2018-01-29T17:45:22Z

tests/core/contracts/test_contract_constructor.py

+    #assert Web3.toHex(signed.hash) == tx_hash
+
+    #account = acct.privateKeyToAccount(private_key)
+    #assert account.signTransaction(txn) == signed


looks like this test has all of it's assertions commented out.

Will Finish

pipermerriam · 2018-01-29T17:46:20Z

tests/core/contracts/test_contract_constructor.py

+    ),
+    ids=['web3js_example', '31byte_r_and_s'],
+)
+


odd extra whitespace.

pipermerriam · 2018-01-29T17:46:27Z

tests/core/contracts/test_contract_constructor.py

+    ),
+    ids=['web3js_example', '31byte_r_and_s'],
+)
+


odd extra whitespace.

pipermerriam · 2018-01-29T17:46:37Z

tests/core/contracts/test_contract_constructor.py

+    ids=['web3js_example', '31byte_r_and_s'],
+)
+
+


extra whitespace.

pipermerriam · 2018-01-29T18:13:24Z

web3/contract.py

@@ -268,6 +287,52 @@ def deploy(cls, transaction=None, args=None, kwargs=None):
        txn_hash = cls.web3.eth.sendTransaction(deploy_transaction)
        return txn_hash

+    @classmethod
+    @deprecated_for("contract.<constructor>.buildTransaction")
+    def deploy_data(cls, transaction=None, args=None, kwargs=None):


Can we remove this method since it was never part of the original API.

pipermerriam · 2018-01-29T18:14:08Z

web3/contract.py

@@ -1039,6 +1195,93 @@ def call_contract_function(abi,
        return normalized_data


+# Transact Constructor
+# TODO


leftover comment?

Sorry, I leave my functions as TODO's until finished.

dangell7 · 2018-01-29T20:33:38Z

Gotcha, yeah the tests were definitely not perfected but I have a good place to start now.

dangell7 · 2018-01-29T22:29:57Z

@pipermerriam I'm running into some trouble with the tests now. When I call contract.constructor().buildTransaction() the result transaction doesn't include the "to" or the "nonce" fields. If I then try to pass this into the signTransaction I get the "TypeError: Not all fields initialized". So then if I try to enter the 'to' field, I have it giving me the 'ValueError: Cannot specify to for contract deployment' which makes sense. And If I try to pass the nonce, I receive a "Only the keys 'data/from/gas/gas_price/to/value' are allowed. Got extra keys: 'nonce'" error. I have made tests to assert that I get an error when trying to pass this information as there shouldn't be a deploy to address and the nonce I'm still trying to figure out. (I think it might just be a test thing, I'm checking in production). The nonce is a new error I haven't seen and I'm not sure how to handle it. Furthermore, the transaction v, s r from the transact() and the v, s, r from the buildTransaction() are different. I can confirm that the data is the same. I will continue testing until its production ready, I just wanted to update you.

pipermerriam · 2018-01-29T22:34:36Z

cc @carver ^ might be something we need to address with how sign transaction validates contract deployment transactions.

Regarding signing the transactions. As for tests, I don't think you should need to sign the transaction as you should be able to simply submit it over web3.eth.sendTransaction without signing it. Maybe I'm missing something.

carver · 2018-01-29T22:57:59Z

it will automatically pre populate the "to" as ""

Cool that's roughly the approach I was going to suggest @harpangell7 . My instinct is that to should be None, and if that doesn't work in signTransaction then I'd like to update that function.

dangell7 · 2018-01-29T22:58:52Z

@pipermerriam @carver So buildTransaction() adds the default chainId and this is giving me a "eth_tester.exceptions.ValidationError: Only the keys 'data/from/gas/gas_price/nonce/to/value' are allowed. Got extra keys: 'chainId'". when I try to run web3.eth.sendTransaction. I've confirmed that this is only a testing error, as I can use buildTransaction({'chainId': 1}) just fine in my program.

dangell7 · 2018-01-29T23:02:07Z

@carver So I was incorrect in stating that it will auto 'to' to "". The reason it was doing that was because I hacked in built_transaction['to'] = '' just before returning to the user. Will there ever be an instance when they can deploy to a specific address? Not to sound stupid, but I want to make sure. With the current hack though, when you sign the transaction, it changes the "" to None.

carver · 2018-01-29T23:10:00Z

buildTransaction() adds the default chainId and this is giving me a "eth_tester.exceptions.ValidationError: Only the keys 'data/from/gas/gas_price/nonce/to/value' are allowed. Got extra keys: 'chainId'"

Can you open an issue on eth-tester about this (and link back here)?

Will there ever be an instance when they can deploy to a specific address?

Nope, the defined way to deploy a contract is to leave the to field empty. So building a transaction from a constructor must set to to empty.

dangell7 · 2018-01-29T23:27:10Z

@carver Purrfect!

dangell7 · 2018-01-31T09:05:17Z

@carver I’m just waiting on the eth-tester issue before I submit again...

nueverest · 2018-03-02T22:34:08Z

Would love to see this working.

carver · 2018-03-02T22:45:36Z

@nueverest Please subscribe to #666 which will be updated when the problem is fixed (even if this particular PR doesn't get merged)

carver · 2018-04-03T18:48:41Z

Resolved in #693

dangell7 · 2018-04-04T18:29:39Z

web3/contract.py

+                                                 *self.args,
+                                                 **self.kwargs)
+
+        built_transaction['to'] = ''


Here was my suggestion to fix that bug. #739

Add Deploy Data Function

e3fca67

Add Deploy Data Function

92a5659

Add Deploy Data Function

6e3dd79

Add Constructor Function

613e516

dangell7 added 5 commits January 28, 2018 21:00

Add Constructor Function

195de0d

Add Constructor Function

ac955a3

Add Constructor Function

9816029

Add Constructor Function

29e8655

Add Constructor Function

398e742

dangell7 changed the title ~~Add Deploy Data Function~~ Add Constructor Function Jan 29, 2018

pipermerriam requested changes Jan 29, 2018

View reviewed changes

Add Constructor Function

7e9dde2

dangell7 mentioned this pull request Jan 29, 2018

Validate Transaction Function Doesnt allow for chainId ethereum/eth-tester#56

Closed

This was referenced Feb 12, 2018

Support for fallback and constructor functions #577

Closed

What is preventing contract deployment with sendRawtransaction? #633

Closed

carver mentioned this pull request Feb 20, 2018

Pop and validate chainId, if provided with txn #656

Merged

carver added this to the Version 4 Stable milestone Feb 21, 2018

carver mentioned this pull request Feb 22, 2018

Constructor Functions #666

Closed

carver removed this from the Version 4 Stable milestone Feb 22, 2018

zheli mentioned this pull request Mar 12, 2018

Constructor functions #693

Closed

carver closed this Apr 3, 2018

dangell7 commented Apr 4, 2018

View reviewed changes

web3/contract.py

*self.args,

**self.kwargs)

built_transaction['to'] = ''

Copy link

Author

dangell7 Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here was my suggestion to fix that bug. #739

Add Constructor Function #595

Add Constructor Function #595

Conversation

dangell7 commented Jan 28, 2018

Cute Animal Picture

pipermerriam commented Jan 28, 2018

pipermerriam commented Jan 28, 2018

dangell7 commented Jan 28, 2018

pipermerriam commented Jan 28, 2018

dangell7 commented Jan 28, 2018 • edited Loading

zheli commented Jan 28, 2018 • edited Loading

dangell7 commented Jan 28, 2018

zheli commented Jan 28, 2018

dangell7 commented Jan 28, 2018

zheli commented Jan 28, 2018

dangell7 commented Jan 28, 2018

dangell7 commented Jan 29, 2018

zheli commented Jan 29, 2018 • edited Loading

dangell7 commented Jan 29, 2018

pipermerriam 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangell7 commented Jan 29, 2018

dangell7 commented Jan 29, 2018

pipermerriam commented Jan 29, 2018

carver commented Jan 29, 2018

dangell7 commented Jan 29, 2018

dangell7 commented Jan 29, 2018

carver commented Jan 29, 2018

dangell7 commented Jan 29, 2018

dangell7 commented Jan 31, 2018

nueverest commented Mar 2, 2018

carver commented Mar 2, 2018

carver commented Apr 3, 2018

Choose a reason for hiding this comment

dangell7 commented Jan 28, 2018 •

edited

Loading

zheli commented Jan 28, 2018 •

edited

Loading

zheli commented Jan 29, 2018 •

edited

Loading