Skip to content
This repository was archived by the owner on Nov 10, 2025. It is now read-only.

add invoke contract list#656

Closed
Ashuaidehao wants to merge 2 commits intoneo-project:masterfrom
Ashuaidehao:add-callstack
Closed

add invoke contract list#656
Ashuaidehao wants to merge 2 commits intoneo-project:masterfrom
Ashuaidehao:add-callstack

Conversation

@Ashuaidehao
Copy link
Contributor

add invoke details.


JObject json = new();
json["script"] = Convert.ToBase64String(script);
json["invokedcontracts"] = new JArray(contracts.Select(c => (JString)c.ToString()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need another RPC method for this? Can't we always return it from invokescript and invokefunction?

While here, it'd also be useful to return:

  • logs emitted during execution
  • storage changes (probably in the same format StatesDumper uses)

This can help debugging and evaluating transaction side-effects.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're adding all this information, doesn't it make sense to also make it a new RPC method? Most of the current usage of InvokeFunction/Script won't make use of this info and yet would still need to transfer and parse more data

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, this was on my list for a long time, it's just that I've not managed to file an issue. To me it's about capturing and presenting all side-effects and important aspects of execution (like contracts called) properly. All of this is important to understand what exactly happens when something is executed and it can affect some user's (or backend, if transaction is created/signed automatically) decisions.

So I'd rather have simple invokefunction/invokescript pair that always returns everything needed (and server-side we have this data anyway, it's just that we don't give it to user).

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need another RPC method for this? Can't we always return it from invokescript and invokefunction?

I agree with @roman-khimov , even if you need to separate the detail from the simple result, you can add a parameter for that instead of adding an extra interface.....

Copy link
Member

@shargon shargon Oct 16, 2021

Choose a reason for hiding this comment

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

Agree, add details argument is enough

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with adding a details argument to control inclusion of additional execution data instead of making new RPC methods.

return json;
}

private static Block CreateDummyBlock(DataCache snapshot, ProtocolSettings settings)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is already a CreateDummyBlock private method on ApplicationEngine. I needed one in the debugger and was told to copy/paste the code. Now here's a 3rd copy of the same function? Can't we simply make the one on ApplicationEngine public?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is already a CreateDummyBlock private method on ApplicationEngine. I needed one in the debugger and was told to copy/paste the code. Now here's a 3rd copy of the same function? Can't we simply make the one on ApplicationEngine public?

@erikzhang what do you think?

@erikzhang
Copy link
Member

Please check neo-project/neo#2613

@superboyiii
Copy link
Member

superboyiii commented Oct 19, 2021

Please check neo-project/neo#2613

So we'd better close this and create a new one, right?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants