-
Notifications
You must be signed in to change notification settings - Fork 624
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 10 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 |
---|---|---|
|
@@ -200,6 +200,12 @@ pub enum ActionsValidationError { | |
UnsuitableStakingKey { public_key: PublicKey }, | ||
/// The attached amount of gas in a FunctionCall action has to be a positive number. | ||
FunctionCallZeroAttachedGas, | ||
/// The actions in DelegateAction are searilized incorrectly | ||
DelegateActionDeserializeError, | ||
/// DelegateAction actions contain another 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. I'd mention "which is not allowed" |
||
DelegateActionCantContainNestedOne, | ||
/// There should be the only one DelegateAction | ||
DelegateActionMustBeOnlyOne, | ||
} | ||
|
||
/// Describes the error for validating a receipt. | ||
|
@@ -317,6 +323,18 @@ impl Display for ActionsValidationError { | |
f, | ||
"The attached amount of gas in a FunctionCall action has to be a positive number", | ||
), | ||
ActionsValidationError::DelegateActionDeserializeError => write!( | ||
f, | ||
"Can't deserialize actions in DelegateAction" | ||
), | ||
ActionsValidationError::DelegateActionCantContainNestedOne => write!( | ||
f, | ||
"DelegateAction must not contain another DelegateAction" | ||
), | ||
ActionsValidationError::DelegateActionMustBeOnlyOne => write!( | ||
f, | ||
"The actions can contain the ony one DelegateAction" | ||
) | ||
} | ||
} | ||
} | ||
|
@@ -441,6 +459,20 @@ pub enum ActionErrorKind { | |
OnlyImplicitAccountCreationAllowed { account_id: AccountId }, | ||
/// Delete account whose state is large is temporarily banned. | ||
DeleteAccountWithLargeState { account_id: AccountId }, | ||
/// Signature does not match the provided actions and given signer public key. | ||
DelegateActionInvalidSignature, | ||
/// Receiver of the transaction doesn't match Sender of the delegate action | ||
DelegateActionSenderDoesNotMatchTxReceiver { sender_id: AccountId, receiver_id: AccountId }, | ||
/// Sender account doesn't exist | ||
DelegateActionSenderDoesNotExist { sender_id: AccountId }, | ||
/// Delegate action has expired. `max_block_height` is less than actual block height. | ||
DelegateActionExpired, | ||
/// The given public key doesn't exist for Sender account | ||
DelegateActionAccessKeyError(InvalidAccessKeyError), | ||
/// DelegateAction nonce must be greater sender[public_key].nonce | ||
DelegateActionInvalidNonce { delegate_nonce: Nonce, ak_nonce: Nonce }, | ||
/// DelegateAction nonce is larger than the upper bound given by the block height | ||
DelegateActionNonceTooLarge { delegate_nonce: Nonce, upper_bound: Nonce }, | ||
} | ||
|
||
impl From<ActionErrorKind> for ActionError { | ||
|
@@ -751,6 +783,13 @@ impl Display for ActionErrorKind { | |
ActionErrorKind::InsufficientStake { account_id, stake, minimum_stake } => write!(f, "Account {} tries to stake {} but minimum required stake is {}", account_id, stake, minimum_stake), | ||
ActionErrorKind::OnlyImplicitAccountCreationAllowed { account_id } => write!(f, "CreateAccount action is called on hex-characters account of length 64 {}", account_id), | ||
ActionErrorKind::DeleteAccountWithLargeState { account_id } => write!(f, "The state of account {} is too large and therefore cannot be deleted", account_id), | ||
ActionErrorKind::DelegateActionInvalidSignature => write!(f, "DelegateAction is not signed with the given public key"), | ||
ActionErrorKind::DelegateActionSenderDoesNotMatchTxReceiver { sender_id, receiver_id } => write!(f, "Transaction reciever {} doesn't match DelegateAction sender {}", sender_id, receiver_id), | ||
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. parameters in wrong order in the error message. Also - typo in receiver. |
||
ActionErrorKind::DelegateActionSenderDoesNotExist { sender_id } => write!(f, "DelegateAction sender account {} doesn't exist", sender_id), | ||
ActionErrorKind::DelegateActionExpired => write!(f, "DelegateAction has expired"), | ||
ActionErrorKind::DelegateActionAccessKeyError(access_key_error) => Display::fmt(&access_key_error, f), | ||
ActionErrorKind::DelegateActionInvalidNonce { delegate_nonce, ak_nonce } => write!(f, "DelegateAction nonce {} must be larger than nonce of the used access key {}", delegate_nonce, ak_nonce), | ||
ActionErrorKind::DelegateActionNonceTooLarge { delegate_nonce, upper_bound } => write!(f, "DelegateAction nonce {} must be smaller than the access key nonce upper bound {}", delegate_nonce, upper_bound), | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -87,6 +87,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. |
||
signer_id: &AccountId, | ||
predecessor_id: &AccountId, | ||
receiver_id: &AccountId, | ||
actions: &Vec<Action>, | ||
public_key: &PublicKey, | ||
gas_price: Balance, | ||
) -> Self { | ||
Receipt { | ||
predecessor_id: predecessor_id.clone(), | ||
receiver_id: receiver_id.clone(), | ||
receipt_id: CryptoHash::default(), | ||
|
||
receipt: ReceiptEnum::Action(ActionReceipt { | ||
signer_id: signer_id.clone(), | ||
signer_public_key: public_key.clone(), | ||
gas_price: gas_price, | ||
output_data_receivers: vec![], | ||
input_data_ids: vec![], | ||
actions: actions.clone(), | ||
}), | ||
} | ||
} | ||
} | ||
|
||
/// Receipt could be either ActionReceipt or DataReceipt | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,10 @@ | ||
use std::borrow::Borrow; | ||
use std::fmt; | ||
use std::hash::{Hash, Hasher}; | ||
use std::io::{Error, ErrorKind}; | ||
|
||
use borsh::{BorshDeserialize, BorshSerialize}; | ||
use near_primitives_core::types::BlockHeight; | ||
use serde::{Deserialize, Serialize}; | ||
|
||
use near_crypto::{PublicKey, Signature}; | ||
|
@@ -70,6 +72,7 @@ pub enum Action { | |
AddKey(AddKeyAction), | ||
DeleteKey(DeleteKeyAction), | ||
DeleteAccount(DeleteAccountAction), | ||
Delegate(SignedDelegateAction), | ||
} | ||
|
||
impl Action { | ||
|
@@ -220,6 +223,81 @@ impl From<DeleteAccountAction> for Action { | |
} | ||
} | ||
|
||
#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))] | ||
#[derive(Serialize, BorshSerialize, Deserialize, PartialEq, Eq, Clone, Debug)] | ||
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. we should also implement a custom Deserialize, and a custom constructor (and make the internal Action a non-public field) |
||
pub struct NonDelegateAction(Action); | ||
|
||
const ACTION_DELEGATE_NUMBER: u8 = 8; | ||
|
||
impl From<&NonDelegateAction> for Action { | ||
fn from(action: &NonDelegateAction) -> Self { | ||
action.0.clone() | ||
} | ||
} | ||
e-uleyskiy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
impl borsh::de::BorshDeserialize for NonDelegateAction { | ||
fn deserialize(buf: &mut &[u8]) -> ::core::result::Result<Self, borsh::maybestd::io::Error> { | ||
match buf[0] { | ||
e-uleyskiy marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ACTION_DELEGATE_NUMBER => Err(Error::new( | ||
ErrorKind::InvalidInput, | ||
"DelegateAction mustn't contain a nested one", | ||
)), | ||
_ => Ok(Self(borsh::BorshDeserialize::deserialize(buf)?)), | ||
} | ||
} | ||
} | ||
|
||
#[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 |
||
/// Signer of the delegated actions | ||
pub sender_id: AccountId, | ||
/// Receiver of the delegated actions. | ||
pub receiver_id: AccountId, | ||
/// List of actions to be executed. | ||
pub actions: Vec<NonDelegateAction>, | ||
/// Nonce to ensure that the same delegate action is not sent twice by a relayer and should match for given account's `public_key`. | ||
/// After this action is processed it will increment. | ||
pub nonce: Nonce, | ||
/// The maximal height of the block in the blockchain below which the given DelegateAction is valid. | ||
pub max_block_height: BlockHeight, | ||
/// Public key that is used to sign this delegated action. | ||
pub public_key: PublicKey, | ||
} | ||
#[cfg_attr(feature = "deepsize_feature", derive(deepsize::DeepSizeOf))] | ||
#[derive(BorshSerialize, BorshDeserialize, Serialize, Deserialize, PartialEq, Eq, Clone, Debug)] | ||
pub struct SignedDelegateAction { | ||
pub delegate_action: DelegateAction, | ||
pub signature: Signature, | ||
} | ||
|
||
impl SignedDelegateAction { | ||
pub fn verify(&self) -> bool { | ||
let delegate_action = &self.delegate_action; | ||
let hash = delegate_action.get_hash(); | ||
let public_key = &delegate_action.public_key; | ||
|
||
self.signature.verify(hash.as_ref(), public_key) | ||
} | ||
} | ||
|
||
impl From<SignedDelegateAction> for Action { | ||
fn from(delegate_action: SignedDelegateAction) -> Self { | ||
Self::Delegate(delegate_action) | ||
} | ||
} | ||
|
||
impl DelegateAction { | ||
pub fn get_actions(&self) -> Vec<Action> { | ||
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. you should be able to avoid the clone by referencing the inner actions, no? 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. Yes I can return
|
||
self.actions.iter().map(|a| a.into()).collect() | ||
} | ||
|
||
pub fn get_hash(&self) -> CryptoHash { | ||
let bytes = self.try_to_vec().expect("Failed to deserialize"); | ||
hash(&bytes) | ||
} | ||
} | ||
|
||
#[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 |
---|---|---|
|
@@ -32,9 +32,10 @@ use crate::sharding::{ | |
ShardChunkHeaderV3, | ||
}; | ||
use crate::transaction::{ | ||
Action, AddKeyAction, CreateAccountAction, DeleteAccountAction, DeleteKeyAction, | ||
DeployContractAction, ExecutionMetadata, ExecutionOutcome, ExecutionOutcomeWithIdAndProof, | ||
ExecutionStatus, FunctionCallAction, SignedTransaction, StakeAction, TransferAction, | ||
Action, AddKeyAction, CreateAccountAction, DelegateAction, DeleteAccountAction, | ||
DeleteKeyAction, DeployContractAction, ExecutionMetadata, ExecutionOutcome, | ||
ExecutionOutcomeWithIdAndProof, ExecutionStatus, FunctionCallAction, SignedDelegateAction, | ||
SignedTransaction, StakeAction, TransferAction, | ||
}; | ||
use crate::types::{ | ||
AccountId, AccountWithPublicKey, Balance, BlockHeight, CompiledContractCache, EpochHeight, | ||
|
@@ -954,6 +955,10 @@ pub enum ActionView { | |
DeleteAccount { | ||
beneficiary_id: AccountId, | ||
}, | ||
Delegate { | ||
delegate_action: DelegateAction, | ||
signature: Signature, | ||
}, | ||
Comment on lines
+1053
to
+1056
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. 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? 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 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 |
||
} | ||
|
||
impl From<Action> for ActionView { | ||
|
@@ -982,6 +987,10 @@ impl From<Action> for ActionView { | |
Action::DeleteAccount(action) => { | ||
ActionView::DeleteAccount { beneficiary_id: action.beneficiary_id } | ||
} | ||
Action::Delegate(action) => ActionView::Delegate { | ||
delegate_action: action.delegate_action, | ||
signature: action.signature, | ||
}, | ||
} | ||
} | ||
} | ||
|
@@ -1011,6 +1020,12 @@ impl TryFrom<ActionView> for Action { | |
ActionView::DeleteAccount { beneficiary_id } => { | ||
Action::DeleteAccount(DeleteAccountAction { beneficiary_id }) | ||
} | ||
ActionView::Delegate { delegate_action, signature } => { | ||
Action::Delegate(SignedDelegateAction { | ||
delegate_action: delegate_action, | ||
signature, | ||
}) | ||
} | ||
}) | ||
} | ||
} | ||
|
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.