CityOfZion / neon-js

Javascript libraries that allow the applications to interact with NEO blockchain
https://docs.coz.io/neo3/neon-js/index.html
MIT License
183 stars 166 forks source link

Smart RPC calculateNetworkFee or restructure and remove deprecation note #849

Closed ixje closed 2 years ago

ixje commented 2 years ago

https://github.com/CityOfZion/neon-js/blob/bb298756d4bcb6edeb3c545816bc9c92bceae3c3/packages/neon-api/src/api/calculateNetworkFee.ts#L11-L13

The calculateNetworkFee function is marked deprecated and suggests using the "recently" introduced calculateNetworkFee RPC method. I think this is a good suggestion because recently there are some hard fork things happening in the code that affect interop prices. However, using the RPC method isn't straight forward at all. Let me elaborate

In order for the calculateNetworkFee RPC method to work correctly one needs a transaction with sufficient witnesses. The problem is that witnesses aren't added until we sign, and we can't sign until we have the network fee because that is part of the signed data. So we we have a chicken-egg problem.

The solution is to add temporary witnesses (with at least a valid verificationScript) prior to calling the RPC method, then remove them after obtaining the fee and prior to signing. This is internal knowledge that we can't expect most users to have. This trick is actually what the existing code is doing but locally.

Now comes the question how to make this accessible to the end-users, I see 2 options

  1. remove this from the existing function https://github.com/CityOfZion/neon-js/blob/bb298756d4bcb6edeb3c545816bc9c92bceae3c3/packages/neon-api/src/api/calculateNetworkFee.ts#L44-L64, replace it with a RPC call to calculateNetWorkFee passing txClone as input and remove the deprecated note. This means the function needs to become async and it leaves you with an RPC call that is arguably unusable without the internal knowledge described above and pushed that responsibility to the user.
  2. Make calculateNetworkFee intelligent by adding the temporary witnesses there.

-edit- my vote would go for option 2, what's your take @snowypowers ?

snowypowers commented 2 years ago

I think having the RPC call work differently is quite dangerous, it distorts the replies coming in from the RPC endpoint and might confuse the user, especially when they use other languages or the RPC endpoint directly. Traditionally, I would expect an RPC client to merely return the data or slightly decorate the data for readability.

How about a different approach where we have a new method that uses the calculateNetworkFee RPC call intelligently (aka merging the best of both methods into a smartCalculateNetworkFee).

How does neo py resolve this issue?

ixje commented 2 years ago

Neo-mamba doesn’t handle it yet as i only recently realized this is an issue for the calculateNetworkFee rpc method. I plan to at least raise an error if the signer count doesn’t match the witness count because then it will always fail on the receiving side. In this error i will probably suggest to specify an additional argument that can solve it for them by adding the basic witnesses. Mamba already does some input pre-processing on other RPC calls anyway, so it doesn’t deviate from the current “standard”.

For neon-js a new method sounds fine as well. I just wasn’t very creative in how to name it as ideally it’s jist called “calculateNetworkFee” :-)

On 2 Jun 2022, at 08:28, Yak Jun Xiang @.***> wrote:

 I think having the RPC call work differently is quite dangerous, it distorts the replies coming in from the RPC endpoint and might confuse the user, especially when they use other languages or the RPC endpoint directly. Traditionally, I would expect an RPC client to merely return the data or slightly decorate the data for readability.

How about a different approach where we have a new method that uses the calculateNetworkFee RPC call intelligently (aka merging the best of both methods into a smartCalculateNetworkFee).

How does neo py resolve this issue?

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

ixje commented 2 years ago

completed in v5.2.0 release