-
Notifications
You must be signed in to change notification settings - Fork 102
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
Sync neo changes #391
Sync neo changes #391
Conversation
shargon
commented
Nov 12, 2020
•
edited
Loading
edited
- NEP17 changes Implement NEP-17 neo#2024
- Contract hash create changes Update contract without change the hash neo#2044
- Require Remove ContractFeatures #390 Update contract without change the hash neo#2044
- Close Neo.Compiler.MSIL no longer compiles against neo master branch #400
call |
In the template right? |
Yes |
UT failed |
It will need the neo nuget |
{ | ||
var docMap = GetDocumentMap(module); | ||
addrConvTable ??= ImmutableDictionary<int, int>.Empty; | ||
|
||
var outjson = new JObject(); | ||
outjson["hash"] = FuncExport.ComputeHash(script); |
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.
@devhawk do you need the name in debugExport
?
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 need whatever unique identifier is used to identify the contract.
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.
Is there a design document describing how contract deploy and invoke is supposed to work now? I am all for having a stable contract identifier other than the script hash, though it's not clear what the new design is?
One question - Have we considered the impact on dynamic contract invocations scenarios. Using script hash to invoke a contract means that any calling contracts break when a dependent contract is updated. That is, I was calling a specific contract by script hash it will be obvious something has changed if that script hash is no longer valid. With a stable identifier, will it still be obvious if a dependent contract has made a backwards incompatible change?
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.
Hi @devhawk, thinking in this perspective you mentioned, it may be important to create a flag that says that the contract trust any update on the dynamic invoked contract.
For instance, most of the DeFi projects are relying on external dynamic invoked contracts. In this sense, consider the need of mutual update is something plausible.
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 would expect that dynamic invoke scenarios always require "mutual update" as you called it @vncoelho. I'm not sure it's a good idea to make the change less obvious. Right now, if you update your contract it breaks anyone who calls it since the hash changes. If we keep the hash, will that allow a developer to publish a malicious contract update w/o their users noticing?
src/Neo.Compiler.MSIL/FuncExport.cs
Outdated
//hash | ||
outjson["hash"] = ComputeHash(script); | ||
//name | ||
outjson["name"] = module.attributes |
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.
According to the https://github.com/neo-project/neo/pull/2024/files#diff-6a8fe6159fb062d52f619635b6c0e2123632e3a61acdc5f86b867da878af2e02R166, name
is on first level in manifest, but here the name
is inside the abi
.
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.
Now I am not sure, maybe it's more appropriate to put the Name
in the abi 🤔. What do you think?
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 name
and supportedstandards
should be on the same level, maybe it shouldn't be added here?
UT failed |
It require neo-project/neo#2044 |
Test passed with commit 882a721
|
#391 (comment) |
@cloud8little Fixed. |
When transfer NEO, it'll trigger twice Another is NEO transfer: There are two |
It still get the wrong CallingScriptHash. I found the first received |
It should be fixed by neo-project/neo#2130 |
Because the problem it's in core and not in the template, we can merge it? |
I think template is OK, we can merge it first because #402 is waiting for this. |
Merge? 🚀 |
@shargon We need to fix #391 (comment) |
Co-authored-by: Erik Zhang <erik@neo.org>
Null should return false or throw? |