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

neo3 ABI readonly methods #927

Closed
igormcoelho opened this issue Jul 16, 2019 · 21 comments
Closed

neo3 ABI readonly methods #927

igormcoelho opened this issue Jul 16, 2019 · 21 comments
Assignees
Labels
blocked This issue can't be worked at the moment design Issue state - Feature accepted but the solution requires a design before being implemented feature Type: Large changes or new features ledger Module - The ledger is our 'database', this is used to tag changes about how we store information

Comments

@igormcoelho
Copy link
Contributor

Neo3 should provide readonly identifier on manifest. When these functions are demanded, two things happen: offchain services execute operations by themselves (unless forced to send to chain); and two, a readonly storage context is provided. This way, a readonly invoked method will.not be able to invoke a read write method.

@lock9 lock9 added the discussion Initial issue state - proposed but not yet accepted label Jul 17, 2019
@shargon
Copy link
Member

shargon commented Jul 17, 2019

in order to implement it, could you especify what methods?

@erikzhang erikzhang added this to the NEO 3.0 milestone Jul 17, 2019
@igormcoelho
Copy link
Contributor Author

igormcoelho commented Jul 17, 2019

The simplest example I have in mind is: balanceOf.

Suppose you do the appcall contractX "balanceOf" myscripthash. Neo3 would look into contractXmanifest, and notice that balanceOf has readonly flag (similar to safe flag, or is it the same?). If it's readonly, the application engine is loaded in readonly mode, and syscall GetCurrentContext would fail, for example (it would require GetReadOnlyStorageContext).

On C# devpack, it could be an attribute:

[ReadOnlyMethod]
BigInteger balanceOf(byte[] me)

What happens, from wallet side, is that it process locally this invocation instead of issuing a transaction (unless you are forced to, via sendrawtransaction). To do that efficiently, we would need the state messages from blockchain or p2p, in order to properly calculate locally the whole state, with 100% confidence. Once in readonly application mode, you can never get out of it, even using dynamic/static invokes.

@igormcoelho
Copy link
Contributor Author

@erikzhang is it exactly the same concept as safe flag? if it's the same, I then propose to rename safe to readonly.

@erikzhang
Copy link
Member

Same.

@shargon
Copy link
Member

shargon commented Jul 17, 2019

Maybe is better to call it readonly

@erikzhang
Copy link
Member

Agree. And also we can move it to ABI.

@lock9
Copy link
Contributor

lock9 commented Jul 17, 2019

@igormcoelho isn't this the same as #829 ? We can keep the discussion here if you guys prefer

@igormcoelho
Copy link
Contributor Author

No, we need to put this on NEP-3 Ammendment... another one.

@lock9
Copy link
Contributor

lock9 commented Jul 18, 2019

@igormcoelho could you describe the difference between this one and #829? Maybe we need to rename one of them because they look equal to me 😂

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Jul 18, 2019

Ok, perhaps this is why I didn't understand it at first 😂

There are many situations where you could possibly "lock" state updates.

  • The proposal here is to lock it during invocation of a Neo Standard Method (these methods with strings and parameters), which is already what Erik had proposed months ago (but by the name of safe methods). This is a lock specially related to ABI invocation of methods, and indirectly connected to NEP-3

  • At Add storage access restriction at method level #829 I saw a different perspective, for example, by locking methods when invoked from inside a smart contract. I mean, during a CALL, or an APPCALL (now System.Call). Or even during multiple transactions (I didn't get that part). That one is a very different perspective, because it affects how neovm-appengine would process everything after jumps (or calls), so I didn't understand how to do that.

In short, if both are the same, good for us: we just need to rename safe to readonly and close two issues ;)

@lock9 lock9 changed the title neo3 manifest readonly methods neo3 ABI readonly methods Jul 31, 2019
@igormcoelho igormcoelho removed their assignment Aug 1, 2019
@igormcoelho
Copy link
Contributor Author

No need to lock this feature to me. Focusing on other things first.

@lock9
Copy link
Contributor

lock9 commented Aug 5, 2019

Sorry @igormcoelho . I assigned you because you said you would like to work on this task later. I suppose someone else can do this while you are working on other tasks?

@lock9 lock9 added this to Preview1 - 16/08/2019 in NEO 3 Releases Aug 7, 2019
@lock9 lock9 added design Issue state - Feature accepted but the solution requires a design before being implemented ledger Module - The ledger is our 'database', this is used to tag changes about how we store information feature Type: Large changes or new features and removed discussion Initial issue state - proposed but not yet accepted labels Aug 10, 2019
@shargon
Copy link
Member

shargon commented Aug 12, 2019

@igormcoelho do you want to implement this? otherwise @lock9 you can assign it to me

@shargon
Copy link
Member

shargon commented Aug 13, 2019

In order to proceed with this change , we need to be able to store some information inside ExecutionContext because currently there are no way to store a flag in this class.

ExecutionContext context_new = engine.LoadScript(contract.Script, 1);

I propose to modify neo-vm for be able to extend the ExecutionContext class, this could allow to add the script hash in the future in this class too

Proposed changes : neo-project/neo-vm#191

@igormcoelho
Copy link
Contributor Author

@shargon, I'm not sure that extending ExecutionContext is a good path... but I understand why you need this.
Perhaps we just need a flag field on ExecutionContext, and reuse it, for many things.

@shargon
Copy link
Member

shargon commented Aug 13, 2019

Today is one flag, tomorrow two flags xD i don't like this model 😅

@igormcoelho
Copy link
Contributor Author

Ok, it's approved on the vm side: neo-project/neo-vm#191
We can advance on this one now, to validate those changes before merging.

@shargon
Copy link
Member

shargon commented Aug 22, 2019

Require #1048

@shargon
Copy link
Member

shargon commented Oct 18, 2019

This will be do o should be closed?

@shargon
Copy link
Member

shargon commented Nov 8, 2019

This will be merged or should be closed? @neo-project/core

@lock9 lock9 added blocked This issue can't be worked at the moment and removed neo3-preview2 labels Nov 17, 2019
@lock9
Copy link
Contributor

lock9 commented Nov 17, 2019

Hi @shargon, let's continue the discussion here: neo-project/proposals#104
We need to update NEP-3 before continuing.

@erikzhang erikzhang removed this from the NEO 3.0 milestone Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked This issue can't be worked at the moment design Issue state - Feature accepted but the solution requires a design before being implemented feature Type: Large changes or new features ledger Module - The ledger is our 'database', this is used to tag changes about how we store information
Projects
NEO 3 Releases
Preview2 - 27/09/2019
Development

Successfully merging a pull request may close this issue.

4 participants