-
Notifications
You must be signed in to change notification settings - Fork 142
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
Basic contract metadata proposal #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'll be nice to have a good example of real-life methods, corresponding pretty-printed generated JSON and metadata
method.
That would be the meaty part, cause we already discussed motivation, etc
@vgrichina updated example |
After reading this I had a few questions:
|
|
|
|
As an aside, is this issue meant to be discussing the implementation of the metadata, or the general idea of some metadata being added? As I noticed a small change to the JSON that can save a few bytes (and a different format that would reduce the size by ~60%) |
The implementation details should probably be discussed in this PR. Please note that the commit history is somewhat messed up right now because of some backlog on our end. |
@DanielRX we use JSON at many places cause it's easy to work with, especially when debugging. At some point we'll add compression and may switch to some more efficient (but semantically compatible) format like MessagePack. I wouldn't worry too much about optimizing size before compression, cause it's not really scalable – compressing all data going to be bigger win then saving on field names. |
Some UI usage: References near/near-wallet#66 (comment) |
Given the addition of the decorators for the metadata, are they enforced? |
I don't think that's possible to do given that the compiler knows nothing about state. So if you annotate a state-changing function with |
text/0000-contract-metadata.md
Outdated
This contract has two change methods, `incrementCounter` and `decrementCounter`, as well as one view method, `getCounter`. | ||
In this case, the metadata we want looks like | ||
```json | ||
[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good but kinda still seems not diverse enough. E.g. it is not clear how do we represent nested objects, arrays, strings, uint128, etc.
Maybe we need few examples to cover it doesn't have to be single contract.
text/0000-contract-metadata.md
Outdated
# Motivation | ||
[motivation]: #motivation | ||
|
||
Currently there is no direct way for a developer to programmatically list the methods of a contract that is deployed on chain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, this is not correct. If names were mangled host wouldn't be able to call the method from Wasm. The argument names might be mangled though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the names stored in a table or are they in a header file, or something else? If the names and types and offsets of all functions are programmatically available already, the metadata wouldn't need any of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I leave this research to @bowenwang1996 . I just know for a fact that names are not mangled, because host calls functions in Wasm by their names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the names are stored anywhere. Only the function names are available. Also because wasm only has i32, i64, f32, and f64, all the type information is erased.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should figure out it for a fact, for instance, find it in the Wasm spec, or ask it from someone developing Wasm or Wasm-related infra (you can ask Max).
Also because wasm only has i32, i64, f32, and f64, all the type information is erased.
I don't see how this:
Also because wasm only has i32, i64, f32, and f64
implies this:
all the type information is erased.
I suggest we do not do own our deduction, and either find it in the Wasm spec or talk to the Wasm team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on this link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, did you look into how Wasm preserves the names of the functions? I am pretty sure they are not mangled, otherwise we would not be able to call functions by name from Wasmer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link says the parameter of a function takes valtype, which is one of i32
, i64
, f32
, f64
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I see now that argument names are not preserved. Thank you. Could you do now the research on how function names are handled in Wasm?
text/0000-contract-metadata.md
Outdated
For developers, there will be two main changes: | ||
- Instead of annotating view and change methods in the `initContract` function in `main.js`, | ||
they will instead annotate the methods of a contract by decorators. | ||
More specifically, every method is by default a change method, unless annotated by `@view_method`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest that all our methods are callable as near call
and near view
independently on whether they change the state or not. We should remove the current constraint the prohibits calling near view
on methods that mutate the state. There are not safety concerns to have this constraint. @evgenykuzyakov @ilblackdragon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not about safety, it's about developer experience. If I call the function and it didn't purist state change silently I won't know WTF is going on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are calling near view
then you know that it has no permanent effect on the state, because you read near view --help
before calling it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On this point, does a view
contract function have the restriction that it can't call a non-view
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Right now if you call a state-changing function within a view function, the execution will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the proposed change by max, that will not error but the state will not be changed instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two should be consistent though? Either they both error or neither error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think there is some advantage of being explicit. The annotation serves as documentation for developers who might interact with the contract and with metadata, the list of view methods can be easily extracted and displayed, which is better, in my opinion, than looking at the source code and tries to understand for each function, whether it changes the state or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From today's hackathon, the distinction between view and change methods should be either strictly enforced (and extensively documented), or their functionality should be merged into a single kind. Otherwise, it gets really confusing and error-prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay it seems like the distinction is confusing. I think we should eliminate the difference and allow dry run of change methods. One thing that is not clear to me is how we should support cross contract view calls. It seems that if you pass 0 gas, the instantiation of wasm module for the call on the second contract will fail.
I think we should compare how other blockchains are doing metadata for their contracts. We need to make sure this design is solid because we won't be able to iterate on it neither through governance nor forks; at least not without losing contracts that are already compiled to Wasm. Please check https://github.com/paritytech/ink and other blockchains that you can find. |
Would it make sense to add some form a An alternative I would suggest is make the metadata "output" as close to binary and low level as possible, and support the Binary data (simply the name, types and some other basic data like view and public/private) would be fairly robust since you aren't adding anything extra (and anything extra added after such data, could easily be combined with extension). One final comment I have is "Why must the data be returned inside of the contract?" Is there no "data" protocol you could use to store this, which would allow the same UX, but without bundling it onto the contract itself. Something like an IPFS hash of the file (with all files pinned) would provide the data in a computer readable format, while having an option to update the format in the future. |
Does parity have anything related to metadata? I didn't find any but maybe I missed something. |
As I mentioned above, the conventional name for contract metadata is ABI. Search for ABI in the link. But also, please don't wait for me to do the research and send you the links, there are prominent blockchains that have ABI, e.g. Ethereum https://solidity.readthedocs.io/en/develop/abi-spec.html# |
also contained in the metadata. | ||
|
||
# Motivation | ||
[motivation]: #motivation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to also include app title in the metadata.
If we look at playstore app submission process, it requires devs to fill in some display info for the app so that it can be listed in various UI contexts. For us, this might mean displaying the name of the app in the wallet for authorization, and/or NEAR app listing. This data may need to be localized if we want to support multiple languages in the future. Google in fact provides easy integration with their translation service in case the developer is unable to translate in-house.
For more info on play store app submission process see https://themanifest.com/app-development/how-publish-app-google-play-step-step-guide -- see product details section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds very much to me as following on from https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md With the idea of being able to add domain specific values (like app title) in a way that humans can verify it. Would it make sense to include all of this on contract, or out of band / on a separate part of the protocol?
EDIT: What would an exhaustive list of keys be that would be needed for every app, but not so large as to mean the majority of apps have "random_hex_string"
as a placeholder to satisfy the requirements?
@DanielRX @bowenwang1996 yes, we totally should have version of our build tools in metadata. |
Agree. Will update this when I have time |
ABI vs Metadata
The other item that appears for This to me seems like it is a feature not typically put onto the protocol level but the contract level (which is then put into the "protocol" level due to support for a specific form of ABI). The way I would view the naming of the spec (ignoring the fact this is bikeshedding at this point): Location of storageIs there a more logical location to store the metadata of the contract? Should it be stored in the code storage of the contract, or in a separate area (which lends itself to a much more "focused" area to find the data). If it were to be stored in a separate area, you could almost imagine it something like
or better:
|
Should the metadata string be signed, allowing for uses and consumers of the data to verify that the metadata belongs to the same contract they intend? It would be possible to do this on deploy (since the name it's stored at is not included on compile) or it could be bundled with the compiled code if the name was compiled with it. |
I agree, this sounds like a valid separation of abstractions.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good.
Please do not forget to add a subsection "Contract Metadata" to "Runtime" section in https://nearprotocol.github.io/nomicon/ that includes the final spec, once the implementation is done.
* `Seq<T>`: A variably sized homogeneous sequence of values, for example `Vec<T>` or `HashSet<T>` in Rust, `Array<T>` in | ||
AssemblyScript. | ||
* tuple: A statically sized heterogeneous sequence of values, for example `(u8, string)`. This doesn't apply to AssemblyScript. | ||
* `Map<K, V>`: A variably sized heterogeneous key-value pairing, for example `BTreeMap<K, V>` in Rust and `Map<K, V>` in AssemblyScript. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropping variants was a good call, they are too Rust-specific.
Currently there is no convenient way for a developer to programmatically list the methods of a contract that is deployed on chain | ||
because the contract code stored on chain is the compiled wasm code, where the parameters and type information are already mangled (even though | ||
method name is not, it is still cumbersome to extract them from wasm binary). | ||
As a result, if developers want to display the methods of a contract to an user of the app to provide transparency, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed with @bowenwang1996 before I think the only real application of the contract metadata is input validation. The case when front-end is somehow implemented to not have hardcoded dependencies on method or argument names has several issues:
- As discussed, it is very unlikely that front-end developer will somehow use a contract method without using the name of the method anywhere in their code (because they at least need to identify it) so if the contract method name changes they would have to manually modify the code anyway;
- It opens a huge security concern and such usage should be in general discouraged. We need to have a section in this NEP and documentation explaining the following example: Suppose someone malicious implements a contract with method
burn(gas_in_thousands: u64)
and lets other developers to use it in their front-ends. Then malicious contract developer changes the signature of the function to beburn(gas: u64)
without announcing to anyone. If front-end did not break because it has automatically rebound to the the argument name the users now are burning 1000 more gas without having any notification. The core problem with metadata is that it does not describe semantics, it describes syntax
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed with @bowenwang1996 before I think the only real application of the contract metadata is input validation.
As well as it allows to generate a wrapper which would automatically encode/decode ABI (which could be considered as an input validation too).
[guide-level-explanation]: #guide-level-explanation | ||
|
||
With contract metadata, developers, instead of annotating view and change methods in the `initContract` function in `main.js`, | ||
will instead annotate the methods of a contract by decorators. We propose that view functions be annotated with the `@view` decorator |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will instead annotate the methods of a contract by decorators. We propose that view functions be annotated with the `@view` decorator | |
will instead annotate the methods of a contract by decorators. We propose that view functions be annotated with the `@view` decorator in AssemblyScript (in Rust there is no need for additional decorators) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do we plan to do this in Rust? This decorator doesn't enforces anything but rather serves as a way to document the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically it's distinguished by the borrowing type of ABI method in implementation (fn method(&mut self)
or method(&self)
for view). See
https://github.com/nearprotocol/near-bindgen/blob/master/examples/status-message/src/lib.rs
text/0000-contract-metadata.md
Outdated
# Reference-level explanation | ||
[reference-level-explanation]: #reference-level-explanation | ||
|
||
To implement this NEP, we need to modify the binding generation procedure to generate a method called `metadata` that returns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To implement this NEP, we need to modify the binding generation procedure to generate a method called `metadata` that returns | |
To implement this NEP, we need to modify the binding generation procedure in AssemblyScript compiler to generate a method called `metadata` that returns |
To implement this NEP, we need to modify the binding generation procedure to generate a method called `metadata` that returns | ||
json serialization of contract metadata described in the previous section. This involves an AST walk to collect the relevant | ||
information about functions and classes, as well as mapping types to the types used in metadata. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Rust we would need to augment the procedural macro to inject the metadata method. |
"fields": [{"name": "id", "type": "string"}, {"name": "title", "type": "string"}, {"name": "completed", "type": "bool"}] | ||
} | ||
], | ||
"contract": {"name": "Todo list", "description": "A todo list on blockchain!"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contract name / description need to be localizable
wonder if maybe it makes sense to keep it in separate metadata, so that user can call it like description(userLocale)
and get this info for given locale.
Update: due to technical difficulties, we will not have contract metadata support for rust in the near future. Related issue: rust-lang/rust#44034 |
We should investigate whether WAI https://medium.com/wasmer/introducing-webassembly-interfaces-bb3c05bc671 can be used here as a drop-in universal solution. Pros: If it works we will be compatible with other Wasm infrastructure that supports WAI, e.g. Wasm package manager. |
Consider to use custom section as a good place to store metadata. |
We have implemented metadata for Rust contract in this PR: near/near-sdk-rs#99 The format is quite different from the one described in this PR. The explanation of the metadata format consists of mostly explanation of Borsh Schema format introduced here: near/borsh#56 and near/borsh#57 Since we don't have that documentation yet, I suggest we do not push this PR and instead later simply add documentation for Borsh Schema and metadata as a part of documentation work. CC @amgando @mikedotexe Therefore I am closing this PR |
@vgrichina feel free to change things/add more stuff.