-
Notifications
You must be signed in to change notification settings - Fork 148
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
[NEP-591]: Global Contracts #591
base: master
Are you sure you want to change the base?
Conversation
aa2970e
to
650b881
Compare
650b881
to
39b0c80
Compare
39b0c80
to
b4df91c
Compare
b4df91c
to
6abff6a
Compare
6abff6a
to
424159e
Compare
Hi @pugachAG – thank you for starting this proposal. As the moderator, I labeled this PR as "Needs author revision" because we assume you are still working on it since you submitted it in "Draft" mode. |
Global contract can be deployed in 2 ways: either by its hash or by owner account id. | ||
Contracts deployed by hash are effectively immutable and cannot be updated. | ||
When deployed by account id the owner can redeploy the contract updating it for all its users. |
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 we talk about when to use each way? For instance, when should a user use hash instead of account id, and vice versa?
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.
Sure, added in bfad2e3
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.
Sorry to jump in, but I’m curious, will the deployed-by account ID receive a hash?
I don’t see a reason to allow someone to change my code and potentially take everything from my contract. But if a globally deployed account also gets a hash that I can lock onto (and update later if needed), that would be a nice DevX improvement.
I do see a downside, though that every update essentially creates a new copy of the code instead of replacing it. So I’m curious, how is this going to be addressed?
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.
@akorchyn Nope, in this case we store account id, so the owner can update contract for all current users. There are underlying use cases for that, for example application developers onboarding users on NEAR.
Also it is possible to lock such contract if owner deletes all keys from the account, effectively making it impossible to make any changes.
To help me schedule the work here, what is the deadline for this review? |
@akhi3030 testnet release is scheduled for March 24th, so it would be nice to get this NEP reviewed by that that date. |
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 had the first look, looks good so far, I'll come back to it some time this week.
} | ||
``` | ||
|
||
Also new action is added for using previously deployed global 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.
Can you clarify that this action is the same / similar as deploying the contract directly to the account?
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.
addressed in c528f22
``` | ||
|
||
Also new action is added for using previously deployed global 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 it possible to "unlink" the global contract from the account? Is it even possible for regular contracts? What about cleaning the state of the contract after it's removed?
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 far as I know it is not possible to unlink the regular contract, so we don't have to solve that for the global contracts. The same applies for the state of the contracts.
neps/nep-0591.md
Outdated
`GlobalContractDistribution` receipt is generated as a result of processing `DeployGlobalContractAction`. | ||
Receipt distribution logic is updated to route such receipts to all shards. | ||
So effectively it is a part of `ShardProof` (incoming receipts) for each shard, but occurs only once in the outgoing receipts of that chunk. | ||
Applying `GlobalContractDistribution` receipt updates the corresponding `TrieKey::GlobalContractCode` in the trie. |
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 would be good to test that this logic works well in case of resharding. One particular corner case would be:
#10
- deploy smart contract in shard X#11
- shard Y and shard Z are both missing chunk#12
- new shard layout after resharding where Y was split into Y1 and Y2
For X = Y, X = Z, X != Y && X != Z
### Costs | ||
|
||
For global contracts deployment we burn tokens for storage instead of locking like what we do regular contracts today. | ||
The cost per byte of global contract code `global_contract_storage_amount_per_byte` is set as 10x the storage staking cost `storage_amount_per_byte`. |
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 it set to 10x symbolically or do you just set its value to be 10x? I would suggest decoupling them so that they can be updated independently.
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.
+1 to this comment.
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.
global_contract_storage_amount_per_byte
is a separate runtime parameter completely independent from storage_amount_per_byte
, here I meant that initial values is set to be 10 * storage_amount_per_byte
/// Contract is deployed under the owner account id. | ||
/// Users will be able reference it by that account id. | ||
/// This allows the owner to update the contract for all its users. | ||
AccountId, |
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.
Does deploying a global contract by account id deploy said contract to the owner account as well?
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.
nope, it does not do that
Global contract has to be distributed to all shards after being deployed. | ||
This is implemented with a dedicated receipt type: |
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 this accounted for in the BandwidthScheduler?
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.
implemented as part of near/nearcore#13147
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.
generally looks good to me and I am happy for it to proceed with the current spec. I will do a detailed SME review soon.
### Costs | ||
|
||
For global contracts deployment we burn tokens for storage instead of locking like what we do regular contracts today. | ||
The cost per byte of global contract code `global_contract_storage_amount_per_byte` is set as 10x the storage staking cost `storage_amount_per_byte`. |
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.
+1 to this comment.
Co-authored-by: Adam Chudaś <[email protected]>
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 a subject matter expert I approve this NEP.
It adds an important feature, it is well designed, it strikes a great balance between added complexity and practical aspects and it well integrates with the rest of nearcore. Thank you @bowenwang1996, @pugachAG and @stedfn for this great proposal.
`GlobalContractDistribution` receipt is generated as a result of processing `DeployGlobalContractAction`. | ||
Such receipt contains destination shard `target_shard` as well as list of already visited shard ids `already_delivered_shards`. | ||
Applying `GlobalContractDistribution` receipt updates the corresponding `TrieKey::GlobalContractCode` in the trie for the current shard. | ||
It also generates distribution receipt for the next shard in the current shard layout. This process continues until `already_delivered_shards` contains all shards. |
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.
One consequence of this approach is that global contract deployment will take at least num_shards blocks to get onto all shards. In the future we may need some way of checking if the contract is fully deployed / updated. Nothing to worry about now, I'm sure the contract developers will ask about it once they need 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.
Indeed with more shards in the future propagating global contract updates might take quite some time.
We can extend the functionality in the future by adding some deployment progress indicator as part of owners account id data if that would be required.
We change `Account` struct to make it possible to reference global contracts. | ||
`AccountV2` is introduced changing `code_hash: CryptoHash` field to more generic `contract: AccountContract`: | ||
|
||
```rust | ||
enum AccountContract { | ||
None, | ||
Local(CryptoHash), | ||
Global(CryptoHash), | ||
GlobalByAccount(AccountId), | ||
} | ||
``` |
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 it be possible to deploy / use global contracts from accounts created before AccountV2 is released? Is there any migration / lazy update necessary?
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.
AccountV2 is released along with global contract.
User account is upgraded to V2 lazily when global contract is used, so no migration is required.
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.
NEP Status (Updated by NEP Moderators)
Status: Review
SME reviews: