API eth_sign properties

aionnetwork / aion

Aion Network - Java Implementation

https://theoan.com/

MIT License

337 stars 112 forks source link

API eth_sign properties #672

Closed qoire closed 5 years ago

qoire commented 5 years ago

This is more of a design choice, but was there a reason we decided to implement eth_sign as such: https://github.com/aionnetwork/aion/blob/843761824132b2dfe2a6e8dbc5b755167f571718/modApiServer/src/org/aion/api/server/rpc/ApiWeb3Aion.java#L614-L636

Functionally it works, but the use of keccak256 when we (probably?) use blake2b elsewhere doesn't make sense. I vote we switch to blake2b because:

There are no compatibility issues, the VM does not support either hashing algorithm right now
It's more consistent with what developers and users expect when using Aion

For context, I stumbled on this adding the offline signing feature for aion-keystore

AionJayT commented 5 years ago

Agree your thought. We can arrange resource to revise this in the next release.

qoire commented 5 years ago

Just discussed this with @ali-sharif, let's do this:

Right now eth_sign is in a half and half state, lets restore it to its former glory by going back to the original eth_sign header:

https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign

This let's us keep compatibility with Ethereum. Then we create a new JSON-RPC call for aion_sign. I'm not exactly sure yet how this will be exposed in the API (web3, JSON-RPC) but right now eth_sign is deceptive.

ali-sharif commented 5 years ago

I vote to completely remove eth_sign from the json rpc api. It's confusing, since people think this function let's you sign a raw transaction (which it doesn't) and most people don't know about the payload signature scheme to validate random data (which this endpoint was designed for)

aion-kelvin commented 5 years ago

I think we should keep it around, in some form.

IMO, the confusion can be cleared up through good/consistent API documentation.

Removing it could break existing code if they depend on this functionality. For instance, in web3.js v1.x.x, there is a sign function that calls this RPC method. If someone wrote code that depends on this, removing it would break them.

It also makes us deviate further from Ethereum's RPC spec, making it harder for people to port over their software from Ethereum to Aion. There are use cases for this functionality, i.e. this tutorial https://programtheblockchain.com/posts/2018/02/17/signing-and-verifying-messages-in-ethereum/

Lastly, it seems strange to me from a design/philosophy perspective to not support this, since signing things with your account is sort of a fundamental, primitive operation of blockchains.

AionJayT commented 5 years ago

the aion web3 support the sign feature.