Skip to content
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

sendTransaction() async needs defining #131

Closed
ethers opened this issue Mar 19, 2015 · 16 comments
Closed

sendTransaction() async needs defining #131

ethers opened this issue Mar 19, 2015 · 16 comments
Labels
Enhancement Includes improvements or optimizations

Comments

@ethers
Copy link
Contributor

ethers commented Mar 19, 2015

On develop branch commit de5de9e

sendTransaction() sync is currently returning an address if it creates a contract. For a non-creation, what will it return?

In the async case, sendTransaction() callback returns an address before the contract is even mined, which loses the point of it being async. Can the async sendTransaction() callback only fire after the contract has been mined successfully?

When calling sendTransaction(), Alethzero warning dialog pops up which allows the user to Reject the tx. Currently sendTransaction() ignores the user's response to this dialog. If the user chooses "Reject" on the dialog, sendTransaction() should return that the user rejected the tx.

Some suggestions for the above:

  • make the sync version of sendTransaction() return nothing
  • the async version will then return one of the following:
  • a) false (bool) if user Rejects the tx
  • b) contract address if a contract was created
  • c) transaction id
@drcode
Copy link

drcode commented Mar 20, 2015

One additional complexity is that there is no certainty that a transaction has ever been truly mined, given the possibility of forks. Ideally, there would be a way to check whether a transaction has been "unmined" as well (i.e. because a fork of the chain not containing the mined transaction has just become the longer fork)

@SilentCicero
Copy link

Agreed, we need some infrastructure for this, and I'm having a tone of trouble with Ethereum.js sendTransaction to Go-Ethereum CLI 8.6 and 9+, I can get contracts like the multiply contract to work, but not contracts like SimpleStorage. This has been affirming my sanity, b/c I know it's not my own contracts/code.

contract SimpleStorage { uint storedData; function set(uint x) { storedData = x; } function get() constant returns (uint retVal) { return storedData; } }

**Ignore this, the latest dev build has addressed these issues.

@ethers
Copy link
Contributor Author

ethers commented Apr 2, 2015

is this issue (131) being postponed or can it be made part of 0.3.0 like #133 ?

@frozeman
Copy link
Contributor

Sorry for not answering we are all very busy right now.

  1. -> it will return the transaction hash see https://github.com/ethereum/wiki/wiki/JavaScript-API#web3ethsendtransaction return value.
  2. using a callback is just calling the RPC method asynchronously and will return the same as the sync function. On the RPC side there is no way to get a notification whether the tx was mined or not. you would need to check that yourself with a script looking at the next block.
    We could think of integrating this into web3.js, but thats out of scope for now.
  3. True the error reporting is bad therefor we came up with a better error scheme (https://github.com/ethereum/wiki/wiki/JSON-RPC-Error-Codes-Improvement-Proposal), which needs to be implemented into the clients first and then properly passed to the js users. Then we would give back a JSON RPC error {code: 1, message 'Unauthorized'}
    See: Add new error codes according to new spec ethereum/go-ethereum#768

@frozeman frozeman added the Enhancement Includes improvements or optimizations label Apr 21, 2015
@ethers
Copy link
Contributor Author

ethers commented Apr 26, 2015

Thanks for 1 and 3. sendTransaction() async does need improvement because even with improved error reporting, the sync version of sendTransaction() will never be able to return the error corresponding to "user rejected the tx"

@frozeman
Copy link
Contributor

why not?

@ethers
Copy link
Contributor Author

ethers commented Apr 28, 2015

sync version of sendTransaction() can't return "user rejected tx" because it has to wait for the user to click on the (AZ) dialog "Do you accept or reject this transaction?" (the user might be very fast at clicking dialog, or they could take a long time, and I don't think the sync version can/should block indefinitely until user clicks)

@frozeman
Copy link
Contributor

sure, but that why we introduced the optional async. The developer has to decide when to use what. But i personally agree. freezing interfaces is a no go and thats why chrome deprecated that.

But you would still get the not confirmed error on the end and you code can act on it.

@debris debris closed this as completed May 21, 2015
@radster360
Copy link

I think there has to be a better solution here. We have head-less client, so there is no availability of an interface where a user would be responding to some pop-up, We need to do this programmatically. So an we achieve this?

@kumavis
Copy link
Contributor

kumavis commented May 28, 2015

@radster360 I'm not sure about your usecase or your problem specifically. What are you trying to achieve that you don't think you will be able to?

@radster360
Copy link

@kumavis We had a discussion with Fabian on Skype, after posting the request here. He had provided a solution, which we will give it a try. Though the solution is a bit prohibitive (need to watch for 12 blocks and need to deal with forking of chain ourselves). Few other folks on the "Solidity" Skype group has similar issue as ours, so it might not be one off issue. So, let me try to explain what my usecase/problem is.

We have an application which is hosted using node.js that takes in requests from end users (with or without UI). The node.js is using Ethereum JS to transact against block chain. The user requests are translated into sendTransaction() call. The API call needs to be async, where we can set up a callback function when a specific action was completed. In our case, when the transaction was mined and request processed ( majority of case), we need to make additional changes in our non-Ethereum part of the system. Yes, there is callback parameter for sendTransaction(), but doesn't seem to be functioning in our case, or what we are trying to do is not possible in current implementation of Ethereum JS API.

During the conversation, couple of work arounds were suggested and we will try them out also. I hope this helps to explain my use case.

@kumavis
Copy link
Contributor

kumavis commented May 29, 2015

@radster360 so it sounds like you want a callback that returns after the tx is sealed in a block and at N-depth. That sounds generally useful, and like something many people would want.

@ethers
Copy link
Contributor Author

ethers commented May 29, 2015

+1 for an extension to web3.js or a separate lib that does a callback when a tx is "sealed". Let us know how it goes @radster360 Thanks

@radster360
Copy link

@kumavis I think that is a base requirement. Of course, that is for the "sealed" or "success". There might be other states that one would have to consider too, for example, "Aborted", "Failed", but I think those are application specific things and that could be handled (I think) by defining application specific events (NotEnoughBalance or NotOriginalOwner, etc.) Thanks for looking into this.

@frozeman
Copy link
Contributor

Currently the callbacks, just make calls async, we could think of adding more functionality in web3.js, but this will generally blow the api to do more than what just the api should do.

Its probably better to create an on top library which facilitates these things.

Additionally i proposed a change to the error reporting, which is still in the making, and might take some time: ethereum/go-ethereum#768

@radster360
Copy link

@frozeman Thanks Fabian. If having another library makes sense that provides the functionality - I am good. I will also keep an eye out for #768

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Includes improvements or optimizations
Projects
None yet
Development

No branches or pull requests

7 participants