-
Notifications
You must be signed in to change notification settings - Fork 619
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
Changes from 2 commits
ce474eb
3eb7f02
433b158
a7bcf89
529ca2c
cfb9780
602baca
3b78721
106dc67
ee124a6
11c29fc
b344298
d6ef8ba
df86e3a
59a5986
cd289d4
9a9c0da
f34acda
704c524
88973ce
335537e
7575c90
010d718
1935499
79915fd
155054f
81f4eb7
d0c3787
e9e1f8f
d5cb1a0
c9eb505
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,7 @@ use crate::borsh::maybestd::collections::HashMap; | |
use crate::hash::CryptoHash; | ||
use crate::logging; | ||
use crate::serialize::{dec_format, option_base64_format}; | ||
use crate::transaction::{Action, TransferAction}; | ||
use crate::transaction::{Action, DelegateAction, TransferAction}; | ||
use crate::types::{AccountId, Balance, ShardId}; | ||
|
||
/// Receipts are used for a cross-shard communication. | ||
|
@@ -52,6 +52,7 @@ impl Receipt { | |
|
||
receipt: ReceiptEnum::Action(ActionReceipt { | ||
signer_id: "system".parse().unwrap(), | ||
relayer_id: None, | ||
signer_public_key: PublicKey::empty(KeyType::ED25519), | ||
gas_price: 0, | ||
output_data_receivers: vec![], | ||
|
@@ -79,6 +80,7 @@ impl Receipt { | |
|
||
receipt: ReceiptEnum::Action(ActionReceipt { | ||
signer_id: receiver_id.clone(), | ||
relayer_id: None, | ||
signer_public_key, | ||
gas_price: 0, | ||
output_data_receivers: vec![], | ||
|
@@ -87,6 +89,30 @@ impl Receipt { | |
}), | ||
} | ||
} | ||
|
||
pub fn new_delegate_actions( | ||
jakmeier marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add comment - and explain why do we need a special function for this case. |
||
relayer_id: &AccountId, | ||
predecessor_id: &AccountId, | ||
delegate_action: &DelegateAction, | ||
public_key: &PublicKey, | ||
gas_price: Balance, | ||
) -> Self { | ||
Receipt { | ||
predecessor_id: predecessor_id.clone(), | ||
receiver_id: delegate_action.reciever_id.clone(), | ||
receipt_id: CryptoHash::default(), | ||
|
||
receipt: ReceiptEnum::Action(ActionReceipt { | ||
signer_id: predecessor_id.clone(), | ||
relayer_id: Some(relayer_id.clone()), | ||
signer_public_key: public_key.clone(), | ||
gas_price: gas_price, | ||
output_data_receivers: vec![], | ||
input_data_ids: vec![], | ||
actions: delegate_action.actions.clone(), | ||
}), | ||
} | ||
} | ||
} | ||
|
||
/// Receipt could be either ActionReceipt or DataReceipt | ||
|
@@ -103,6 +129,8 @@ pub enum ReceiptEnum { | |
pub struct ActionReceipt { | ||
/// A signer of the original transaction | ||
pub signer_id: AccountId, | ||
/// A relayer's identifier in case of a delgated action | ||
pub relayer_id: Option<AccountId>, | ||
/// An access key which was used to sign the original transaction | ||
pub signer_public_key: PublicKey, | ||
/// A gas_price which has been used to buy gas in the original transaction | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -1,6 +1,7 @@ | ||||
use std::borrow::Borrow; | ||||
use std::fmt; | ||||
use std::hash::{Hash, Hasher}; | ||||
use std::io::Error; | ||||
|
||||
use borsh::{BorshDeserialize, BorshSerialize}; | ||||
use serde::{Deserialize, Serialize}; | ||||
|
@@ -70,6 +71,7 @@ pub enum Action { | |||
AddKey(AddKeyAction), | ||||
DeleteKey(DeleteKeyAction), | ||||
DeleteAccount(DeleteAccountAction), | ||||
Delegate(SignedDelegateAction), | ||||
} | ||||
|
||||
impl Action { | ||||
|
@@ -83,6 +85,10 @@ impl Action { | |||
match self { | ||||
Action::FunctionCall(a) => a.deposit, | ||||
Action::Transfer(a) => a.deposit, | ||||
Action::Delegate(a) => { | ||||
let delegate_action = a.get_delegate_action().unwrap(); | ||||
delegate_action.deposit | ||||
} | ||||
_ => 0, | ||||
} | ||||
} | ||||
|
@@ -220,6 +226,40 @@ impl From<DeleteAccountAction> for Action { | |||
} | ||||
} | ||||
|
||||
#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))] | ||||
#[derive(BorshSerialize, BorshDeserialize, Serialize, Deserialize, PartialEq, Eq, Clone, Debug)] | ||||
pub struct DelegateAction { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. comment |
||||
pub reciever_id: AccountId, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Adding description comment to DelegateAction with a link to NEP and moving diagram there will be useful indeed. |
||||
pub deposit: Balance, | ||||
pub nonce: u64, | ||||
pub actions: Vec<Action>, | ||||
} | ||||
#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))] | ||||
#[derive(BorshSerialize, BorshDeserialize, Serialize, Deserialize, PartialEq, Eq, Clone, Debug)] | ||||
pub struct SignedDelegateAction { | ||||
// Borsh doesn't support recursive types. Therefore this field | ||||
// is deserialized to DelegateAction in runtime | ||||
pub delegate_action_serde: Vec<u8>, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? nearcore/core/primitives/src/transaction.rs Line 227 in 62f6634
You can add borsh_init to initialize whatever data is needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||
pub public_key: PublicKey, | ||||
pub signature: Signature, | ||||
jakmeier marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
} | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Some comments on |
||||
|
||||
impl SignedDelegateAction { | ||||
pub fn get_delegate_action(&self) -> Result<DelegateAction, Error> { | ||||
DelegateAction::try_from_slice(&self.delegate_action_serde) | ||||
} | ||||
|
||||
pub fn get_hash(&self) -> CryptoHash { | ||||
hash(&self.delegate_action_serde) | ||||
} | ||||
} | ||||
|
||||
impl From<SignedDelegateAction> for Action { | ||||
fn from(delegate_action: SignedDelegateAction) -> Self { | ||||
Self::Delegate(delegate_action) | ||||
} | ||||
} | ||||
|
||||
#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))] | ||||
#[derive(BorshSerialize, BorshDeserialize, Serialize, Deserialize, Eq, Debug, Clone)] | ||||
#[borsh_init(init)] | ||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -11,7 +11,7 @@ use near_primitives::runtime::config::AccountCreationConfig; | |||||||||||||
use near_primitives::runtime::fees::RuntimeFeesConfig; | ||||||||||||||
use near_primitives::transaction::{ | ||||||||||||||
Action, AddKeyAction, DeleteAccountAction, DeleteKeyAction, DeployContractAction, | ||||||||||||||
FunctionCallAction, StakeAction, TransferAction, | ||||||||||||||
FunctionCallAction, SignedDelegateAction, StakeAction, TransferAction, | ||||||||||||||
}; | ||||||||||||||
use near_primitives::types::validator_stake::ValidatorStake; | ||||||||||||||
use near_primitives::types::{AccountId, BlockHeight, EpochInfoProvider, TrieCacheMode}; | ||||||||||||||
|
@@ -30,6 +30,7 @@ use near_vm_errors::{ | |||||||||||||
use near_vm_logic::types::PromiseResult; | ||||||||||||||
use near_vm_logic::VMContext; | ||||||||||||||
|
||||||||||||||
use crate::balance_checker::receipt_cost; | ||||||||||||||
use crate::config::{safe_add_gas, RuntimeConfig}; | ||||||||||||||
use crate::ext::{ExternalError, RuntimeExt}; | ||||||||||||||
use crate::{ActionResult, ApplyState}; | ||||||||||||||
|
@@ -253,6 +254,7 @@ pub(crate) fn action_function_call( | |||||||||||||
receipt_id: CryptoHash::default(), | ||||||||||||||
receipt: ReceiptEnum::Action(ActionReceipt { | ||||||||||||||
signer_id: action_receipt.signer_id.clone(), | ||||||||||||||
relayer_id: action_receipt.relayer_id.clone(), | ||||||||||||||
signer_public_key: action_receipt.signer_public_key.clone(), | ||||||||||||||
gas_price: action_receipt.gas_price, | ||||||||||||||
output_data_receivers: receipt.output_data_receivers, | ||||||||||||||
|
@@ -613,6 +615,46 @@ pub(crate) fn action_add_key( | |||||||||||||
Ok(()) | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
pub(crate) fn action_delegate_action( | ||||||||||||||
apply_state: &ApplyState, | ||||||||||||||
action_receipt: &ActionReceipt, | ||||||||||||||
predecessor_id: &AccountId, | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 |
||||||||||||||
signed_delegate_action: &SignedDelegateAction, | ||||||||||||||
result: &mut ActionResult, | ||||||||||||||
) -> Result<(), RuntimeError> { | ||||||||||||||
match signed_delegate_action.get_delegate_action() { | ||||||||||||||
Ok(delegate_action) => { | ||||||||||||||
let new_receipt = Receipt::new_delegate_actions( | ||||||||||||||
&action_receipt.signer_id, | ||||||||||||||
predecessor_id, | ||||||||||||||
&delegate_action, | ||||||||||||||
&signed_delegate_action.public_key, | ||||||||||||||
action_receipt.gas_price, | ||||||||||||||
); | ||||||||||||||
|
||||||||||||||
let transaction_costs = &apply_state.config.transaction_costs; | ||||||||||||||
let current_protocol_version = apply_state.current_protocol_version; | ||||||||||||||
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); | ||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason that we have to handle deposit refunds specially? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Why just do not pre-pay gas? Then I will play with Both variants are tricky and I chose the first one. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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. nearcore/core/primitives/src/transaction.rs Lines 76 to 81 in 36e3162
The value for a delegate action should be the sum of all send+exec fees for inner actions. This should balance out |
||||||||||||||
|
||||||||||||||
result.new_receipts.push(new_receipt); | ||||||||||||||
result.new_receipts.push(refund_receipt); | ||||||||||||||
} else { | ||||||||||||||
result.result = Err(ActionErrorKind::LackBalanceForState { | ||||||||||||||
account_id: action_receipt.signer_id.clone(), | ||||||||||||||
amount: cost.clone(), | ||||||||||||||
} | ||||||||||||||
.into()); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
Err(_) => todo!(), | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
Ok(()) | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
pub(crate) fn check_actor_permissions( | ||||||||||||||
action: &Action, | ||||||||||||||
account: &Option<Account>, | ||||||||||||||
|
@@ -645,7 +687,10 @@ pub(crate) fn check_actor_permissions( | |||||||||||||
.into()); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
Action::CreateAccount(_) | Action::FunctionCall(_) | Action::Transfer(_) => (), | ||||||||||||||
Action::CreateAccount(_) | ||||||||||||||
| Action::FunctionCall(_) | ||||||||||||||
| Action::Transfer(_) | ||||||||||||||
| Action::Delegate(_) => (), | ||||||||||||||
}; | ||||||||||||||
Ok(()) | ||||||||||||||
} | ||||||||||||||
|
@@ -720,6 +765,7 @@ pub(crate) fn check_account_existence( | |||||||||||||
.into()); | ||||||||||||||
} | ||||||||||||||
} | ||||||||||||||
Action::Delegate(_) => (), | ||||||||||||||
}; | ||||||||||||||
Ok(()) | ||||||||||||||
} | ||||||||||||||
|
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.
@jakmeier
Should this
todo
be fixed for commiting this patch or it can be done in another patch?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.
Let's keep this as a todo for the initial PR, this should not stop us from getting through the NEP approval.
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.
IMHO it should be fixed before submitting (otherwise this code path could cause panic in the node)
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 currently don't have a way to parse
DelegateAction
without the feature enabled. So it would only be possible to panic on nightly builds. But if you want to fix this before we merge, @firatNEAR told me they will take care of this one.