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
NEP-480: Account Namespaces #480
NEP-480: Account Namespaces #480
Changes from 8 commits
ce04936
a313091
c192f43
4b3854c
0bcafe3
5d7bbc3
52c9906
5ae93a7
70a2950
b479eaf
1cb0c23
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.
This does not explain how namespaces enable this feature. More specifically, what is the limitation of the current protocol that is eliminated with this proposal
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.
Workflow example:
alice.near
.v1
to the default namespace.upgrade-manager
to theupgrade-manager
namespace. Theupgrade-manager
bytecode provides anupgrade
method that performs a contract upgrade sequence (explained in later steps).alice.near
.v2
.alice.near:upgrade-manager->upgrade(v2)
, passing in thev2
bytecode.upgrade-manager
code performs a contract upgrade sequence, deploying thev2
bytecode to the default namespace ofalice.near
.v2
bytecode is invalid/unusable.v3
.upgrade-manager
to perform the upgrade sequence withv3
.If namespaces had not existed, the
v2
deployment would have bricked the entirealice.near
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.
Are these the only capabilities that you propose to enable? What about arbitrary host functions, etc.? Having read through the proposal in more detail, I think it is important to have fine grained capabilities for namespaces. What you are proposing is to allow smart contract developers who are not proficient to deploy multiple wasm modules on their account. These wasm modules could be buggy or malicious. If this proposal is expected, I can imagine some users just having a single account with all the different contracts they want to try out / experiment with and without fine grained control over their capabilities, they could end up losing a lot of funds.
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'm not sure how all of the questions in this comment fit together.
Currently, I don't think that the typical non-tech-savvy user deploys any code to their accounts, but please correct me if I am wrong.
Specifically when speaking of non-tech-savvy users, the workflow we conceptualized is something like:
bob.near
dead-man-switch
0x...
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 reason why we’d need a concept of namespace creation? Would this creation step introduce semantics meaningfully different from the empty namespace existing for accounts that haven't deployed a contract yet?
If not, I think it would be quite a bit more straightforward to work with this feature if all namespaces were presumed to “exist” with a code_hash equal to
11111111111111111111111111111111
or whatever we use for invalid 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.
Namespace creation is approximately equal to contract deployment in this NEP. If all namespaces are presumed to exist, how does an RPC account view call show namespaces?
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.
Hm, I'm not sure! I imagine it would list the non-
111...111
hashes, rather than the list of “namespaces”?But counter-question to your question: what's the expected behaviour of calling a non-existent namespace? If my memory serves me right, in the namespace-less world of today, the behaviour is meaningfully different between calling a function in a non-existent account and calling a function in an existing account without a contract deployed.
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 that case would be similar to calling a non-existent method on an account that does have a contract deployed. Possibly a case for a new, "namespace does not exist" 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.
Is there a reason I’m not seeing why this deviates from the usual “no namespace ⇒ empty namespace” defaulting elsewhere in this NEP? I feel like it might be a notable implicit foot gun in the use of protocol. Consider for example:
namespace
;Forgetting I’ve done 2, I have accidentally allowed the important contract to be called arbitrarily by the old key(s).
The alternative is to equate “empty
namespaces
⇒ default empty namespace” here as well, in which case the failure mode is that I attempt to call the new contract with an old key, notice it doesn't work and expand the permissions for the key.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.
Right now, a function call access key that does not specify a list of methods is allowed to call any method. Therefore, a function call access key that does not specify a list of namespaces is allowed to call any namespace.
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.
Right. I feel like the stakes are likely somewhat different here in that deploying a new contract to an account in absence of namespaces overwrites the old one, and thus the user is conscious about both the previous and the new contract (and thus possibly is thinking of the access controls in context of both.) In the world of namespaces they no longer need to care about any previous state anymore.
But fair, I see how the proposed behaviour is consistent.
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.
Maybe we need a new syntax for function call access keys, wherein they can be permitted to call "any method on
xyz
namespace"?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.
Hmm... so we are using the empty namespace to represent two different values: empty namespace in the predecessor or no smart contract. Could this be a problem? Is there a good way to distinguish between the two?
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 usually don't care too much whether a call comes from an EOA or contract, but if the distinction is pertinent, one may use
env::signer_account_id()
.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.
What would be the value of obtaining the storage usage of a different namespace than the one currently executing? If there isn’t one, might it make sense to require that the
Namespace
arugment iscurrent_namepsace()
otherwise a panic is raised for the time being?Might be worthwhile to double check if an ability to do so does not preclude whatever other improvements we might want to make.
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 of the suggested use-cases for namespaces is to support "hot-swapping" wherein a manager namespace deploys code to other namespaces and manages their state and execution at a high level, so some cross-namespace introspection functions like this would be useful.
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 current proposal, then there are two ways of getting storage used.
storage_usage()
andnamespace_storage_usage()
. I can see some developers getting confused whenstorage_usage()
does not add up to what they are seeing the current namespace is actually using (they might have forgotten that other namespaces exist).In the ideal world, it would be great to have a single hostfunction with a signature like:
storage_usage(enum {EntireContract; Namespace(namespace)'})
. Does something like this make sense?If it does, I propose that we update the proposed
namespace_storage_usage()
to be something like the above and then markstorage_usage()
as deprecated. We probably never will be able to remove it but hopefully with sufficient documentation, we will minimise the confusion.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 I have enough information to have an informed opinion here. Making a decision one way or another has the potential to break contracts: if
storage_usage()
only references the current namespace, old contracts could incorrectly think the account is not using storage when it is, and if it references the whole account's usage (all namespaces), then old contracts could incorrectly think the current contract is using storage when it is 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.