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

Strange Behavior when using sc.ContractParam #850

Closed
melanke opened this issue May 31, 2022 · 10 comments
Closed

Strange Behavior when using sc.ContractParam #850

melanke opened this issue May 31, 2022 · 10 comments

Comments

@melanke
Copy link
Contributor

melanke commented May 31, 2022

Hello, I notice a problem with some methods of sc.ContractParam, they can't be directly used with `invokeFunction.

How to Reproduce

Making a simple request like the following:

const params = [sc.ContractParam.hash160(address)]; // <------- using the method directly

const res = await new rpc.RPCClient(rpcAddress).invokeFunction(
      '0xb2c806468effacd2951b913e724e4c3f8506a357',
      'balanceOf',
      params
    )

Result

Results in a request body like the following:
(This result was retrieved by monitoring the Chrome Dev Tools, Network tab)

{
   "params":[
      "0xb2c806468effacd2951b913e724e4c3f8506a357",
      "balanceOf",
      [
         {
            "type":20,
            "value":{
                // <------- empty object
            }
         }
      ],
      [
         
      ]
   ],
   "jsonrpc":"2.0",
   "id":1234,
   "method":"invokefunction"
}

The value is an empty object.

Workaround

I can workaround this problem by using toJson method:

const params = [sc.ContractParam.hash160(address).toJson()];

Result

Resulting in a working request body:

{
   "params":[
      "0xb2c806468effacd2951b913e724e4c3f8506a357",
      "balanceOf",
      [
         {
            "type":"Hash160",
            "value":"cc776527da4a34b80f411b0ccb9dffddb38523ae" // <-------
         }
      ],
      [
         
      ]
   ],
   "jsonrpc":"2.0",
   "id":1234,
   "method":"invokefunction"
}

I had to use toJson with the following methods:

  • hash160
  • byteArray
  • string (not always)
  • integer
  • publicKey

Is this expected? Is it happening to you too?

@lllwvlvwlll
Copy link
Member

Which version of neon-js are you experiencing this on? We use and identical pattern to the one you reference in the props sdk for all test invokes and I have not encountered the problem.

Method:
https://github.com/CityOfZion/props/blob/develop/sdk/src/api/interface.ts#L23

@ixje
Copy link
Member

ixje commented Aug 12, 2022

@melanke is this still an issue or can it be closed?

@ixje
Copy link
Member

ixje commented Aug 15, 2022

@snowypowers I need some expert help here to solve this one. The following works succesfully

const { sc, rpc } = require("@cityofzion/neon-js");
// const { rpc } = require("@cityofzion/neon-js");
// const { sc } = require("@cityofzion/neon-core");
async function f() {
  const params = [
    sc.ContractParam.hash160("NcLx6gjoLCYF6VkBhQ25SQxGFd5o3YHs5s"),
  ];
  const res = await new rpc.RPCClient(
    "http://seed1t5.neo.org:20332"
  ).invokeFunction(
    "0xef4073a0f2b305a38ec4050e4d3d28bc40ea63f5",
    "balanceOf",
    params
  );
  return res;
}

f()
  .then((r) => console.log(r))
  .catch((r) => console.log(r));

now comment the top line and uncomment line 2 + 3 such that sc is imported from neon-core instead. Now it fails to properly convert hash160 somehow.

In the working case the contract param inside the query looks as follows
image

in the failing case at the same point it looks like this
image

-edit- to fix screenshot

@ixje
Copy link
Member

ixje commented Aug 15, 2022

-update-

In the failing case inside invokeFunction()

params.map((p) => (p instanceof ContractParam ? p.toJson() : p)),

it does not seem to think that the param is an instance of ContractParam and thus does not call toJson()

Again, working screenshot at invokeFunction
image

and none working
image

Objects both seem to indicate to be a ContractParam in the debugger, but the instanceof check seems to think different. Any chance you could help solve this?

@snowypowers
Copy link
Member

Looks like we have encountered one of the more annoying parts of JS which is how the code is packaged and imported. So if you import both neon-js and neon-core, the javascript environment will contain 2 instances of the same code but they are different instances. So, instanceof does not work very well as they differentiate between neon-js.ContractParam and neon-core.ContractParam.

What's the use case here that you will require to import these 2 together?

@ixje
Copy link
Member

ixje commented Aug 19, 2022

What's the use case here that you will require to import these 2 together?

I'll let @melanke answer that one.

As a side note, it's not the first time I've heard people question the package structure. Not in the sense of "it's bad" but in the sense of "I don't understand why it's like that or needed like this". I'm personally not a neon-js (or JS) user so I have no opinion on it. It seems like the workaround I proposed in #868 is just for a single use-case we now happen to run into but isn't going to solve any others we don't know of yet. We should probably put a big warning somewhere to always import from one package?

@melanke
Copy link
Contributor Author

melanke commented Aug 19, 2022

@snowypowers after we could identify how to reproduce the error and how to workaround I was imagining the error was caused exactly by what you are describing, indeed, very annoying.

I don't have an use case where I NEED to import they as you asked. It was a mistake, and since we have a workaround it is not a problem for me anymore.

But I am afraid this might lead to problems and confusion for other developers, don't you agree?
I think we should try to estimate the effort of fixing it to see if it's really worth it, if not, we should document this issue so other developers can avoid spending as much time as I spent to figure this out.

@ixje
Copy link
Member

ixje commented Aug 22, 2022

As I'd like todo a release this week I want to keep this conversation going.

We should probably put a big warning somewhere to always import from one package?

Some ideas on this

  1. put a note in the "Usage" section of the readme
  2. put the same note in the installation section of the docs. I choose installation section here because in this doc the installation specifically discuss importing neon-js vs neon-core.

Draft note:

Note: mixing import neon-core and neon-js (the latter which includes neon-core) in the same file can lead to undesired behaviour (example). Import neon-js only in such scenario's.

@snowypowers is this import issue file based or project based? Please fix the above note with your insights :-)

@snowypowers
Copy link
Member

its project-based (but by that definition, file-based too since project is a superset). Ideally, a project just needs to install either 1 dependency and use it throughout.

Maybe its easier to just recommend neon-js? For most of the developers, I would imagine neon-js fits 80% of the use cases. neon-core is made for certain niche development scenarios (e.g. extending it into a full node or a website that only wants readonly access to the network, etc).

@ixje
Copy link
Member

ixje commented Aug 24, 2022

completed in v5.2.0 release

@ixje ixje closed this as completed Aug 24, 2022
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

No branches or pull requests

4 participants