-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Feature/asyncify contract #2394
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature/asyncify contract #2394
Conversation
… into feature/asyncify_contract
… into feature/asyncify_contract
… into feature/asyncify_contract
@pacrob from what I see I think this is it! Can you look through contract and see if anything jumps out at you that needs to be async? Also, what are your thoughts on testing. I threw a couple of test in there along the way and modified the AsyncEthereumTestProvider to enable testing. It seems a little bit too much to copy all the blocking test and make them async. I feel like we should pick out a few test here and there to do the testing. Thoughts?? I am going to go ahead and start working on the docs. Let me know if you have any feedback. |
Hi! Sorry for chiming in, but I think buildTransaction doesn't work yet:
And here
Sorry if I misunderstood something (: |
@MrNaif2018. Thanks for pointing that out! I actually made that change but just realized I didn't push the commit. When I am back by my computer later I will push it. If you see anything else let me know. |
@MrNaif2018 here are the changes for build_transaction. |
@dbfreem Yeah, it works now! estimate_gas always returns execution reverted but that's definitely not related to this PR |
@MrNaif2018 can you provide more details on your comment about estimate_gas I added it in a different PR. Is it not working correctly? |
Yeah. I am using infura on kovan testnet with contract The code used:
And if I provide gas param to The more generic code actually used: https://github.com/bitcartcc/bitcart/blob/2535c781db67082bd9085b77ce932f80d222822a/daemons/eth.py#L1116-L1123 |
@MrNaif2018 can you post the actual stacktrace. I am not seeing right off where the issue might be and I haven't been able to reproduce it with a unit test yet. |
I don't think this would help much, maybe it is something on my side. By the way it doesn't seem to allow passing chainId to EDIT: as I've thought, this is something my side, that's why I told you it's probably not related to this PR (: Adding from field made the estimation work. Though shouldn't |
@MrNaif2018 let me know if you find anything else with the async implementation. As of now I will assume this is an issue on your end. Thanks, for looking over this. It gives me more confidence that the Async Implementation of Contract works. |
Thanks, @dbfreem! Nothing jumps out as still needing to be asynced, but I'm going to get some more eyes on this later this week. I agree on tests - can still be thorough without copying everything over. I was looking at the test_contract_estimateGas and fell down a hole trying to find an async answer for functools - I found paco, but it's no longer maintained. Do you know of anything else? |
@dbfreem Missing Attribute for
|
Yeah, just to confirm: using factories from eth object itself didn't work for me in my testing so I used |
My bad regarding the Calls working now for me - so I'll continue migrating things over. Maybe I catch something when doing so. |
* fill_transaction_defaults async for later use in build_transaction_for_function * build_transaction in ContractFunction * estimateGas async in Contract and docs
* fill_transaction_defaults async for later use in build_transaction_for_function * build_transaction in ContractFunction * estimateGas async in Contract and docs
* fill_transaction_defaults async for later use in build_transaction_for_function * build_transaction in ContractFunction * estimateGas async in Contract and docs
More Async, I think the only thing left before test and docs is build_transaction_for_function.
I plan to work on build_transaction_for_function tomorrow.