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

CheckWitness on invokescript (2.x) #1418

Merged
merged 4 commits into from
Feb 21, 2020

Conversation

meevee98
Copy link
Contributor

Port of #1122 to 2.x

@melanke
Copy link

melanke commented Jan 15, 2020

I made some quick review with meevee98, FYI the relevant changes are on RpcServer file, the other changes are only to prettify the code

@superboyiii
Copy link
Member

Here's my test result:
I deployed an Nep5 contract, and set Checkwitness() in Transfer(). It works well on my neo-cli(command line) & RPC(sendfrom, sendtoaddress). Then I take out the script of tx into Invokescript and input my address scripthash.
image

{
  "jsonrpc": "2.0",
  "method": "invokescript",
  "params": ["0500e876481714ef29abc9496c14a9e93ea0d284bf712a197a849a14498c884f128f1c9ae705ff9b20ab17e6cb539c4c53c1087472616e7366657267a9db10bce155f113746be9f079f0906c64754c64f166", "4c9c53cbe617ab209bff05e79a1c8f124f888c49"],
  "id": 3
}

I find checkWitnessHashes hasn't been transferred into GetInvokeResult().
image
image
image
Test result: [Fail]
@meevee98 You must modify your code.

@superboyiii
Copy link
Member

superboyiii commented Jan 21, 2020

You should make it like this:

                case "invoke":
                    {
                        UInt160 script_hash = UInt160.Parse(_params[0].AsString());
                        CheckWitnessHashes checkWitnessHashes = null;
                        ContractParameter[] parameters = ((JArray)_params[1]).Select(p => ContractParameter.FromJson(p)).ToArray();
                        if (_params.Count > 2)
                        {
                            UInt160[] scriptHashesForVerifying = _params.Skip(2).Select(u => UInt160.Parse(u.AsString())).ToArray();
                            checkWitnessHashes = new CheckWitnessHashes(scriptHashesForVerifying);
                        }
                        return Invoke(script_hash, parameters, checkWitnessHashes);
                    }
                case "invokefunction":
                    {
                        UInt160 script_hash = UInt160.Parse(_params[0].AsString());
                        string operation = _params[1].AsString();
                        ContractParameter[] args = _params.Count >= 3 ? ((JArray)_params[2]).Select(p => ContractParameter.FromJson(p)).ToArray() : new ContractParameter[0];
                        CheckWitnessHashes checkWitnessHashes = null;
                        if (_params.Count > 3)
                        {
                            UInt160[] scriptHashesForVerifying = _params.Skip(3).Select(u => UInt160.Parse(u.AsString())).ToArray();
                            checkWitnessHashes = new CheckWitnessHashes(scriptHashesForVerifying);
                        }
                        return InvokeFunction(script_hash, operation, args, checkWitnessHashes);
                    }
                case "invokescript":
                    {
                        byte[] script = _params[0].AsString().HexToBytes();
                        CheckWitnessHashes checkWitnessHashes = null;
                        if (_params.Count > 1)
                        {
                            UInt160[] scriptHashesForVerifying = _params.Skip(1).Select(u => UInt160.Parse(u.AsString())).ToArray();
                            checkWitnessHashes = new CheckWitnessHashes(scriptHashesForVerifying);
                        }
                        return InvokeScript(script, checkWitnessHashes);
                    }
        private JObject Invoke(UInt160 script_hash, ContractParameter[] parameters, CheckWitnessHashes checkWitness)
        {
            byte[] script;
            using (ScriptBuilder sb = new ScriptBuilder())
            {
                script = sb.EmitAppCall(script_hash, parameters).ToArray();
            }
            return GetInvokeResult(script, checkWitness);
        }

        private JObject InvokeFunction(UInt160 script_hash, string operation, ContractParameter[] args, CheckWitnessHashes checkWitness)
        {
            byte[] script;
            using (ScriptBuilder sb = new ScriptBuilder())
            {
                script = sb.EmitAppCall(script_hash, operation, args).ToArray();
            }
            return GetInvokeResult(script, checkWitness);
        }

        private JObject InvokeScript(byte[] script, CheckWitnessHashes checkWitness)
        {
            return GetInvokeResult(script, checkWitness);
        }

@meevee98
Copy link
Contributor Author

I've updated the code
@superboyiii could you test it again?

@devhawk
Copy link
Contributor

devhawk commented Jan 28, 2020

In the future, can we avoid making unrelated formatting improvements in PRs like this? As far as I can tell, only one of the 26 files updated by this PR is related to the behaviour change. The other 25 appear to be whitespace formatting updates. That makes it much harder to review the PR.

shargon
shargon previously approved these changes Jan 30, 2020
superboyiii
superboyiii previously approved these changes Feb 2, 2020
Copy link
Member

@superboyiii superboyiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@meevee98

  1. Made witness included tx, take its script as a param of invokescript and input correct and incorrect bigend scripthash of address for check. Correct address returned HALT and incorrect address returned FAULT. [Pass]
  2. Made bigend scripthash of address as the latest param into invokefunction and invoke, execute deploy and transfer methods of SC, no negative effect appeared. [Pass]
    Test Result: PASS

@meevee98
Copy link
Contributor Author

I've rebased this branch into the latest changes of master-2.x.
Could anyone review it, please?

@shargon
Copy link
Member

shargon commented Feb 18, 2020

Because in 2x there are no travis anymore, could you remove the format changes in order to make the review easier?

@meevee98
Copy link
Contributor Author

Sure, no problem

@superboyiii
Copy link
Member

@meevee98 @shargon Compared the code with my last test version, no difference here. It's OK.
image

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

Successfully merging this pull request may close these issues.

6 participants