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

feat: draft implementation of NEP-366 #7497

Closed
wants to merge 31 commits into from

Conversation

e-uleyskiy
Copy link

@e-uleyskiy e-uleyskiy commented Aug 29, 2022

Hello!

This is a draft implementation of NEP-366 (near/NEPs#366)

Please, review the code and architecture we proposed.

Diagram V3

UPD: I removed old description because it was too outdated.

Diagram V2 is here

Diagram V1 is here

This is a draft implementation of NEP-366 (near/NEPs#366)

Not done yet:
* Need to implement DelegateAction for implicit accounts (nonce problem)
* Need new error codes for DelegateAction
* Implement Fees for DelegateAction
* Add tests
@akhi3030
Copy link
Collaborator

@jakmeier has volunteered to do a first pass review here. We will pull in additional folks as needed.

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Thank you for the draft implementation and the description with diagram! I think this outline adds a lot of details on top of the original NEP. The outline presented here makes it possible to start a discussion on how this works in details.

So I have now taken an extensive look but I must say I am still quite confused.

My main questions are, which keys are owned by whom and correspond to which signatures. What's described in the NEP isn't quite clear to me in some details. And the diagram lacks these details. And the code doesn't really make the intention quite clear either.

Also, I think the diagram only describes the user's intention. Missing are the technical details of how it is executed on chain, which is arguable the important bit for the specification. What I would like to see is a clear description, ideally in a diagram, that makes the steps clear. In my head the steps are:

  1. Alice creates a SignedDelegateAction (not a transaction) and sends it to the publisher off-chain.
  2. The publisher creates a transaction with the delegation action, signs it with their own key and sends it to the account that will pay for the execution.
  3. Transaction is converted to one delegation action receipt, gas is paid for by publisher, and the receipt is sent to the receiver defined inside SignedDelegateAction.
  4. Delegation action receipt is applied by Alice's account and action receipts for each inner action are produced.

In each step I would like to know exactly who is signer of a transaction or receipt, who is receiver of inner and outer actions and action receipts, who is predecessor, and who is publisher. Also who signed SignedDelegateAction should be clearly visible.

Maybe something like the existing diagram but showing the values of all the involved PKs and account fields clearly stated in each step. Probably then the diagram also needs to clearly separate steps 3 and 4. I think that would go a long way to clarify how this works.

I have more comments and also some ideas around the 3 pain points you listed. But let's clarify my questions first to make sure I am not writing complete nonsense.

Comment on lines 239 to 245
pub struct SignedDelegateAction {
// Borsh doesn't support recursive types. Therefore this field
// is deserialized to DelegateAction in runtime
pub delegate_action_serde: Vec<u8>,
pub public_key: PublicKey,
pub signature: Signature,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Some comments on public_key and signature would be helpful. Whose signature is it? Whose PK? My guess is both belong to the delegator, which would be Alice in your example.

Comment on lines 806 to 812
let signer_is_predecessor =
action_receipt.signer_id.as_str() == receipt.predecessor_id.as_str();
let refund_id = if action_receipt.publisher_id.is_some() && signer_is_predecessor {
action_receipt.publisher_id.as_ref().unwrap()
} else {
&receipt.predecessor_id
};
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this condition.

signer_is_predecessor I think means that this call has no predecessor or function calls have gone around in a loop. The shortest loop being Alice using delegation to call a function on her own contract.I don't understand how this is relevant to how refunds are handled. Can you please elaborate a bit and help me understand what this is doing?

My take would have been that deposit refunds still always go to the predecessor, regardless of delegated actions. And gas refunds should always go the publisher, if there is one. I don't see why it would not go to the publisher in the case where the signer is the predecessor.

But it is very much possible that I misunderstand something entirely.

core/primitives/src/receipt.rs Outdated Show resolved Hide resolved
core/primitives/src/transaction.rs Outdated Show resolved Hide resolved
#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))]
#[derive(BorshSerialize, BorshDeserialize, Serialize, Deserialize, PartialEq, Eq, Clone, Debug)]
pub struct DelegateAction {
pub reciever_id: AccountId,
Copy link
Contributor

Choose a reason for hiding this comment

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

The receiver in this case is the delegator, right? (Alice in your example) Maybe write that in a comment.

(Also, there is a typo in receiver)

Copy link
Member

Choose a reason for hiding this comment

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

receiver_id is who the actions in this Action will be targeted toward after it has been delegated.
Think of DelegateAction/SignedDelegateAction as Transaction and SignedTransaction respectively.

Adding description comment to DelegateAction with a link to NEP and moving diagram there will be useful indeed.

pub(crate) fn action_delegate_action(
apply_state: &ApplyState,
action_receipt: &ActionReceipt,
predecessor_id: &AccountId,
Copy link
Contributor

Choose a reason for hiding this comment

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

predecessor_id is ambiguous here and confused me a lot.

If I'm understanding it right, this is the predecessor of the actions that will be spawned now, which is going to be the current account that unpacks the delegated actions. In your example, this should be Alice. Whereas the predecessor of the current action is the publisher.

Maybe rename to something like delegator_id.

let cost = receipt_cost(transaction_costs, current_protocol_version, &new_receipt)?;

if let Some(refund) = delegate_action.deposit.checked_sub(cost.clone()) {
let refund_receipt = Receipt::new_balance_refund(&action_receipt.signer_id, refund);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason that we have to handle deposit refunds specially?
I would have expected that we treat it like existing deposit refunds. That is, towards the end of apply_action_receipt we call generate_refund_receipts and give a refund to the predecessor of the action. The predecessor of the delegation action should be the publisher, right? So that seems correct to me.

Copy link
Author

Choose a reason for hiding this comment

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

Is there a reason that we have to handle deposit refunds specially? I would have expected that we treat it like existing deposit refunds. That is, towards the end of apply_action_receipt we call generate_refund_receipts and give a refund to the predecessor of the action. The predecessor of the delegation action should be the publisher, right? So that seems correct to me.

This deposit covers the total cost (gas and deposits) of all actions in DelegateAction. To run actions from DelegateAction, some prepaid gas is required. Relayer doesn't prepaid gas but attaches deposit instead. If Relayer attaches deposit which is less than the total cost of actions, need to generate a refund.
generate_refund_receipts generates a refund only when the receipt fails. This is not a fail case.

Why just do not pre-pay gas? Then I will play with result.gas_used counter here (and maybe deposit, if we want to refund it). result.gas_used will not be actually used but will be "reserved" for actions in DelegateAction. Otherwise generate_refund_receipts will refund all prepaid gas and check_balance call panic.

Both variants are tricky and I chose the first one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for explaining.

Then, there is one issue with the current design. The gas price for the initial receipts is computed based on attached gas = 0 because the delegate action has no attached gas. This gas price is then passed on to the inner actions receipt produced by the relayer. This circumvents the pessimistic price calculation that is meant to ensure that no receipt ever executed at a gas price lower than the current gas price, even if the gas price is increased on every block.

Prepaying all the gas upfront would avoid such trickiness. And it would be more in line with the existing bahaviour.

The protocol currently has two situations where gas is burnt later than when it is purchased.

  1. In general, action receipts are created in one block and executed in the next. In ActionResult, gas_burnt covers the cost for executing the current action and for creating new receipts without executing them. gas_used covers the full cost of executing the current action and all new action receipts. In other words, gas_used is gas_burnt + gas reserved for later execution.
    I don't see a reason why this model cannot be used here. The used_gas of the delegate action result after is has been unwrapped includes the usual exec fees of all created actions plus the gas burnt.
  2. For function calls, where the exact amount of gas is unknown at the time of gas purchasing, we attach gas. This is treated separately because the user defines this number and it is not fixed in the protocol. But in the ActionResult, the attached gas is still included in used_gas.
    For delegate actions, you could define attached gas to be determined by the inner actions, instead of an explicit field.

Unless there is a good reason for it, is is my opinion that we should avoid introducing the new concept of attached deposit that is used to buy gas a block later.

Instead, I suggest to prepay gas. Just include the gas as prepaid gas here.

pub fn get_prepaid_gas(&self) -> Gas {
match self {
Action::FunctionCall(a) => a.gas,
_ => 0,
}
}

The value for a delegate action should be the sum of all send+exec fees for inner actions. This should balance out used_gas for the new receipts, such that check_balance does no panic.

@ilblackdragon
Copy link
Member

Thanks for the implementation.

Few questions:

  • Why calling it publisher vs relayer per NEP?
  • What is the reason to add publish_id into all actions? The original NEP has the expectation that signer_id is matching relayer_id, and hence the refund will be automatically returned to signer per existing model.
  • Agree with @jakmeier re: on picture it's confusing as User signs a DelegateAction not a Transaction per your implementation. This leads to the question - in the NEP there is a relayer_id field to ensure that only specific relayer is able to send this transaction (in case there is any reliance in the actions on that), which is missing in your implementation.

@e-uleyskiy e-uleyskiy force-pushed the NEP-366 branch 2 times, most recently from 76ff37a to 1b52660 Compare August 31, 2022 21:44
@e-uleyskiy
Copy link
Author

Thank you for review!
@jakmeier, @ilblackdragon

  1. I've updated the diagram in the description.
  2. I've renamed publisher_id to relayer_id.

I will answer other questions a little bit later.

@jakmeier
Copy link
Contributor

jakmeier commented Sep 1, 2022

Thank you for review! @jakmeier, @ilblackdragon

1. I've updated the diagram in the description.

2. I've renamed `publisher_id` to `relayer_id`.

I will answer other questions a little bit later.

The new diagram looks awesome, thanks for updating it! I think then I am mostly on the same page as you are. I'm sure this will also help other folks to understand more quickly how this works.

So then let me share some more thoughts I has, specifically on the 3 troubles you listed.

The troubles we faced:

1. Gas cost and deposit should be refunded to Relayer (~Publisher~). By default, it's refunded to Signer and we haven't found a better way than to add the "relayer_id" field to the Receipt for refund.

That's a bit unfortunate but I don't see any good ways out. Adding the field seems like the best option to me. But here are two ideas for how we could avoid it.

We could just refund to the signer (Alice) and argue that Alice has purchased the full gas amount from the relayer and therefore is allowed to spend it all or receive the refund. That would allow us to not add the relayer_id field. But I am worried that this makes it tricky to implement free (as in gratis) relayers as it opens the door to profitable abuse.

Another option would be to have no refunds at all for delegated actions. The details are a bit more tricky but basically we could be implement it by setting the gas price on the receipt to zero. When creating it, we would pay the (pessimistic) gas price at the time but when refunds are created, it will always be zero and no refunds are created. Obviously, this is financially unpalatable for the relayer and the user. But from a network's perspective, this could be the premium one has to pay for delegation.

2. Storage for the nonce for an implicit account. An implicit account doesn't have a stored access key in blockchain so we can't store/update nonce. We tried the solve this issue in the following way:
   (1) Relayer creates a transaction with "transfer" and "delegate" actions, (2) "transfer" action creates an account, (3) nonce is verified while "delegate" action is processing.
   It didn't work because if "transfer" causes the creation of the account, then there should be only a "transfer" action
   If you have any ideas on how to solve this issue, please, share.

Could we mandate that implicit accounts must first be created before they can be used in delegated actions?

3. Borsh doesn't support recursive data types. As a workaround, we deserialize the DelegateAction structure just in place. There is a pull request to Borsh which partially solve this issue. I hope it will be merged soon.

The way we use delegated actions, it does not have to be recursive anyway. It only happens in the type system because we want to make it look like a regular action, while it actually is not. I wonder, perhaps DelegateAction should not become an action but instead a new type of receipt? E.g.

pub enum ReceiptEnum {
    Action(ActionReceipt),
    Delegate(DelegateActionReceipt),
    Data(DataReceipt),
}

This might require larger refactoring. But it ensures in the type system that no delegation action is hidden inside another action, so we would not have to add checks for that everywhere.

Maybe this is going a bit too far but I am curious to hear your opinion.

@jakmeier
Copy link
Contributor

jakmeier commented Sep 1, 2022

  • What is the reason to add publish_id into all actions? The original NEP has the expectation that signer_id is matching relayer_id, and hence the refund will be automatically returned to signer per existing model.

Giving more thoughts to this, it seems to me that this cleanly resolves the refund issue.

@fadeevab
Copy link

fadeevab commented Sep 1, 2022

@jakmeier Hi! Thank you for your review! I'm assisting @e-uleyskiy with the Pull Request, and I'd like to add a few notes to what you have already said about signer_id vs relayer_id.

  1. I think there is quite a problem in how signer_id can be misused by developers.

    There is already some confusion about how to correctly utilize signer_account_id, and making a relayer to be a signer will add more confusion.
    For instance, I can think about cases when signer_account_id can be used to figure out whether the predecessor is a smart contract (having a cross-contract call). In the case of a meta transaction, neither party is a smart contract, however, it could be misinterpreted.

  2. There is a conceptual issue also: basically, the Relayer is a signer, and also Alice is a signer - Alice signs the delegated action, Relayer signs the final transaction. Both are signers with different roles, it's better to not mixing up them into the single identifier "signer_id".

  3. A final thought is that there could be a benefit in introducing relayer_id: maybe, knowing a relayer, knowing it is a meta transaction, a smart contract could introduce specific discounts, interest rate calculation, or another special logic.

@e-uleyskiy
Copy link
Author

e-uleyskiy commented Sep 1, 2022

@jakmeier

The new diagram looks awesome, thanks for updating it!

Thanks @fadeevab! He is an author :)

The troubles we faced:

  1. Gas cost and deposit should be refunded to Relayer (Publisher). By default, it's refunded to Signer and we haven't >>found a better way than to add the "relayer_id" field to the Receipt for refund.

That's a bit unfortunate but I don't see any good ways out. Adding the field seems like the best option to me. But here are two ideas for how we could avoid it.

Thank you for ideas!

  1. Storage for the nonce for an implicit account. An implicit account doesn't have a stored access key in blockchain so we can't store/update nonce. We tried the solve this issue in the following way:
    (1) Relayer creates a transaction with "transfer" and "delegate" actions, (2) "transfer" action creates an account, (3) nonce is verified while "delegate" action is processing.
    It didn't work because if "transfer" causes the creation of the account, then there should be only a "transfer" action
    If you have any ideas on how to solve this issue, please, share.

Could we mandate that implicit accounts must first be created before they can be used in delegated actions?

I think, no. We want to give an opportunity for new users (implicit accounts) to pay for transaction with Fungible Tokens. It should simplify onboarding in mind. There are projects where users might have FT but never have NEAR. In such case, users can't create an account because they don't have NEAR and can't exchange FT to NEAR because they need NEAR for this.

If we could transfer NEAR to an implicit account and do DelegateAction in same transaction, it would solve the issue.
There is the following comment in code:

// OK. It's implicit account creation.
// Notes:
// - The transfer action has to be the only action in the transaction to avoid
// abuse by hijacking this account with other public keys or contracts.

Which attack can be implemented if we allow this? I thought, an attacker could add its own key or deploy a contract to the receiver account but these were checked in check_actor_permissions. Maybe I missed something.

  1. Borsh doesn't support recursive data types. As a workaround, we deserialize the DelegateAction structure just in place. >>There is a pull request to Borsh which partially solve this issue. I hope it will be merged soon.

The way we use delegated actions, it does not have to be recursive anyway. It only happens in the type system because we >want to make it look like a regular action, while it actually is not. I wonder, perhaps DelegateAction should not become an action >but instead a new type of receipt?

Thank you for the idea. The same problem is present in Transactiion. Transaction is a part of client API. As I understand, we can't change field's type or add new fields because it will break backwards compatibility. Actually, I used that workaround because of Transaction

   What is the reason to add publish_id into all actions? The original NEP has the expectation that signer_id is matching >>relayer_id, and hence the refund will be automatically returned to signer per existing model.

Giving more thoughts to this, it seems to me that this cleanly resolves the refund issue.

You are right, this resolves the refund issue. But there are several caveats with this:

  1. If we use signer_id for relayer_id, we change meaning of signer_id. At this moment it's the initiator of the transaction and signer_id != predecessor_id means a cross-contract call but in our case this is a delegate action.
  2. Developers are already using signer_id for some purposes. I saw, that NFT marketplaces used it for tracking an owner of NFT. In this case, users will not be able trade NFT using Delegate action.
  3. Seems, this is a breaking change and all contracts should be reviewed and updated. And I'm concerned that this might add security issues to the some projects if developers miss an announce of this feature.

@ilblackdragon
Copy link
Member

ilblackdragon commented Sep 1, 2022

  1. Storage for the nonce for an implicit account. An implicit account doesn't have a stored access key in blockchain so we can't store/update nonce. We tried the solve this issue in the following way:
    (1) Relayer creates a transaction with "transfer" and "delegate" actions, (2) "transfer" action creates an account, (3) nonce is verified while "delegate" action is processing.
    It didn't work because if "transfer" causes the creation of the account, then there should be only a "transfer" action
    If you have any ideas on how to solve this issue, please, share.

Could we mandate that implicit accounts must first be created before they can be used in delegated actions?

This is great question that indeed NEP doesn't cover. At the same time implicit non-initialize accounts is def one of the most interesting use cases, e.g. allowing people to create a new wallet and start interacting with some app.

One way I can suggest to address this is to have a deposit attached by relayer that will be used for creation of account data. Only needed for the first time for implicit, and relayer can check if they should add deposit or not.

Deposit can be attached to the DelegateAction itself vs sending other separate action.

@ilblackdragon
Copy link
Member

ilblackdragon commented Sep 2, 2022

You are right, this resolves the refund issue. But there are several caveats with this:

If we use signer_id for relayer_id, we change meaning of signer_id. At this moment it's the initiator of the transaction and signer_id != predecessor_id means a cross-contract call but in our case this is a delegate action.
Developers are already using signer_id for some purposes. I saw, that NFT marketplaces used it for tracking an owner of NFT. In this case, users will not be able trade NFT using Delegate action.
Seems, this is a breaking change and all contracts should be reviewed and updated. And I'm concerned that this might add security issues to the some projects if developers miss an announce of this feature.

There is a constant recommendation to never use signer_id as an identifier of the caller. This is why there is predecessor_id. Multisig or DAOs won't work with contracts that are using signer_id. Similarly to Ethereum to not use tx.origin.

It's also already possible for user's transaction is been proxied via different signer, for example via https://github.com/ilblackdragon/near-eth-gateway (just a lot more awkward than current implementation).

For instance, I can think about cases when signer_account_id can be used to figure out whether the predecessor is a smart contract (having a cross-contract call). In the case of a meta transaction, neither party is a smart contract, however, it could be misinterpreted.

One must not do this, as every account on NEAR is smart contract.

There is a conceptual issue also: basically, the Relayer is a signer, and also Alice is a signer - Alice signs the delegated action, Relayer signs the final transaction. Both are signers with different roles, it's better to not mixing up them into the single identifier "signer_id".

Although I see there is a possible confusion, but the suggested design offers a lot of flexibility that needs to be accounted for. For example one can record a signature of the user in the smart contract and execute it later. In this case the DelegateAction is actually coming from a smart contract where it was recorded, and the tx originator (i.e. signer) will be a croncat agent. The simplest use case here is that I preauthorize to withdraw unstaked assets for me and it needs to be executed in 48 hours.

Signer has a very special meaning in NEAR to validate access key usage (e.g. to verify the tx was signed by an access key of this account). Everything else is a misuse of a signer.

If you know examples that misuse signer_id - we should surface that as potential issues to the contract maintainers.

core/primitives/src/transaction.rs Outdated Show resolved Hide resolved
pub struct SignedDelegateAction {
// Borsh doesn't support recursive types. Therefore this field
// is deserialized to DelegateAction in runtime
pub delegate_action_serde: Vec<u8>,
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand why not just use delegate_action: DelegateAction here?
See SignedTransaction for example -

pub transaction: Transaction,

You can add borsh_init to initialize whatever data is needed.

Copy link
Author

@e-uleyskiy e-uleyskiy Sep 9, 2022

Choose a reason for hiding this comment

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

I don't fully understand why not just use delegate_action: DelegateAction here?
The comment is wrong. The issue is that there will be a type recursion:

pub struct Transaction {
   ....
   pub actions: Vec<Action>, // <--- Recursion
}

pub enum Action {
   ...
   Delegate(SignedDelegateAction),
}

pub struct SignedDelegateAction {
   pub delegate_action: DelegateAction,
   ....
}

pub struct DelegateAction {
   ...
   pub actions: Vec<Action>, /// <--- Recursion
}

#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))]
#[derive(BorshSerialize, BorshDeserialize, Serialize, Deserialize, PartialEq, Eq, Clone, Debug)]
pub struct DelegateAction {
pub reciever_id: AccountId,
Copy link
Member

Choose a reason for hiding this comment

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

receiver_id is who the actions in this Action will be targeted toward after it has been delegated.
Think of DelegateAction/SignedDelegateAction as Transaction and SignedTransaction respectively.

Adding description comment to DelegateAction with a link to NEP and moving diagram there will be useful indeed.

@jakmeier
Copy link
Contributor

jakmeier commented Sep 5, 2022

@e-uleyskiy @fadeevab Thank you for your answers! I think there are now 2 design questions open.

1. Relayer ID as signer id or not

I am not familiar with current usage patterns, nor the originally intended semantics behind signer_id. Personally I would like to not add the relayer ID field to receipts but I see no technical impossibility in doing it.

I would say the pros and cons if this should be discussed and decided on the NEP, not as part of the draft implementation.

2. Implicit accounts that don't exist previously

If we could transfer NEAR to an implicit account and do DelegateAction in same transaction, it would solve the issue. There is the following comment in code:

// OK. It's implicit account creation.
// Notes:
// - The transfer action has to be the only action in the transaction to avoid
// abuse by hijacking this account with other public keys or contracts.

Which attack can be implemented if we allow this? I thought, an attacker could add its own key or deploy a contract to the receiver account but these were checked in check_actor_permissions. Maybe I missed something.

The reasoning for this is described here: near/NEPs#71 (comment)
For context, when creating a non-implicit account, the actor_id is overwritten and all subsequent actions are executed on the behalf of the new account. This is commonly used to create an account and deploy access keys in the same transaction.
This means that the check in check_actor_permissions would pass if we do the same for implicit account creation. I don't know why the solution chosen was to disallow all extra actions in the transaction instead of treating implicit account creations differently and just not update the actor_id. Maybe there are some important reasons there that I missed.
But in any case, since the transfer would already be signed by the implicit account's key, I see nothing wrong with allowing delegated actions to create an implicit account and continue execution on behalf of that account.

On the other hand, I'm still not convinced that we cannot mandate that the account must exist. I do understand that implicit accounts are the main intended use case. But why does this use case need to create the account in the same transaction? The relayer can first check if the account already exists. If not, it first creates the account and only releases the delegated action afterwards.


Before resolving the two questions above, there is not much value bikeshedding the implementation details further. But I hope my inputs on this have been helpful.

@akhi3030 and I decided that it is too early for us (Pagoda's protocol team) to track every step of this at this stage. This simply means I will not follow updates on this PR unless pinged. If you have more questions regarding the technical implementation, or if I forgot to answer any of your questions, please ping me. Otherwise, I will do a full review of the code once the implementation is complete and aligned with the NEP.

Egor Ulesykiy added 2 commits October 28, 2022 01:51
* Removed published_id
* Added sender_id to DelegateAction
* All things are verified when action is processing except verification
  of DelegateAction count in the transaction and verification wheher
  DelegateAction contains the nested ones
* Added block_hash, nonce, public_key fields to DelegateAction
* Gas is refunded to Transaction signer, deposit is refund to Receipt
  predecessor (because publisher_id has been removed)

Need to be done:
1. `block_hash` isn't verified because there is not access to Store from
   `runtime'
2. Need to apply recommendation to `action_array_serde`: https://near.zulipchat.com/#narrow/stream/295302-general/topic/recursive.20types.20in.20borsh-rs/near/305181116
3. Need to add unit tests
@e-uleyskiy
Copy link
Author

e-uleyskiy commented Oct 27, 2022

I updated the patch according to the discussion in NEP-366
Need to be done:

  1. block_hash isn't verified because there is not access to Store from
    `runtime'
  2. Need to apply recommendation to action_array_serde: https://near.zulipchat.com/#narrow/stream/295302-general/topic/recursive.20types.20in.20borsh-rs/near/305181116
  3. Need to add unit tests
  • There are test fees in code.

Should I update the description?

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

Hey @e-uleyskiy, I am really happy to see progress on this! Your changes are looking great.

I left a comment on the NEP regarding block hash, I think it makes sense to discuss that there.

For the implementation, I added a few unimportant small comments on coding styles etc and one big comment around gas fees. I intentionally avoided this topic so far as we had larger design questions to clarify first. But now we really have to look at that as well.

Let me know how I can help if you have questions or concerns.


// DelegateAction shouldn't contain a nested DelegateAction
if actions.iter().any(|a| matches!(a, Action::Delegate(_))) {
return Err(ActionsValidationError::TotalNumberOfActionsExceeded {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return Err(ActionsValidationError::TotalNumberOfActionsExceeded {
return Err(ActionsValidationError::DelegateActionCantContainNestedOne {

runtime/runtime/src/actions.rs Outdated Show resolved Hide resolved
runtime/runtime/src/actions.rs Outdated Show resolved Hide resolved
})
}

pub fn validate_access_key(
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be pub? (and consider renaming to make it clear that it's specific for delegate actions)


let delegate_actions = signed_delegate_action.delegate_action.get_actions();
if let Ok(delegate_actions) = &delegate_actions {
validate_actions(limit_config, &delegate_actions)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this check redundant? We call validate_action a bit further down which calls validate_delegate_action which calls validate_actions again.

@@ -120,6 +120,7 @@ pub fn total_send_fees(
},
DeleteKey(_) => cfg.delete_key_cost.send_fee(sender_is_receiver),
DeleteAccount(_) => cfg.delete_account_cost.send_fee(sender_is_receiver),
Delegate(_) => cfg.delegate_cost.send_fee(sender_is_receiver),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's about time we start talking about gas fees.

There are 3 places where validate_actions loops through all inner actions and validates them.

  1. When validating the transaction on the relayer shard.
  2. When validating the receipt with the delegate action at Alice's shard.
  3. When validating the final receipt at FT's shard.

The send fee of an action is meant to cover the cost of validation at the sender side and the conversion to a receipt. The exec fee covers validation and execution of the action on the receiver. But here we have a step in between, which makes things a bit more complicated. One approach would be to pay the send fee for inner actions twice, in step 1 & 2 but this raises the cost of an FT by 2.3 Tgas, almost 50%. Not sure if we have a better option though.

If I got this right, the current implementation does not include the send cost of inner actions at all. When the tx_cost is computed in step 1, the inner actions' send fees are not added, only the constant send fee for delegate action itself. And in step 2, when the receipt is created, the prepaid exec fees for inner actions is calculated and added to used gas, which is correct, but the send fees are not calculated and burnt as they should be.

Do I make sense? And have you already thought about this?

Copy link
Author

Choose a reason for hiding this comment

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

Do I make sense? And have you already thought about this?

I thought about it and I've just realized that I misunderstood the purposes of send and exec fees.
I didn't include send fee because I thought the inner actions' fee should be included only to exec fee (they are converted on the receiver side therefore it should be include to exec fee). That was my idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it's easy to think like you did. It's also not documented all that well. But fact is, for normal transaction we charge send and exec fees for all actions included and at the very least we have to do the same for delegated actions.

I would suggest to charge the send fee twice for delegated actions. But if you have other suggestions I am happy to discuss them.

@e-uleyskiy
Copy link
Author

@jakmeier

It's probably about time to manually merge your changes with the master branch

Done

Other than that, we just have to get passed CI.

Done

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

This seems complete and ready to merge to me! 🎉 Thank you @e-uleyskiy for your persistence getting this so far.

I left two minor comments for things I just saw while going briefly through the full PR again. But otherwise we should now wait for the NEP decision on Monday. When we have green light from there, we can have someone additional review this and right after that we can finally merge.

runtime/runtime/src/actions.rs Show resolved Hide resolved
runtime/runtime/src/actions.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

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

@e-uleyskiy Well first, congratulation on squashing all doubts that this is the right first step for meta transactions! 🎉 I'm really excited to see the progress here.

Next, we should try to merge this soon. Once it is on nearcore master, the efforts by the integration team at Pagoda and other tooling around meta transaction can begin.

I just did a final line-by-line review and it looks all good to me on the technical side. I left a few comments to improve code readability, for the benefit of other code reviewers now and in the future. If you could address those whenever you find the time, I'd appreciate it.

Of course, there is also the issue around signature domains. I would suggest we address it in follow-up PRs, I already added it to the list in #8075.

@mm-near I think we need a second reviewer here before we hit merge. I've been looking at this so many time now, I probably developed some blind spots by now. Can you either delegate to someone or review it yourself?

runtime/runtime/src/actions.rs Outdated Show resolved Hide resolved
runtime/runtime/src/actions.rs Show resolved Hide resolved
runtime/runtime/src/config.rs Show resolved Hide resolved
runtime/runtime/src/config.rs Outdated Show resolved Hide resolved
@bowenwang1996
Copy link
Collaborator

@mm-near I think we need a second reviewer here before we hit merge. I've been looking at this so many time now, I probably developed some blind spots by now. Can you either delegate to someone or review it yourself?

@mm-near could you review this PR so that we can merge this PR? Thanks!

}

impl DelegateAction {
pub fn get_actions(&self) -> Vec<Action> {
Copy link
Contributor

Choose a reason for hiding this comment

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

you should be able to avoid the clone by referencing the inner actions, no?

Copy link
Author

Choose a reason for hiding this comment

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

Yes I can return Vec<&Action> but there is no sense, I think. There are only three cases when get_actions is used:

  1. When the returned Vec<Action> is passed to another function in config.rs as an &[Action] argument. So cloning &Action is required in this case.
  2. When a new receipt is created in actions.rs. In this case, we have to copy Vec<Action> in any case.
  3. In validate_delegate_action_key. This is the only case when Vec<&Action> would be helpful.

Comment on lines +1053 to +1056
Delegate {
delegate_action: DelegateAction,
signature: Signature,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

curious why some parts of the PR are gated behind the feature flag and not some things like this. Is this not an inconsistency or is there a reason for example the views here are consistent regardless of protocol feature?

Copy link
Author

Choose a reason for hiding this comment

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

I tried to wrap the new code in the feature flag but this requires a lot of changes. Therefore I decided to return an error when SignedDelegateAction is submitted and manage it with the feature flag. I mentioned this in the comment

let mut iter = actions.iter().peekable();
while let Some(action) = iter.next() {
if let Action::DeleteAccount(_) = action {
if iter.peek().is_some() {
return Err(ActionsValidationError::DeleteActionMustBeFinal);
}
} else if let Action::Delegate(_) = action {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cross-posting this comment to track: near/NEPs#366 (comment)

This behavior seems unclear and inconsistent to me.

Copy link
Author

Choose a reason for hiding this comment

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

I answered in the comment

@mm-near mm-near self-requested a review December 19, 2022 14:57
@jakmeier jakmeier mentioned this pull request Dec 20, 2022
4 tasks
@@ -84,6 +84,11 @@ expression: store.get_config(*version)
"send_sir": 147489000000,
"send_not_sir": 147489000000,
"execution": 147489000000
},
"delegate_cost": {
Copy link
Contributor

Choose a reason for hiding this comment

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

@jakmeier - are these changes expected? shouldn't the delegate cost be from the 'next' release onwards only?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, expected, due to Serde we need these values also in old version (maybe one day we can get away from the JSON representation all together, then we could avoid this)

@@ -410,6 +410,7 @@ impl From<NearActions> for Vec<crate::models::Operation> {
);
operations.push(deploy_contract_operation);
}
near_primitives::transaction::Action::Delegate(_) => todo!(),
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO it should be fixed before submitting (otherwise this code path could cause panic in the node)

@@ -48,12 +48,14 @@ protocol_feature_reject_blocks_with_outdated_protocol_version = []
protocol_feature_ed25519_verify = [
"near-primitives-core/protocol_feature_ed25519_verify"
]
protocol_feature_delegate_action = []
Copy link
Contributor

Choose a reason for hiding this comment

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

small preference to add the nep_366 to the name (makes it a lot easier to find in the future).

@@ -66,6 +66,10 @@ action_add_function_call_key_per_byte_execution: 1_925_331
action_delete_key_send_sir: 94_946_625_000
action_delete_key_send_not_sir: 94_946_625_000
action_delete_key_execution: 94_946_625_000
# TODO: place-holder values, needs estimation, tracked in #8114
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have the correct values now, right @jakmeier

Copy link
Contributor

Choose a reason for hiding this comment

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

No, not yet. So far I have only estimated all other actions with respect to meta transaction. (I ensured that charging the SEND cost twice cost is safe. A possible alternative would have been to introduce new costs for each action called META_SEND or something like that.)

I will need this to be merged to master before I can do a proper estimation of the new cost. The coding part will be quick but doing a clean experiment that spits out useful numbers is a bit more involved. I'd rather not do it on forks of PRs that are still changing. So I am waiting for this to be merged before I do the final estimation for the new parameters.

@@ -198,6 +198,10 @@ pub enum ActionsValidationError {
UnsuitableStakingKey { public_key: PublicKey },
/// The attached amount of gas in a FunctionCall action has to be a positive number.
FunctionCallZeroAttachedGas,
/// DelegateAction actions contain another DelegateAction
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd mention "which is not allowed"

Ok(())
}

fn receipt_required_gas(apply_state: &ApplyState, receipt: &Receipt) -> Result<Gas, RuntimeError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

comment

})
}

fn validate_delegate_action_key(
Copy link
Contributor

Choose a reason for hiding this comment

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

comment

Copy link
Contributor

Choose a reason for hiding this comment

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

also - shouldn't this be in src/verifiers.rs?

Copy link
Author

@e-uleyskiy e-uleyskiy Jan 16, 2023

Choose a reason for hiding this comment

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

May be it should. But it returns ActionErrorKind errors.

result: &mut ActionResult,
) -> Result<(), RuntimeError> {
// 'delegate_action.sender_id' account existence must be checked by a caller
let mut access_key = match get_access_key(
Copy link
Contributor

Choose a reason for hiding this comment

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

line 726 -> 762 is very similar to code in verify_and_charge_transaction method. (except that here we return a little bit different error messages). I wonder if we should somehow combine the two (might be done in separate pr)


if let AccessKeyPermission::FunctionCall(ref function_call_permission) = access_key.permission {
if actions.len() != 1 {
result.result = Err(ActionErrorKind::DelegateActionAccessKeyError(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused -- if there is more than 1 action we return 'invalid access key'? Can you explain in a comment?

Copy link
Author

@e-uleyskiy e-uleyskiy Jan 12, 2023

Choose a reason for hiding this comment

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

This part of code is similar to the one in verify_and_charge_transaction. I hope this behavior is documented somewhere (access key permissions) because I can't find this in the documentation :) This is a restriction of "function call" access keys. The transaction must contain the only FunctionCall if "function call" access key is used

return Ok(());
}
} else {
result.result = Err(ActionErrorKind::DelegateActionAccessKeyError(
Copy link
Contributor

Choose a reason for hiding this comment

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

comment under when does this happen (when action is not a function call, right?)

@e-uleyskiy
Copy link
Author

e-uleyskiy commented Jan 16, 2023

@mm-near
@jakmeier
I fixed the most of comments except implementation of serde::de::Deserialize. let me know, if missed something else.
About serde::de::Deserialize. If somebody from your team could implement it quickly, it would be good. Because I have some lack of time.

@jakmeier
Copy link
Contributor

As discussed with @mm-near I just created a new PR #8385 from a near/nearcore branch where we can also push changes.
Final adjustments until we are comfortable merging this will be added there.

near-bulldozer bot pushed a commit that referenced this pull request Jan 19, 2023
A fork of #7497 

Nearcore code owners currently don't have permissions to change the
original PR, so we will add the necessary changes here before merging.

This is also merged with the current master.
@jakmeier
Copy link
Contributor

This has been merged from #8385 🎉

Thanks again @e-uleyskiy for the implementations and @fadeevab for the design!
(I am really sorry that you are not listed as the authors in git history. I really thought Github adds you as authors when it squashes commits in the PR but apparently it did not...)

@jakmeier jakmeier closed this Jan 19, 2023
@e-uleyskiy
Copy link
Author

Thanks @jakmeier for review!
Thanks @fadeevab for review and helping with design!

nikurt pushed a commit to nikurt/nearcore that referenced this pull request Jan 30, 2023
A fork of near#7497 

Nearcore code owners currently don't have permissions to change the
original PR, so we will add the necessary changes here before merging.

This is also merged with the current master.
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.

8 participants