Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Basic contract metadata proposal #3
Changes from 5 commits
10854b9
5992aea
757e927
b449eaf
c178b4d
17951dc
e994817
00a005c
697f384
301eeb3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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?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).
I don't see how this:
implies this:
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.
http://webassembly.github.io/spec/core/text/types.html
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?
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:
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 syntaxThere 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 well as it allows to generate a wrapper which would automatically encode/decode ABI (which could be considered as an input validation too).
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
andnear view
independently on whether they change the state or not. We should remove the current constraint the prohibits callingnear view
on methods that mutate the state. There are not safety concerns to have this constraint. @evgenykuzyakov @ilblackdragonThere 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 readnear 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 anon-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.
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.