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

Improve CalculateNetworkFee for contract signers #2805

Closed
roman-khimov opened this issue Aug 18, 2022 · 3 comments · Fixed by #3385
Closed

Improve CalculateNetworkFee for contract signers #2805

roman-khimov opened this issue Aug 18, 2022 · 3 comments · Fixed by #3385
Assignees
Labels
Discussion Initial issue state - proposed but not yet accepted P1 High Priority: Targeting The Next Release
Milestone

Comments

@roman-khimov
Copy link
Contributor

Summary or problem description
CalculateNetworkFee as it is now executes verify methods for contract-based witnesses, but it can lead to wrong results if verify has parameters that can't be provided at this stage (like a transaction signature). Such methods will just fail leading to inability to use CalculateNetworkFee for what it's intended to do. Technically it's still the same issue as #840 (reminding of #1763 (comment) as well), but while it can't be solved for the generic case we can make some improvements for deployed contracts specifically.

Do you have any solution you want to propose?

  1. Ignore verify execution results. We're calculating fees here, so witnesses can fail (take standard signature contracts for example, they're not even executed, we know they'll fail) and it's OK. But we'll still have some (maybe not perfect, but still) data for the GAS amount needed for execution (and contracts can use Runtime.BurnGas to compensate for any differences with the happy path).
  2. If no invocation script is provided, try to determine its size from the parameters (and even create some stub for it). It can't be perfect also, but NEP-14 is helpful in many cases, we know the size of Boolean, Hash160, Hash256, PublicKey and Signature quite well. Integer is quite predictable in its upper limit too. Other types not so much, but I'd expect them to be used a little less often than hashes or signatures.

This won't solve all problems, but I think this way we'll cover a bit more cases which can be helpful for dApps.

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • RPC
  • Wallet
@roman-khimov roman-khimov added the Discussion Initial issue state - proposed but not yet accepted label Aug 18, 2022
@roman-khimov
Copy link
Contributor Author

Even more provocative, we can improve this a bit:

else
{
//We can support more contract types in the future.
}

by executing whatever is given in the invocation/verification scripts. Of course this will fail as well, but in many cases the GAS spent will be correct and if there is a stub invocation script provided by the user then the size can be calculated correctly.

Again, not perfect, but something that will cover a bit more use cases.

@roman-khimov
Copy link
Contributor Author

Implemented in NeoGo 0.99.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted P1 High Priority: Targeting The Next Release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants