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

eth/api: rename signAndSendTransaction to sendTransaction #2712

Merged
merged 1 commit into from
Jul 22, 2016

Conversation

bas-vk
Copy link
Member

@bas-vk bas-vk commented Jun 17, 2016

See ethereum/EIPs#113. Will update web3 when it released a new version with this method included.

@frozeman

@robotally
Copy link

robotally commented Jun 17, 2016

Vote Count Reviewers
👍 1 @karalabe
👎 0

Updated: Fri Jul 22 11:19:30 UTC 2016

@codecov-io
Copy link

codecov-io commented Jun 17, 2016

Current coverage is 52.16%

Merging #2712 into develop will decrease coverage by 4.60%

@@            develop      #2712   diff @@
==========================================
  Files           217        217           
  Lines         24865      24867      +2   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
- Hits          14114      12971   -1143   
+ Misses        10750      10746      -4   
- Partials          1       1150   +1149   

Powered by Codecov. Last updated by 4f3f6e2...4801d6a

@karalabe
Copy link
Member

Mostly discussion/opinion:

I'm a bit reluctant from this change because we're starting to litter our personal namespace with various methods that perhaps should have been part of eth in the first place. There could be a potential confusion in what the difference is between eth.sendTransaction and personal.sendTransaction.

Couldn't we simply drop eth.sendTransaction altogether and have only the personal version remaining with the password being optional? This could potentially solve a lot of issues in the long run (both confusion as well as security wise).

@bas-vk
Copy link
Member Author

bas-vk commented Jun 21, 2016

Removing eth_sendTransaction would break a lot of current applications and I don't expect this to happen anytime soon. There is a valid use case for it in a trusted environment.

Currently there is a clear separation between method that accept sensitive input and the one's that are not. I would like to keep this separation. If we let eth_sendTransaction accept an optional password this might give a false signal that it is safe to use on a http/ws interface.

@karalabe
Copy link
Member

I meant to keep both versions inside personal and completely drop from eth.
It's not safe anyway.
On Jun 21, 2016 18:59, "bas-vk" notifications@github.com wrote:

Removing eth_sendTransaction would break a lot of current applications
and I don't expect this to happen anytime soon. There is a valid use case
for it in a trusted environment.

Currently there is a clear separation between method that accept sensitive
input and the one's that are not. I would like to keep this separation. If
we let eth_sendTransaction accept an optional password this might give a
false signal that it is safe to use on a http/ws interface.


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2712 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAH6Gb6hoRNUkAseowfbqtpucCpPgocJks5qOApHgaJpZM4I4OGO
.

@bas-vk
Copy link
Member Author

bas-vk commented Jun 29, 2016

I understand what you mean, but dropping it from the eth namespace will break a lot of things.
This PR is only a rename of the old method as discussed in the EIP.

@frozeman
Copy link
Contributor

frozeman commented Jun 30, 2016

dropping this from eth is not an option, its the main way people use the node currently.

In the future when we move accounts out of the node we can discuss that. Currently the personal namespace is also not official part of the RPC spec, so changes are still fine. In eth we need to be backwards compatible.

Concerning the name change, we had an ERC discussing the name with the community: ethereum/EIPs#113

So please merge

@obscuren
Copy link
Contributor

Rebase please.

@bas-vk
Copy link
Member Author

bas-vk commented Jul 15, 2016

@obscuren, rebased

@bas-vk
Copy link
Member Author

bas-vk commented Jul 22, 2016

rebased again

@frozeman
Copy link
Contributor

Please merge

@karalabe
Copy link
Member

LGTM 👍

@karalabe karalabe merged commit f58ac2b into ethereum:develop Jul 22, 2016
@obscuren obscuren removed the review label Jul 22, 2016
@bas-vk bas-vk deleted the sendtx branch July 22, 2016 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants