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

OnPersist semantics for native contracts #1913

Closed
roman-khimov opened this issue Sep 7, 2020 · 3 comments
Closed

OnPersist semantics for native contracts #1913

roman-khimov opened this issue Sep 7, 2020 · 3 comments
Labels
discussion Initial issue state - proposed but not yet accepted

Comments

@roman-khimov
Copy link
Contributor

Summary or problem description
#751 has brought with it a new method for native contracts, OnPersist, and added a call to it into persisting logic. It did so with an explicit whitelist of contracts to call this method for, back then it included all existing contracts. Later (with #758) a Policy contract was added, but it wasn't included in this list as it has no actions required to do when persisting a block.

Policy contract technically still has an OnPersist method inherited from NativeContract that is visible in its manifest. And supposedly any new native contract will have it too, but at the same time it won't automatically be called when persisting a block which makes it a little unclear what was the real intention here.

Do you have any solution you want to propose?
If we want all native contracts to have OnPersist that would be called when persisting a block then this method should be called for all of them (iterating over NativeContract.Contracts probably, instead of explicit whitelist).

If we don't expect all contracts to have OnPersist, then it shouldn't be present in the NativeContract class, this way Policy won't have it in the manifest and we can actually make a decision on whether to call OnPersist for a contract or not based on its manifest (which either will have it or not).

In any event, I expect all native contracts to be treated the same way wrt this method, whitelisting shouldn't be used if it's a generic native contract feature.

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • Native contracts
@roman-khimov roman-khimov added the discussion Initial issue state - proposed but not yet accepted label Sep 7, 2020
@roman-khimov
Copy link
Contributor Author

Also relevant for postPersist, btw.

@fyrchik
Copy link
Contributor

fyrchik commented Sep 27, 2020

I am up for the first solution (calling it for all contracts). In postPersist we may also want to update some cached values for all contracts. Right now in neo-go we do this in a separate method which updates cache for Policy, Neo and Oracle contracts, but is reasonable to assume, every contract may want to do similar things after block has been persisted.

@roman-khimov
Copy link
Contributor Author

Solved by #2119.

roman-khimov added a commit to nspcc-dev/neo-go that referenced this issue Dec 13, 2020
Every contract now has these and they're always invoked. See
neo-project/neo#1913 and neo-project/neo#2119.
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
Projects
None yet
Development

No branches or pull requests

2 participants