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

Modify fee of System.Runtime.GetNotifications #2748

Merged
merged 3 commits into from
May 23, 2022

Conversation

erikzhang
Copy link
Member

@erikzhang erikzhang commented May 21, 2022

@Jim8y
Copy link
Contributor

Jim8y commented May 21, 2022

Really happy to see we finally have a benchmark project. it would be better if we could use [BenchmarkDotNet](https://benchmarkdotnet.org/Overview.htm)

@erikzhang
Copy link
Member Author

it would be better if we could use BenchmarkDotNet

If the project become more complex in the future, we can consider it. Currently I don't think it's needed much.

@roman-khimov
Copy link
Contributor

Maybe we should charge per-element here? Sending notifications is naturally charged per-element, maybe getting them should also be, it'd be more fair.

@erikzhang
Copy link
Member Author

The problem with per-element charges is that when you create a transaction it is impossible to know how many notifications the contract will receive when it executes.

@roman-khimov
Copy link
Contributor

Transaction scripts are test-executed usually and I don't think this problem is any different from any other test/real invocation GAS/state difference that can happen in lots of other scenarios.

@erikzhang
Copy link
Member Author

test/real invocation GAS/state difference

We should try to avoid this situation.

@roman-khimov
Copy link
Contributor

roman-khimov commented May 21, 2022

test/real invocation GAS/state difference

We should try to avoid this situation.

Sure, but here it will depend on the number of emitted notifications. To me it's not very likely to change unless the expected transaction outcome changes significantly anyway (like some transfer or asset mint fails). Even if it is to change, it's more likely that some notifications will be omitted rather than created (which costs additional GAS already).

@erikzhang
Copy link
Member Author

@roman-khimov What about neo-project/neo-vm#473?

@roman-khimov
Copy link
Contributor

@roman-khimov What about neo-project/neo-vm#473?

Unexpected. What's the execution time for #2725 with this? In some ways it feels like this problem shouldn't require changing the VM, but at the same time it can be effective and not very restrictive (it's hard to imagine normal contract trying to reuse (referenced) event items after notification).

@erikzhang
Copy link
Member Author

Immutability can be very useful, not only in the notification's scenario, but also for calling between contracts. We can open this function to contracts in the future, such as adding a new OpCode.ASIMMUTABLE.

@erikzhang
Copy link
Member Author

Merge?

@erikzhang erikzhang merged commit eaa434d into master May 23, 2022
@erikzhang erikzhang deleted the modify-fee-of-getnotifications branch May 23, 2022 23:17
roman-khimov added a commit to nspcc-dev/neo-go that referenced this pull request May 24, 2022
ixje added a commit to CityOfZion/neon-js that referenced this pull request Jun 29, 2022
ixje added a commit to CityOfZion/neo-mamba that referenced this pull request Jul 1, 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

Successfully merging this pull request may close these issues.

GAS price for GetNotifications is cheap
5 participants