Discussion: Support for Wallet JSON-RPC functions

gsmachado commented 3 years ago

I consider neo-express as a development and a testing tool. Thus, I truly believe that neo-express should provide at least the same JSON-RPC interfaces as the dotnet Neo node.

There are some questions/observations up to discussion below, and I would like to answer (or comment) them, giving my point of view.

Why Wallet JSON-RPC functions are relevant to neo-express?

Once @devhawk said: "neoxp is a dev tool first, testing tool second. The primary goal is to make the developer's life easier not to accomplish every single thing that neo-cli can do. neo-cli still exists and it can interoperate w/ neoxp via the export command."

While I agree with "neoxp is a dev tool first, testing tool second. The primary goal is to make the developer's life easier", I partially disagree with "not to accomplish every single thing that neo-cli can do". I think that good interface commands are crucial for development experience, and neo-express is great on the CLI commands it provides. But this is out of scope in this discussion. However, I think that neo-node (RpcServer) functions are very very important while developing software with SDKs in the Neo ecosystem. This is also some kind of "interface" to developers. In my opinion, JSON-RPC interface is at least as important as a CLI, if not more.

And that's why I suggest that RpcServer functions (in the case of our discussion here, the Wallet function) should be provided by neo-express -- to make our tools as consistent as possible.

Who uses such Wallet JSON-RPC functions?

As @steven1227 said, exchange developers use functions as sendmany, sendfrom, and potentially others. Those functions require a wallet to be "opened" in the node, using openwallet.

Given the fact that exchange developers might use neo-express to develop and test their software, this is another good indication to support Wallet JSON-RPC functions in neo-express.

Another fact, is: I gave tech support in hackathons (Neo2 times) where devs got so frustrated because different tools in our ecosystems supported different ways of doing things. Now imagine: you, as a developer, finding out that the dotnet Neo node supports the sendto call (because you read in the official docs website)... but then while developing a DApp, backend application, or whatever, things do not work because the development environment that you're using (in this case, neo-express) doesn't provide the basic same features. Frustrating. In my opinion, this should be avoided at all costs.

The question of WHY a given developer uses the sendto/sendfrom (and not building its own raw transaction and submitting it), it's absolutely not in the scope here. This is a DIFFERENT problem and should be discussed in the neo-modules repo.

If people agree in the neo-modules context (RpcServer) that Wallet JSON-RPC functions should be removed because there are no use cases for this, it's a strong sign to remove support for development and testing tools for this. (BTW: if a discussion like this pops up, I will be the first one to say 'guys, let's remove Wallet JSON-RPC functions for good').

Ok, I get that exchanges might use Wallet JSON-RPC functions... but since neo-express targets DApp developers, are exchange developers also DApp developers? What's the difference?

In my opinion, there's no difference, at all. They are simply developers that want to create applications for the Neo Blockchain. That's it.

Any developer could use any function provided by a Neo node, including the RpcServer, for any use case. It's not our role to anticipate the use cases and provide development and testing tools with half-way features here and there. This fact only helps to fragment development experience, ultimately leading to unsatisfied engineering souls. Something we don't want, right? 😄 😅

And before you argue "ok, let's have unsatisfied engineering souls complaining first, and then let's react" -- think it twice. We already had similar situations in the past. Should we try the same again?

...

Alright, @steven1227 and @devhawk, what's your opinion about this?

devhawk commented 3 years ago

And that's why I suggest that RpcServer functions (in the case of our discussion here, the Wallet function) should be provided by neo-express -- to make our tools as consistent as possible.

Neo express already provides RpcServer functions. neoxp provides a superset of JSON RPC methods compared to what a neo-cli node with the RpcServer plugin provides.

superboyiii commented 3 years ago

@gsmachado I agee with you, we should at least make tools support the same RPCs with CLI to avoid additional issues from developers. Also as what I saw, @devhawk has done a good job on neo-express to make it be a superset of JSON RPC methods. So what's the issue now on neo-express? If it's about sendfrom, I think add it to neo-express is not a bad idea since it still has senarios: sendto can't make fromAddress to be a specific one. As we know, cli's wallet can have many addresses. fromAddress is choosed via toAmount and balance comparison after list all addresses' balance in sendto. That's also a way for some exchanges to make transaction because they're using hot wallets and want make everything transparent. They always ensure one user one address and the asset on your address is the same as the balance of your exchange account so that user can go to explorer to see if any mismatch. And for ordinary user, if I have several addresses in a wallet（think about metamask）, I'd like to specific an address as from when I make Tx. But for the others who consider wallet as a whole thing, they could use sendto.

devhawk commented 3 years ago

Today, neoxp only supports wallets created by neoxp, which only ever have a single address (except for thr multisig contract shared by the consensus nodes). Issue #103 is tracking adding nep6 wallet support to neoxp, which will require cli argument support for multi account wallets

superboyiii commented 3 years ago

Today, neoxp only supports wallets created by neoxp, which only ever have a single address (except for thr multisig contract shared by the consensus nodes). Issue #103 is tracking adding nep6 wallet support to neoxp, which will require cli argument support for multi account wallets

We could add sendfrom in the same PR of support nep6, wait for https://github.com/neo-project/neo-express/issues/103 to be solved. In current neo-express, let's keep it like what's it is now. What do you think? @gsmachado @devhawk

devhawk commented 3 years ago

We could add sendfrom in the same PR of support nep6, wait for #103 to be solved. In current neo-express, let's keep it like what's it is now. What do you think? @gsmachado @devhawk

I think sendfrom is supported already.

neoxp doesn't currently use sendfrom (or any of the wallet based JSON RPC methods) to implement any of its commands, but it doesn't block any of them. The RpcServer config file disables openwallet (effectively disabling most wallet RPC methods), but neoxp doesn't read the RpcSettings config from disk - it programmatically generates the config settings. Since neoxp doesn't specify DisabledMethods configuration property, no methods are disabled.

steven1227 commented 3 years ago

I am not that familiar with the usage if send from in cli. For express, if the main target user is the dapp/smart contract developer, I think the current subset methods of cli on express is enough for them. If the exchange developers also use it for other development such as exchange trading/wallet system, I suggest them run a full-node cli directly.

superboyiii commented 3 years ago

We could add sendfrom in the same PR of support nep6, wait for #103 to be solved. In current neo-express, let's keep it like what's it is now. What do you think? @gsmachado @devhawk

I think sendfrom is supported already.

neoxp doesn't currently use sendfrom (or any of the wallet based JSON RPC methods) to implement any of its commands, but it doesn't block any of them. The RpcServer config file disables openwallet (effectively disabling most wallet RPC methods), but neoxp doesn't read the RpcSettings config from disk - it programmatically generates the config settings. Since neoxp doesn't specify DisabledMethods configuration property, no methods are disabled.

I just tried neo-express. It has already used RpcWallet and RpcClient so it's not a sendfrom issue, it's just a DisabledMethods issue since openwallet is disable in RpcWallet as default. Let's set "" as default so that everything can be solved. @devhawk @gsmachado

devhawk commented 3 years ago

I just tried neo-express. It has already used RpcWallet and RpcClient so it's not a sendfrom issue, it's just a DisabledMethods issue since openwallet is disable in RpcWallet as default. Let's set "" as default so that everything can be solved. @devhawk @gsmachado

RpcServerSettings.Load sets DisabledMethods to an empty array by default.

openwallet is specified by default in the RpcServer config.json file, but neo-express doesn't load settings that way

gsmachado commented 3 years ago

RpcServerSettings.Load sets DisabledMethods to an empty array by default.

openwallet is specified by default in the RpcServer config.json file, but neo-express doesn't load settings that way

Ohhh, alright. Things are way more clear now after reading this whole thread. Sorry, I was away from the keyboard for some days. 😄

Ok, recently @csmuller showed me that it's possible to use openwallet if you add a NEP-6 wallet file in the root of the project. Then, reading some messages @devhawk wrote on Discord last Friday, it's clear that neo-express has its "root" folder in the workspace -- and that's why it can read a NEP-6 wallet file that is located relative to this path. All fine!

This is already very good.

In the end, it's totally possible to do this:

openwallet_neow3j_and_neo-express

🎉

I think this closes this issue (at least from my side).

As @devhawk said, we can track the NEP-6 discussion/progress in #103.

devhawk commented 3 years ago

Ok, recently @csmuller showed me that it's possible to use openwallet if you add a NEP-6 wallet file in the root of the project. Then, reading some messages @devhawk wrote on Discord last Friday, it's clear that neo-express has its "root" folder in the workspace -- and that's why it can read a NEP-6 wallet file that is located relative to this path. All fine!

@csmuller should be able to pass a path (absolute or relative to the folder where the neo express file is) as the first parameter OpenWallet JSON RPC method

neo-project / neo-express