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

Exposing Function Call Access Key Info At Runtime #371

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

BenKurrek
Copy link
Contributor

@BenKurrek BenKurrek commented Jul 13, 2022

This PR is based on the gov post found here that talks about introducing a way to query for function call access key information for the given contract at runtime.

@BenKurrek BenKurrek requested a review from a team as a code owner July 13, 2022 19:53
@render
Copy link

render bot commented Jul 13, 2022

@BenKurrek BenKurrek marked this pull request as draft July 13, 2022 19:53
@BenKurrek BenKurrek changed the title Exposing Access Key Info At Runtime Exposing Function Call Access Key Info At Runtime Jul 14, 2022
@BenKurrek BenKurrek marked this pull request as ready for review July 15, 2022 20:37
neps/nep-0371.md Show resolved Hide resolved
neps/nep-0371.md Outdated Show resolved Hide resolved
The contract runtime provides one host function called `access_key_info` which is callable in regular calls and views.

```rust
fn access_key_info (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this API can only be used to access info about a known key? What if a contract wants to know the actual set of keys associated with it? Maybe there is already an API for that or is that not an interesting use case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could interesting to paginate keys, we just haven't had anyone coming up with an actual use case, yet. :)

And because the API for that would be significantly trickier to implement, we opted it out, for now.

I expect most calls to either look up the current access key (signer_account_pk() already exists for that) or otherwise take an argument from outside that specifies which access keys to check. Listing all keys through rpc REST API has been supported for a long time, so the signer side should always be able to provide the list if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And because the API for that would be significantly trickier to implement

To expand on this, we don't really have a way to iterate keys in our runtime.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I understand this API now. Given a pk, it allows a contract to look up various details about it. How the contract got hold of the pk is outside of its scope. That makes sense.

Thinking out loud. If we expect a lot of use cases to be of the style: access_key_info(signer_account_pk(), ...), would it make sense to (in future) offer an optimising API here: access_key_for_signer(). This will mean that we save two copies of the public key. But I suppose that I am premature optimising here. And we can always introduce this API in the future if we think it is warranted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's important to note that the PK that can be passed in MUST belong to the contract account. If benji signs a txn with an access key where the contract is the receive, this won't return any info. The signer's account must be the contract.

neps/nep-0371.md Show resolved Hide resolved
allowance_ptr: u64,
/// out: register to write the access key holder account id
account_id_register_id: u64,
/// out: register to write the method names as a comma separated list
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a comma separated list

This is problematic: , is a valid character in a method name, so this API creates possibility for ambiguity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, I didn't realise it is technically an allowed character in a WASM method name. Very glad you point this out.

But that's interesting. When adding a function access key from within a contract, then the encoding used is a comma separated list, so this makes it impossible to create such a method access key this way. But that's just an artificial limitation from the host function implementation. From near CLI, it is possible to list methods with a , in it. And there is also nothing preventing for a contract with such a method name to be deployed. So this is a real concern.

The contract runtime provides one host function called `access_key_info` which is callable in regular calls and views.

```rust
fn access_key_info (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And because the API for that would be significantly trickier to implement

To expand on this, we don't really have a way to iterate keys in our runtime.

/// out: pointer to where a u128 can be written by the host
allowance_ptr: u64,
/// out: register to write the access key holder account id
account_id_register_id: u64,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should guarantee that this'll be a valid AccountId? Today we have some keys where the receiver_id is borked and isn't actually an AccountId. We are fixing that at the runtime level: near/nearcore#7139.

More specifically, if this fn queries for a key whose receiver isn't an AccountId, we can:

  • return receiver as a string
  • or pretend that the key doesn't actually exist.

Copy link
Contributor

@jakmeier jakmeier Jul 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. My gut feeling here says, if the access key exists, correct or not, this function should be able to reveal this. Otherwise code that checks if a key already exists before adding a new key like this will be buggy:

//...
signer_account_pk(pk_register_id);
let result = access_key_info (
    u64::MAX,
    pk_register_id,
    &mut nonce as *mut u64 as _,  
    &mut allowance as *mut u128 as _, 
    account_register_id,
    method_register_id,
);
match result {
 0 => {
       // key does not exist, so deploying should be possible, right?
       signer_account_id(signer_register_id);
       promise_batch_action_add_key_with_full_access(promise_idx, u64::MAX, signer_register_id, 0);
       // oops, above promise can fail with `AddKeyAlreadyExists` :(
    },
1 | 2 => {
        // key exists, need to delete it first
       promise_batch_action_delete_key(promise_idx, u64::MAX, pk_register_id);
       signer_account_id(signer_register_id);
       // now the new key can  be added
       promise_batch_action_add_key_with_full_access(promise_idx, u64::MAX, signer_register_id, 0);
    }
}

Therefore, I tend towards returning a string and making it clear that it may not necessarily reflect a valid account id.
We also return non-existing but valid receiver account ids. And the returned id would only be invalid if the user (or someone else with full access) has previously put the wrong id there. So this extra caveat seems like a non-issue from a user's perspective.

@matklad
Copy link
Contributor

matklad commented Jul 18, 2022

The issue with method_names seems significant to me. It seems like there's no way out of serializing variable number of variable-length keys. My gut feeling is that we should probably bite the bullet and just return borsh of the whole access key.

@jakmeier
Copy link
Contributor

The issue with method_names seems significant to me. It seems like there's no way out of serializing variable number of variable-length keys. My gut feeling is that we should probably bite the bullet and just return borsh of the whole access key.

@matklad I guess you are right. Do you think we can sneak it in as part of this NEP, making the precedence for borsh being used in the runtime API? Or should this go through a separate NEP with full discussion and getting everyone with a stake involved? I was hoping to avoid too much overhead here but it feels like adding borsh to the spec crosses a line that we should not rush over too quickly.

@akhi3030
Copy link
Contributor

The issue with method_names seems significant to me. It seems like there's no way out of serializing variable number of variable-length keys. My gut feeling is that we should probably bite the bullet and just return borsh of the whole access key.

@matklad I guess you are right. Do you think we can sneak it in as part of this NEP, making the precedence for borsh being used in the runtime API? Or should this go through a separate NEP with full discussion and getting everyone with a stake involved? I was hoping to avoid too much overhead here but it feels like adding borsh to the spec crosses a line that we should not rush over too quickly.

How about pagination for method names then? One function call can return how many method names there are. And then another takes an index specifying which method name to return. To reduce multiple context switches, in future we could introduce an API similar to https://linux.die.net/man/2/writev where the contract can provide the runtime a list of pointers to get back a list of method names in a single call. But we can keep it simple for now and just require the contract to call the function once per method name.

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the proposal. I would like to understand the following:

  • The motivation talks about making developer experience better. Could you provide a specific example of why this host function is needed?
  • What happens if we change AccessKey in the future? (for example we implement feat(runtime): Add delegate keys #342) How does that affect this host function?

@BenKurrek
Copy link
Contributor Author

Thanks for the proposal. I would like to understand the following:

  • The motivation talks about making developer experience better. Could you provide a specific example of why this host function is needed?
  • What happens if we change AccessKey in the future? (for example we implement feat(runtime): Add delegate keys #342) How does that affect this host function?

@bowenwang1996 I can speak to the first point. As stated in the governance post:

For a real world example of why this would help, i’ll explain a use case. Imagine you have a contract that allows users to “purchase” function call access keys that can do different things. The user must pay for the access key allowance when the key is created. An example of this is the linkdrop contract.

When the key is used up / deleted, dApps might want to refund users for any allowance that was unspent. Currently, there is no way to do this. In addition, when creating the key, often times, the dev will know how much GAS will be attached to the function calls and they might want to dynamically calculate the pessimistic allowance that the key must have and to do this, they need the current GAS price.

In addition, @mattlockyer had shared his opinions in the post as well:

An abstract but fundamental use case would be some form of meta transaction.

I want Alice to fund an access key, that is added to a contract she doesn’t control, such that Bob can call a method 10 times.
On the 10th call, the key is deleted and any unused allowance from that key is returned to Alice.

Currently, there is no way to check the unused allowance of the key from inside the contract runtime, AND there is no way to check the current gas price, so crude approximations have to be used that often overestimate how much gas was actually spent.

Panics and reversions further complicate this scenario, since gas is used, but no state is changed. This forces the developer to never panic on an assertion, and instead write a crude approximate of used gas to state and return false, before exiting the method.

If the protocol allowed us to see the allowance, method_names and receiver_id of access keys, there could be more use cases opened up.

@bowenwang1996
Copy link
Collaborator

I want Alice to fund an access key, that is added to a contract she doesn’t control, such that Bob can call a method 10 times.
On the 10th call, the key is deleted and any unused allowance from that key is returned to Alice.

I see. So basically you want Alice to pay for Bob's transactions. There is a separate proposal on meta transactions, which could provide in my opinion a better solution to this problem. Are there any other examples or use cases for this feature?

@mattlockyer
Copy link
Contributor

I see. So basically you want Alice to pay for Bob's transactions. There is a separate proposal on meta transactions, which could provide in my opinion a better solution to this problem. Are there any other examples or use cases for this feature?

I believe in the proposal you mentioned, Alice still needs to execute the transaction.

Using access keys, the way they were intended, someone else can hold the key and execute the transactions when they want.

The nuance being the client in control of the key that's executing the transaction.

This proposal allows developers to examine and deal with the function call parameters of access keys that may have been added to their contract and distributed to clients, so you can fine tune access control and measure the refund of allowances that go unused.

@jakmeier
Copy link
Contributor

@bowenwang1996 Valid question, I see two directions to deal with such cases.

  1. Add different functions for each permission type. E.g. full_access_key_info(), function_access_key_info(), delegation_access_key_info()
  2. Make access_key_info() more flexible.

I am torn between the two. The first option may lead to inconsistent API, as we add more and more functions over time. The second option tries predict the future, which usually doesn't work.

To make it more tangible, the second option could roughly look like this: (Also incorporating @akhi3030 suggestion to paginate methods.)

  • access_key_info queries by PK, maybe rename it to access_key_info_by_pk(). It returns 0: key doesn't exist, 1: key is full access key, 2: key is function access key. Other return values are reserved for future use. All output parameters are removed.
  • For feat(runtime): Add delegate keys #342, we could add access_key_info_by_account_id().
  • access_key_iter_next is used for paginating over key information, returned to registers in a pre-defined order that depends on the key type.
    • For function access key, this could be [nonce, public_key, allowance, method[0], method[1], ..., ].
    • For a delegation access key, it could be [nonce, account_id, action[0], action[1], ..., ]

I think this would allow to cleanly add all kinds of new access keys. But who knows what the future brings.

@BenKurrek
Copy link
Contributor Author

Here is another user asking for this functionality this morning:
https://stackoverflow.com/questions/73277923/near-sdk-rs-view-account-keys

@frol frol added T-runtime About NEAR Protocol Runtime (actions, Wasm, fees accounting) WG-protocol Protocol Standards Work Group should be accountable labels Sep 5, 2022
@ori-near
Copy link
Contributor

Hey @BenKurrek – thank you for submitting this NEP. In order to move this NEP to the review stage, can you please add a reference implementation?

@ori-near ori-near added the S-draft/needs-author-revision A NEP in the DRAFT stage that needs an author revision. label Sep 22, 2022
@ori-near ori-near added the A-NEP A NEAR Enhancement Proposal (NEP). label Oct 13, 2022
@ori-near ori-near added the S-draft/needs-implementation A NEP in the DRAFT stage that needs implementation. label Nov 18, 2022
@ori-near
Copy link
Contributor

Hi @BenKurrek @jakmeier (or anyone else) – are you still interested in moving this NEP forward? If so, can you please submit a reference implementation?

@BenKurrek
Copy link
Contributor Author

I'm definitely interested in moving it forward but don't have time to put out a reference implementation at the moment. Perhaps I can revisit when I have some time.

@mattlockyer
Copy link
Contributor

@ori-near can you please provide some links to other reference implementations e.g. a couple of closed PRs to nearcore that we can look at?

I don't think this should be incredibly difficult.

We know everyone would like to keep the API surface small for packages like near-sys, however getting access key information about the account your contract is currently executing on is fairly critical for some use cases.

@jakmeier
Copy link
Contributor

jakmeier commented Dec 2, 2022

@mattlockyer here is the complete reference implementation for another host function: near/nearcore#7165 (some follow-up work was necessary but for a reference implementation this is more than needed)

Regarding this feature here, I agree it is fairly basic and probably should be there. The main questions/concerns for me are:

  1. Is it actually making new use cases possible, or is it just making hacky solutions slightly less hacky? If it is the latter, we should consider making changes that solve the use cases cleanly. (This is why I deprioritized working on this to focus on things like supporting meta transactions instead.)
  2. What would happen to fn access_key_info() if we make drastic changes to how keys work in the future? There are a bunch of ideas flying around here and we would add complexity to each of those ideas if we also need to consider how fn access_key_info() is affected.

But if you think this is a valuable addition to for smart contract devs and you can provide a reference implementation, then we can certainly move it forward. Me personally, I don't think I have the capacity to implement it in the near future.

@nearbuild
Copy link

@mattlockyer @BenKurrek wanted to reping on this since progress on #452 moves forward

@walnut-the-cat
Copy link
Contributor

Hello! Protocol NEP moderator here. it seems there hasn't been any activity on this NEP for quite some time. Is it still on going? Or shall we close NEP for now? @BenKurrek, @jakmeier

@walnut-the-cat
Copy link
Contributor

Another ping on status of this NEP. As more than two months have passed since the last activity, if we don't hear back from the author by December 4th, we will mark the NEP as retracted for now. Thanks-

@walnut-the-cat
Copy link
Contributor

I will assume this NEP is inactive and label as retracted for now. If anyone picks the NEP up again, please ping NEP moderators to make it active.

@walnut-the-cat walnut-the-cat added S-retracted A NEP that was retracted by the author or had no activity for over two months. and removed S-draft/needs-author-revision A NEP in the DRAFT stage that needs an author revision. S-draft/needs-implementation A NEP in the DRAFT stage that needs implementation. labels Dec 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NEP A NEAR Enhancement Proposal (NEP). S-retracted A NEP that was retracted by the author or had no activity for over two months. T-runtime About NEAR Protocol Runtime (actions, Wasm, fees accounting) WG-protocol Protocol Standards Work Group should be accountable
Projects
Status: NEW❗
Status: RETRACTED
Development

Successfully merging this pull request may close these issues.

10 participants