Remove sender from invoke* RPCs #351

roman-khimov · 2020-09-22T14:47:06Z

Summary or problem description
#309 had introduced a sender parameter for invokescript and invokefunction RPCs, supposedly to solve the problem of null here:

tx = wallet.MakeTransaction(result["script"].AsString().HexToBytes(), null, witnessCosigners);

But it's not really needed as the sender is the first signer and signers are perfectly ordered to rely on this. At the moment having both an explicit sender and signers creates some duplication along with additional error modes that we need to check for. I don't see anything we couldn't do without an explicit sender (just with signers).

Do you have any solution you want to propose?
Something like this should solve the problem:

tx = wallet.MakeTransaction(result["script"].AsString().HexToBytes(), witnessSigners[0], witnessSigners);

And allow to simplify the RPC API (returning to the parameter set we had for preview3).

Where in the software does this update applies to?

RPC (HTTP)

The text was updated successfully, but these errors were encountered:

ixje · 2020-10-02T13:42:30Z

Agreed. neo-project/neo#1752 fixed the sender to the first signer.

Tommo-L · 2020-10-09T09:09:59Z

I think it's clear enough.

neo-modules/src/RpcServer/RpcServer.Wallet.cs

Line 168 in fed3f7a

 private void ProcessInvokeWithWallet(JObject result, UInt160 sender = null, Signers signers = null) 

ixje · 2020-10-09T09:19:31Z

I don't understand your comment?

The discussion is about removing the mandatory sender param for the invokescript and invokefunction RPC calls when you want to add signers, because signer[0] is already always the sender.

neo-modules/src/RpcServer/RpcServer.SmartContract.cs

Lines 120 to 121 in fed3f7a

 UInt160 sender = _params.Count >= 2 ? AddressToScriptHash(_params[1].AsString()) : null; 

 Signers signers = _params.Count >= 3 ? SignersFromJson((JArray)_params[2]) : null;

Tommo-L · 2020-10-09T09:41:30Z

You are right, we should remove it. Plz see #368

superboyiii · 2020-10-10T04:15:56Z

Summary or problem description
#309 had introduced a sender parameter for invokescript and invokefunction RPCs, supposedly to solve the problem of null here:
tx = wallet.MakeTransaction(result["script"].AsString().HexToBytes(), null, witnessCosigners);
But it's not really needed as the sender is the first signer and signers are perfectly ordered to rely on this. At the moment having both an explicit sender and signers creates some duplication along with additional error modes that we need to check for. I don't see anything we couldn't do without an explicit sender (just with signers).

Do you have any solution you want to propose?
Something like this should solve the problem:
tx = wallet.MakeTransaction(result["script"].AsString().HexToBytes(), witnessSigners[0], witnessSigners);
And allow to simplify the RPC API (returning to the parameter set we had for preview3).

Where in the software does this update applies to?

RPC (HTTP)

Why Signers[0] should be the sender? Signers[1], Signers[2] could also be sender, that's why we have to confirm the sender in params. A wallet could contain multi addresses.
For invokefunction, you could see neo-cli is like this: https://github.com/neo-project/neo-node/blob/184db447499e82f4aae57ae686800f9a434618a3/neo-cli/CLI/MainService.Contracts.cs#L55

ixje · 2020-10-10T04:52:52Z

No sender is fixed. See #351 (comment)

…

Why Signers[0] should be the sender? Signers[1], Signers[2] could also be sender, that's why we have to confirm the sender in params — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

superboyiii · 2020-10-10T05:06:37Z

No sender is fixed. See #351 (comment)
…
Why Signers[0] should be the sender? Signers[1], Signers[2] could also be sender, that's why we have to confirm the sender in params — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

Thanks for remind. But it doesn't effect the behaviour of invokefunction. Let me show you an example:

neo> open wallet admin2.json
password: *
neo> list address
   Address: NdP4PHSUtLt1TfhgoCcGwuo54SBuQ3xrvX  Standard
ScriptHash: 0xfb0f98b8cc1124c6a275904f41cc632881b599bf

   Address: NLtDqwnj9s7wQVyaiD5ohjV3e9fUVkZxDp  Standard
ScriptHash: 0xf642a0401fdd56a03114639a98b7963eb587a30a

neo> invoke 0x254b9decd76080ef368e7a6b0a065938dfbc31cf totalSupply [] NLtDqwnj9s7wQVyaiD5ohjV3e9fUVkZxDp NdP4PHSUtLt1TfhgoCcGwuo54SBuQ3xrvX NLtDqwnj9s7wQVyaiD5ohjV3e9fUVkZxDp
Invoking script with: '10c00c0b746f74616c537570706c790c14cf31bcdf3859060a6b7a8e36ef8060d7ec9d4b2541627d5b52'
VM State: HALT
Gas Consumed: 0.0373458
Result Stack: [{"type":"Integer","value":"2000000000000000"}]
Relay tx(no|yes): y
Signed and relayed transaction with hash=0x4af6e4b0fb7be0d3e161adc1d925c3ae24f1462fa1ad3b6479d0529cecde7043

You see in neo-cli, I could still make the second address as sender.

{
    "jsonrpc": "2.0",
    "id": 1,
    "result": {
        "hash": "0x4af6e4b0fb7be0d3e161adc1d925c3ae24f1462fa1ad3b6479d0529cecde7043",
        "size": 331,
        "version": 0,
        "nonce": 174345424,
        "sender": "NLtDqwnj9s7wQVyaiD5ohjV3e9fUVkZxDp",
        "sysfee": "3734580",
        "netfee": "2331780",
        "validuntilblock": 2102676,
        "signers": [
            {
                "account": "0xf642a0401fdd56a03114639a98b7963eb587a30a",
                "scopes": "CalledByEntry"
            },
            {
                "account": "0xfb0f98b8cc1124c6a275904f41cc632881b599bf",
                "scopes": "CalledByEntry"
            }
        ],
        "attributes": [],
        "script": "EMAMC3RvdGFsU3VwcGx5DBTPMbzfOFkGCmt6jjbvgGDX7J1LJUFifVtS",
        "witnesses": [
            {
                "invocation": "DEA7nX3X9ydq1dyhHTGPHAtq3u79rOzKqgMF2fL+gHG0QJHcoVR6jG4Boqv9iGViUc1gAmrAXriA1JKarjIihw7A",
                "verification": "DCEDzqPi+B8a+TUi0p7eTySh8L7erXKTOR0ziA9Uddl4eMkLQZVEDXg="
            },
            {
                "invocation": "DEAKA6m4x7IDZ6sMwEN2vgNoSS3miC3jpUt44llTAwuSiynbkdclr2nLDwY4OiLb915ZkBRrdGxQVj9QKsnLUJtX",
                "verification": "DCECstsz77Nf/mNnZOXm0P7O4Qxl5OKQRvE2VKUmxnBH2E4LQZVEDXg="
            }
        ],
        "blockhash": "0xb0527d4908218d59e97742ed25cd84ad499441b5882e39841def0a07c29dbaf2",
        "confirmations": 2,
        "blocktime": 1602306989640,
        "vmstate": "HALT"
    }
}

I think invokefunction in RPC should be the same mechanism.

roman-khimov · 2020-10-10T05:32:08Z

You see in neo-cli, I could still make the second address as sender.

I doubt it's useful, you can use the same convention here and always get the sender for the first signer.

I think invokefunction in RPC should be the same mechanism.

But even if you find it useful, you can hide it completely in the client (neo-cli here) and use regular encoding scheme with signers[0] == sender for the RPC call. An explicit sender parameter for RPC call is just redundant data and the need to do additional error checking that could easily be avoided.

superboyiii · 2020-10-10T06:34:07Z

You see in neo-cli, I could still make the second address as sender.

I doubt it's useful, you can use the same convention here and always get the sender for the first signer.

I think invokefunction in RPC should be the same mechanism.

But even if you find it useful, you can hide it completely in the client (neo-cli here) and use regular encoding scheme with signers[0] == sender for the RPC call. An explicit sender parameter for RPC call is just redundant data and the need to do additional error checking that could easily be avoided.

Please have a look at my test result.
#368 (comment)

I think the meaning of sender is to show who makes the transaction. And signer is who pays for the transaction. Like we buy something online, we could make this order and ask our friend or families to pay for it. if you'd like to make it as who paid it, then he is the sender, it's OK. But we should pay attention to the consistency of input params and result when invoking RPC.

shargon · 2020-10-10T06:37:24Z

sender is who pay the fee, signers is who sign that this transaction it's ok. because sender must be a signer, we decided to use the first entry for it.

superboyiii · 2020-10-10T06:51:26Z

sender is who pay the fee, signers is who sign that this transaction it's ok. because sender must be a signer, we decided to use the first entry for it.

If designed like this ,then we should modify here, because what we input is not what we get. I use a non-asset address to be the signer, it doesn't return insufficient balance but automatically choose another address as signer which will make people confused.

{
  "jsonrpc": "2.0",
  "method": "invokefunction",
  "params": [
    "0x254b9decd76080ef368e7a6b0a065938dfbc31cf",
    "balanceOf",
    [
      {
        "type": "Hash160",
        "value": "0xf642a0401fdd56a03114639a98b7963eb587a30a"
      }
    ],
    [
      {
        "account": "0xef9b39f97b13be48a48ee1bbdd4c9b35024778ca",
        "scopes": "CalledByEntry"
      }
    ]
  ],
  "id": 3
}

{
    "jsonrpc": "2.0",
    "id": 3,
    "result": {
        "script": "0c140aa387b53e96b7989a631431a056dd1f40a042f611c00c0962616c616e63654f660c14cf31bcdf3859060a6b7a8e36ef8060d7ec9d4b2541627d5b52",
        "state": "HALT",
        "gasconsumed": "3840690",
        "stack": [
            {
                "type": "Integer",
                "value": "2000000000000000"
            }
        ],
        "tx": "001e3fa04eb29a3a0000000000a4e2230000000000ea162000020aa387b53e96b7989a631431a056dd1f40a042f600ca784702359b4cddbbe18ea448be137bf9399bef01003e0c140aa387b53e96b7989a631431a056dd1f40a042f611c00c0962616c616e63654f660c14cf31bcdf3859060a6b7a8e36ef8060d7ec9d4b2541627d5b5202420c40f01f3b9890929d86af6ee3aa0bf8089e5097d9a9acc506067e546aa3ccbca61508122c44f8e6a4e92974e21fde3f92e5c60555b5b050e7fab5a63ce99cbe00bc290c2103cea3e2f81f1af93522d29ede4f24a1f0bedead7293391d33880f5475d97878c90b4195440d78420c40df98a383c5858f11826f7edb765881ddc9d8c0583aef9221368f88a56308c21250f0bb4dfacb5012b9bd87dd8270fadfc474b86d4dd5ec308d15c11c2c0c5429290c21029e86bfd7666b1e11105ed7f427758b4bcde8db6d5e7c8eb3aea1316abfcd6b3a0b4195440d78"
    }
}

{
  "jsonrpc": "2.0",
  "method": "sendrawtransaction",
  "params": ["001e3fa04eb29a3a0000000000a4e2230000000000ea162000020aa387b53e96b7989a631431a056dd1f40a042f600ca784702359b4cddbbe18ea448be137bf9399bef01003e0c140aa387b53e96b7989a631431a056dd1f40a042f611c00c0962616c616e63654f660c14cf31bcdf3859060a6b7a8e36ef8060d7ec9d4b2541627d5b5202420c40f01f3b9890929d86af6ee3aa0bf8089e5097d9a9acc506067e546aa3ccbca61508122c44f8e6a4e92974e21fde3f92e5c60555b5b050e7fab5a63ce99cbe00bc290c2103cea3e2f81f1af93522d29ede4f24a1f0bedead7293391d33880f5475d97878c90b4195440d78420c40df98a383c5858f11826f7edb765881ddc9d8c0583aef9221368f88a56308c21250f0bb4dfacb5012b9bd87dd8270fadfc474b86d4dd5ec308d15c11c2c0c5429290c21029e86bfd7666b1e11105ed7f427758b4bcde8db6d5e7c8eb3aea1316abfcd6b3a0b4195440d78"],
  "id": 1
}

{
    "jsonrpc": "2.0",
    "id": 1,
    "result": {
        "hash": "0x7b3c8ae059ba7cd18d98cedcf8bb3c455ca4668921500274ea4e600be00bdc29"
    }
}

{
  "jsonrpc": "2.0",
  "method": "getrawtransaction",
  "params": ["0x7b3c8ae059ba7cd18d98cedcf8bb3c455ca4668921500274ea4e600be00bdc29", 1],
  "id": 1
}

{
    "jsonrpc": "2.0",
    "id": 1,
    "result": {
        "hash": "0x7b3c8ae059ba7cd18d98cedcf8bb3c455ca4668921500274ea4e600be00bdc29",
        "size": 351,
        "version": 0,
        "nonce": 1319124766,
        "sender": "NLtDqwnj9s7wQVyaiD5ohjV3e9fUVkZxDp",
        "sysfee": "3840690",
        "netfee": "2351780",
        "validuntilblock": 2103018,
        "signers": [
            {
                "account": "0xf642a0401fdd56a03114639a98b7963eb587a30a",
                "scopes": "None"
            },
            {
                "account": "0xef9b39f97b13be48a48ee1bbdd4c9b35024778ca",
                "scopes": "CalledByEntry"
            }
        ],
        "attributes": [],
        "script": "DBQKo4e1Ppa3mJpjFDGgVt0fQKBC9hHADAliYWxhbmNlT2YMFM8xvN84WQYKa3qONu+AYNfsnUslQWJ9W1I=",
        "witnesses": [
            {
                "invocation": "DEDwHzuYkJKdhq9u46oL+AieUJfZqazFBgZ+VGqjzLymFQgSLET45qTpKXTiH94/kuXGBVW1sFDn+rWmPOmcvgC8",
                "verification": "DCEDzqPi+B8a+TUi0p7eTySh8L7erXKTOR0ziA9Uddl4eMkLQZVEDXg="
            },
            {
                "invocation": "DEDfmKODxYWPEYJvftt2WIHdydjAWDrvkiE2j4ilYwjCElDwu036y1ASub2H3YJw+t/EdLhtTdXsMI0VwRwsDFQp",
                "verification": "DCECnoa/12ZrHhEQXtf0J3WLS83o221efI6zrqExar/NazoLQZVEDXg="
            }
        ],
        "blockhash": "0x79e4f4084102e63dcb39153114137312660a556689080596f6a931ed04ab45c6",
        "confirmations": 3,
        "blocktime": 1602312595703,
        "vmstate": "HALT"
    }
}

roman-khimov · 2020-10-12T07:52:40Z

I use a non-asset address to be the signer, it doesn't return insufficient balance but automatically choose another address as signer which will make people confused.

I guess the server just shouldn't do that. In you example you only specify one signer for invokefunction:

    [
      {
        "account": "0xef9b39f97b13be48a48ee1bbdd4c9b35024778ca",
        "scopes": "CalledByEntry"
      }
    ]

Which to me means that he also is a sender, so the fact that you get this in returned transaction:

        "signers": [
            {
                "account": "0xf642a0401fdd56a03114639a98b7963eb587a30a",
                "scopes": "None"
            },
            {
                "account": "0xef9b39f97b13be48a48ee1bbdd4c9b35024778ca",
                "scopes": "CalledByEntry"
            }
        ],

is a bug to me. Instead it should be:

        "signers": [
            {
                "account": "0xef9b39f97b13be48a48ee1bbdd4c9b35024778ca",
                "scopes": "CalledByEntry"
            }
        ],

Or no transaction returned at all as it's not possible to create a valid transaction for a given sender.

This logic for automatically choosing the sender is actually a bit dangerous and can lead to unexpected results when you have multiple mixed zero-balance and non-zero-balance accounts in a wallet.

superboyiii · 2020-10-12T09:24:44Z

Or no transaction returned at all as it's not possible to create a valid transaction for a given sender.

This logic for automatically choosing the sender is actually a bit dangerous and can lead to unexpected results when you have multiple mixed zero-balance and non-zero-balance accounts in a wallet.

Yes, that's the point. We should return insufficient balance in MakeTransaction instead of automatically choosing the sender. If wallet.cs is modified like this, then #368 makes sense.

roman-khimov added the discussion label Sep 22, 2020

roman-khimov mentioned this issue Sep 29, 2020

Preview4 Checklist neo-project/neo#1936

Closed

44 tasks

Tommo-L mentioned this issue Oct 9, 2020

Remove sender from invoke method #368

Merged

superboyiii closed this as completed Oct 10, 2020

superboyiii reopened this Oct 10, 2020

superboyiii mentioned this issue Oct 12, 2020

Non-asset signer in invokefunction should not be able to create tx automatically and should not be in the list of signers. #370

Closed

shargon closed this as completed in #368 Oct 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove sender from invoke* RPCs #351

Remove sender from invoke* RPCs #351

roman-khimov commented Sep 22, 2020

ixje commented Oct 2, 2020

Tommo-L commented Oct 9, 2020

ixje commented Oct 9, 2020

Tommo-L commented Oct 9, 2020

superboyiii commented Oct 10, 2020 •

edited

Loading

ixje commented Oct 10, 2020 via email

superboyiii commented Oct 10, 2020 •

edited

Loading

roman-khimov commented Oct 10, 2020 •

edited

Loading

superboyiii commented Oct 10, 2020 •

edited

Loading

shargon commented Oct 10, 2020

superboyiii commented Oct 10, 2020 •

edited

Loading

roman-khimov commented Oct 12, 2020

superboyiii commented Oct 12, 2020 •

edited

Loading

Remove sender from invoke* RPCs #351

Remove sender from invoke* RPCs #351

Comments

roman-khimov commented Sep 22, 2020

ixje commented Oct 2, 2020

Tommo-L commented Oct 9, 2020

ixje commented Oct 9, 2020

Tommo-L commented Oct 9, 2020

superboyiii commented Oct 10, 2020 • edited Loading

ixje commented Oct 10, 2020 via email

superboyiii commented Oct 10, 2020 • edited Loading

roman-khimov commented Oct 10, 2020 • edited Loading

superboyiii commented Oct 10, 2020 • edited Loading

shargon commented Oct 10, 2020

superboyiii commented Oct 10, 2020 • edited Loading

roman-khimov commented Oct 12, 2020

superboyiii commented Oct 12, 2020 • edited Loading

superboyiii commented Oct 10, 2020 •

edited

Loading

superboyiii commented Oct 10, 2020 •

edited

Loading

roman-khimov commented Oct 10, 2020 •

edited

Loading

superboyiii commented Oct 10, 2020 •

edited

Loading

superboyiii commented Oct 10, 2020 •

edited

Loading

superboyiii commented Oct 12, 2020 •

edited

Loading