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

Generate manifest with static calls #478

Merged
merged 12 commits into from
Mar 17, 2021

Conversation

shargon
Copy link
Member

@shargon shargon commented Mar 11, 2021

Close #475

@superboyiii
Copy link
Member

@shargon I'll test this later but I think it's better not be merged to RC1 since not much time left.

@erikzhang
Copy link
Member

@superboyiii It should be added to RC1.

@superboyiii
Copy link
Member

superboyiii commented Mar 12, 2021

I've tested this. It works but I'm confused if it makes sense in this mechanism.

  1. In previous, innative contracts are allowed to call all contracts as default. And if add 1 permission, it becomes only allowed this permission method to call.
  2. In this PR, innative contracts are not allowed to be called by all contracts as default. That means if we'd like to call any more method of other SC, we have to upgrade our SC again and again. In most DAPP, modules are added step by step not at the same time. In this way, I think most developers will add [ContractPermission(*)] at the beginning and upgrade ContractPermission later. So is it useful we set forbidden all as the default?
    @shargon @erikzhang

@shargon
Copy link
Member Author

shargon commented Mar 12, 2021

We can add a compiler flag --allowAllCalls?

@erikzhang
Copy link
Member

[ContractPermission("*")]

You need to manually add this line to declare the calling permissions of all contracts.

@erikzhang
Copy link
Member

Tested?

@superboyiii
Copy link
Member

Tested?

I will test it soon.

@superboyiii superboyiii mentioned this pull request Mar 15, 2021
27 tasks
@superboyiii
Copy link
Member

superboyiii commented Mar 15, 2021

@shargon I set [ContractPermission("*", "*")] but ToDelegate() still return fault because of authorization. Does this mean if natvie management contract is allowed in permission as default, then all permissions must add manually? "*" seems doesn't work.

{
    "jsonrpc": "2.0",
    "id": 3,
    "result": {
        "script": "ABQMFI5SMl22gwrQirvz2CzRnQyvhFHfDBSabQINJN1EW/uACj2hZ5UvHKCtDwwUj0sLioGSzZgeFe060WuXVzteXjgUwB8MBXRlc3QyDBTX2B1RYQz8amWTYixFhXS9jndqD0FifVtS",
        "state": "FAULT",
        "gasconsumed": "3471120",
        "exception": "An unhandled exception was thrown. No authorization.",
        "stack": []
    }
}

@erikzhang
Copy link
Member

erikzhang commented Mar 15, 2021

[ContractPermission("*", "*")]

I think it's not [ContractPermission("*", "*")] but [ContractPermission("*")].

@superboyiii
Copy link
Member

superboyiii commented Mar 15, 2021

I think it's not [ContractPermission("*", "*")] but [ContractPermission("*")].

I've tried both, [ContractPermission("*", "*")] and [ContractPermission("*")] does no difference in either nef hash or manifest json result. And I saw my permission and native permission exist both, which should be the final result?

image

@erikzhang
Copy link
Member

which should be the final result?

The first one.

But I think we should optimize for it. @shargon

@superboyiii
Copy link
Member

superboyiii commented Mar 15, 2021

Sorry, I turned witnessScopes from CalledByEntry to Global, everything works well.
So in current mechanism, default still is allowAll and whatever your permission is, contrtactmanagement can always be invoked in getContract, destroy, update.
@shargon @erikzhang

@erikzhang
Copy link
Member

You still need the permissions. Otherwise it should fail.

@superboyiii
Copy link
Member

You still need the permissions. Otherwise it should fail.

If one permission is manually added, then will be denyAll except contractmanagerment. But if you don't set any permission, it's the same effect as [ContractPermission("*", "*")].

@superboyiii
Copy link
Member

Test PASS
It works well on mine.

superboyiii
superboyiii previously approved these changes Mar 15, 2021
@erikzhang
Copy link
Member

@shargon Can you optimize the permission list?

@shargon
Copy link
Member Author

shargon commented Mar 15, 2021

Can you optimize the permission list?

Yes, I will

@superboyiii
Copy link
Member

Can you optimize the permission list?

Yes, I will

If optimize it, we need do it quickly since I'll release CLI today and set up Testnet on 18th. It should be merged before Testnet.

@shargon
Copy link
Member Author

shargon commented Mar 15, 2021

@superboyiii please test it

@superboyiii
Copy link
Member

superboyiii commented Mar 15, 2021

@superboyiii please test it

It works on mine.
with ["*"]
image

without ["*"]
image
They all work as expected.

superboyiii
superboyiii previously approved these changes Mar 15, 2021
@superboyiii
Copy link
Member

@erikzhang Need your review.

@erikzhang
Copy link
Member

@shargon Please use the PermissionBuilder added in da3b922.

@shargon
Copy link
Member Author

shargon commented Mar 16, 2021

@superboyiii please test it

@superboyiii
Copy link
Member

superboyiii commented Mar 16, 2021

@shargon Double []?
image
ContractPermission FromJson() will fail here.
image

erikzhang
erikzhang previously approved these changes Mar 16, 2021
@superboyiii
Copy link
Member

superboyiii commented Mar 17, 2021

Test PASS
Works well on mine. But need fix the UT. @shargon

@erikzhang
Copy link
Member

Please fix the UT.

@superboyiii
Copy link
Member

@shargon I just push a PR on your repo to fix this, could you review it? neo-project/neo-modules#564

Fix UT and upgrade to Neo v3.0.0-CI01246
@shargon
Copy link
Member Author

shargon commented Mar 17, 2021

@superboyiii merged!

@superboyiii
Copy link
Member

@erikzhang Merge?

@shargon shargon requested a review from erikzhang March 17, 2021 07:48
@erikzhang
Copy link
Member

It's better to test it again before merging.

@superboyiii
Copy link
Member

It's better to test it again before merging.

Give me half an hour.

@superboyiii
Copy link
Member

It's better to test it again before merging.

Test PASS
Works well on mine.

@erikzhang erikzhang merged commit 81634a3 into neo-project:master Mar 17, 2021
@shargon shargon deleted the auto-manifest-perms branch March 17, 2021 10:05
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.

[Suggestion] automatically add permissions?
3 participants