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

Add readOnly to NEP-3 #104

Closed
wants to merge 4 commits into from
Closed

Add readOnly to NEP-3 #104

wants to merge 4 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented Sep 9, 2019

Camelcase for json property names

Camelcase for json property names
nep-3.mediawiki Outdated Show resolved Hide resolved
nep-3.mediawiki Outdated Show resolved Hide resolved
nep-3.mediawiki Outdated Show resolved Hide resolved
@erikzhang
Copy link
Member

Can we discuss it again, why do we need readOnly? What is its use?

@shargon
Copy link
Member Author

shargon commented Sep 10, 2019

neo-project/neo#927

Is for ensure that is not possible to execute a write operation in these methods, in order to prevent the write access

@erikzhang
Copy link
Member

Why to prevent the write access?

@shargon
Copy link
Member Author

shargon commented Sep 10, 2019

As @igormcoelho said:

offchain services execute operations by themselves (unless forced to send to chain); and two, a readonly storage context is provided

Before this change, the developer promise to not modify the state if the method is safe, after this change is not a promise, is a must.

Also, the Abi will be compleated by the compiler acording to the Attributes, and developers doesn't need to create/modify the Abi.

@erikzhang
Copy link
Member

Can we remove readOnly from ABI, and extend the syscall System.Contract.Call, add a new parameter readOnly to it?

@shargon
Copy link
Member Author

shargon commented Sep 10, 2019

But then, how the wallet know when they need to execute the context as readOnly?

@erikzhang
Copy link
Member

Why a wallet need to execute the context as readOnly?

@shargon
Copy link
Member Author

shargon commented Sep 10, 2019

Imagine that you want to create a website like etherscan.io, and allow to execute readOnly methods (without relay), you need to know what methods are readOnly and what methods don't.

https://etherscan.io/address/0x01eacc3ae59ee7fbbc191d63e8e1ccfdac11628c#readContract

@erikzhang
Copy link
Member

I think readOnly and relay are irrelevant.

@shargon
Copy link
Member Author

shargon commented Sep 10, 2019

How do you do this website without know which methods are readOnly and which methods not?

@erikzhang
Copy link
Member

I don't know. Why people need to know it? If I make this website, I will provide a uniform list of methods without distinguishing between read-only or not.

@lock9
Copy link

lock9 commented Sep 10, 2019

🤔 🤔 🤔
Lol, you are right @erikzhang , this change makes no sense 😅 . I don't think people would even send TX to the blockchain that does not alter the storage. And even if they did, there is no difference, because making a method read-only won't remove the dependency of the storage.

Maybe we need a "no storage" flag instead of a readOnly. A method that has no access to the storage (stateless) can be used in optimizations since these are certain to be "thread safe".

@shargon
Copy link
Member Author

shargon commented Sep 10, 2019

Maybe we need a "no storage" flag instead of a readOnly. A method that has no access to the storage (stateless) can be used in optimizations since these are certain to be "thread safe".

This is precisely for this, allow to create methods that ensure that no changes will be made on the blockchain, has the same behavior of view in ETH

@lock9
Copy link

lock9 commented Sep 10, 2019

@shargon but read-only still depends on the state, even if it is read-only, we can have different results if the content of the storage is changed. This is not true for real stateless methods

@shargon
Copy link
Member Author

shargon commented Sep 10, 2019

maybe we need three states instead of two

storage=stateLess, writable, readOnly

But is very important this information in the abi.

@lock9
Copy link

lock9 commented Sep 10, 2019

Why do we need the readonly? I think we only need stateful and stateless methods. And I believe all methods should be either of them, always. If nothing is said, it is stateful. What do you think @shargon ? I can't see the gains of only having "read only". Can you point it to me?

@erikzhang
Copy link
Member

I think we only need stateful and stateless methods. And I believe all methods should be either of them, always.

I think even the same method can have different state dependencies on each execution.

if (needToWrite)
{
    //Write something to storage
}
else
{
    //return constant
}

So we should specify an option each time we call a method, which can include an agreement on whether the called method has permission to access the state.

@shargon
Copy link
Member Author

shargon commented Sep 11, 2019

For me is the same case as view in ethereum, but they don't do any check at runtime, is only for the abi

https://solidity.readthedocs.io/en/latest/contracts.html#view-functions

"This means library view functions do not have run-time checks that prevent state modifications"

@shargon
Copy link
Member Author

shargon commented Nov 18, 2019

Any news? this should be closed or updated?

@corollari
Copy link
Contributor

Another reason that makes this interesting is that it allows libraries that interact with the blockchain, such as neon-js, to detect which calls require actual transactions and which can be done using a simple RPC call to any node, while providing a unified API.

Example:

contract.getName() // -> RPC call
contract.setName("hey") // -> transaction

@erikzhang
Copy link
Member

erikzhang commented Nov 21, 2019

@corollari

https://github.com/neo-project/neo/blob/24da2bd8df2612643b3a4840cca2bea77b305daf/neo/SmartContract/Manifest/ContractManifest.cs#L59

The readonly is in manifest, and it is named as safemethods. So it is easy to be checked.

This proposal is to move it from manifest to abi. It is a different thing.

@corollari
Copy link
Contributor

I see, you are right in that my comment is invalid then.

@lock9
Copy link

lock9 commented Nov 25, 2019

I vote to move this to the ABI, we can use an attribute at smart contract level. As @corollari mentioned, this is helpful to know when we can use RPC directly and it may also be useful for optimizations. Also, the ABI is an adequate place for it because it is where we store information about the methods in our contract.

@shargon shargon closed this Dec 19, 2019
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.

4 participants