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
Closed
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
ce474eb
Draft implementation of DelegateAction (NEP-366)
Aug 29, 2022
3eb7f02
Rename 'publisher_id' to 'relayer_id'
Aug 31, 2022
433b158
Updated according to the discussion in NEP-366
Aug 31, 2022
a7bcf89
Add fees for DelegateAction.
Oct 26, 2022
529ca2c
Remove redundant code. Add DelegateActionMustBeOnlyOne error
Nov 1, 2022
cfb9780
Add the inner actions' send fees to the total send fee
Nov 1, 2022
602baca
Charge the send fee for the inner actions twice
Nov 2, 2022
3b78721
Refactor deserialization the inner actions
Oct 31, 2022
106dc67
Applied max_block_height
Nov 1, 2022
ee124a6
Fix delegate_cost.exec_fee todo
Nov 3, 2022
11c29fc
Charge DelegateAction's 'send_fee' in two steps
Nov 8, 2022
b344298
Fixed review issues. Added a test for DelegateAction deserialization
Nov 9, 2022
d6ef8ba
Use `signer_public_key` instead of `delegate_action.public_key` in a …
Nov 9, 2022
df86e3a
Add tests to actions.rs. Fix tests in transaction.rs
Nov 10, 2022
59a5986
Return `AccountDoesNotExist` error if `sender` doesn't exist
Nov 10, 2022
cd289d4
Fix review issues
Nov 10, 2022
9a9c0da
Add protocol_feature_delegate_action
Nov 17, 2022
f34acda
Add "protocol_feature_delegate_action" to "nightly" features
Nov 17, 2022
704c524
Create test_delegate_action_deserialization for stable build
Nov 17, 2022
88973ce
Applied cargo fmt
Nov 17, 2022
335537e
Updated snapshots for tests::test_json_unchanged test
Nov 17, 2022
7575c90
Merge branch 'master' into NEP-366
Nov 17, 2022
010d718
Update DelegateAction feature version number
Nov 17, 2022
1935499
Apply cargo fmt
Nov 17, 2022
79915fd
Update snapshots and error schema for tests
Nov 17, 2022
155054f
Use `safe_add_gas` in `apply_delegate_action` function
Nov 18, 2022
81f4eb7
Add comments which explain what's going on
Nov 25, 2022
d0c3787
Rename protocol_feature_delegate_action with protocol_feature_nep366_…
Jan 12, 2023
e9e1f8f
Add comments and remove `new_delegate_actions`
Jan 12, 2023
d5cb1a0
Add "test_delegate_action_must_be_only_one" test
Jan 12, 2023
c9eb505
Add a test case for ACTION_DELEGATE_NUMBER
Jan 12, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions chain/rosetta-rpc/src/adapters/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Author

@e-uleyskiy e-uleyskiy Nov 3, 2022

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?

Copy link
Contributor

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.

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)

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 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.

}
}
operations
Expand Down
4 changes: 4 additions & 0 deletions core/primitives-core/src/parameter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,9 @@ pub enum Parameter {
ActionDeleteKeySendSir,
ActionDeleteKeySendNotSir,
ActionDeleteKeyExecution,
ActionDelegateSendSir,
ActionDelegateSendNotSir,
ActionDelegateExecution,

// Smart contract dynamic gas costs
WasmRegularOpCost,
Expand Down Expand Up @@ -203,6 +206,7 @@ pub enum FeeParameter {
ActionAddFunctionCallKey,
ActionAddFunctionCallKeyPerByte,
ActionDeleteKey,
ActionDelegate,
}

impl Parameter {
Expand Down
11 changes: 10 additions & 1 deletion core/primitives-core/src/runtime/fees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ pub struct ActionCreationConfig {

/// Base cost of deleting an account.
pub delete_account_cost: Fee,

/// Base cost of a delegate action
pub delegate_cost: Fee,
}

/// Describes the cost of creating an access key.
Expand Down Expand Up @@ -219,6 +222,11 @@ impl RuntimeFeesConfig {
send_not_sir: 147489000000,
execution: 147489000000,
},
delegate_cost: Fee {
send_sir: 2319861500000,
send_not_sir: 2319861500000,
execution: 2319861500000,
},
},
storage_usage_config: StorageUsageConfig {
// See Account in core/primitives/src/account.rs for the data structure.
Expand Down Expand Up @@ -253,7 +261,8 @@ impl RuntimeFeesConfig {
function_call_cost_per_byte: free.clone(),
},
delete_key_cost: free.clone(),
delete_account_cost: free,
delete_account_cost: free.clone(),
delegate_cost: free,
},
storage_usage_config: StorageUsageConfig {
num_bytes_account: 0,
Expand Down
3 changes: 3 additions & 0 deletions core/primitives/res/runtime_configs/parameters.txt
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,9 @@ 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
action_delegate_send_sir: 2_319_861_500_000
e-uleyskiy marked this conversation as resolved.
Show resolved Hide resolved
action_delegate_send_not_sir: 2_319_861_500_000
action_delegate_execution: 2_319_861_500_000

# Smart contract dynamic gas costs
wasm_regular_op_cost: 3_856_371
Expand Down
3 changes: 3 additions & 0 deletions core/primitives/res/runtime_configs/parameters_testnet.txt
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,9 @@ 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
action_delegate_send_sir: 2_319_861_500_000
e-uleyskiy marked this conversation as resolved.
Show resolved Hide resolved
action_delegate_send_not_sir: 2_319_861_500_000
action_delegate_execution: 2_319_861_500_000

# Smart contract dynamic gas costs
wasm_regular_op_cost: 3_856_371
Expand Down
39 changes: 39 additions & 0 deletions core/primitives/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
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"

DelegateActionCantContainNestedOne,
/// There should be the only one DelegateAction
DelegateActionMustBeOnlyOne,
}

/// Describes the error for validating a receipt.
Expand Down Expand Up @@ -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"
)
}
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

The 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),
}
}
}
Expand Down
24 changes: 24 additions & 0 deletions core/primitives/src/receipt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,30 @@ impl Receipt {
}),
}
}

pub fn new_delegate_actions(
jakmeier marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down
1 change: 1 addition & 0 deletions core/primitives/src/runtime/parameter_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ impl ParameterTable {
},
"delete_key_cost": self.fee_json(FeeParameter::ActionDeleteKey),
"delete_account_cost": self.fee_json(FeeParameter::ActionDeleteAccount),
"delegate_cost": self.fee_json(FeeParameter::ActionDelegate),
},
"storage_usage_config": {
"num_bytes_account": self.get(Parameter::StorageNumBytesAccount),
Expand Down
78 changes: 78 additions & 0 deletions core/primitives/src/transaction.rs
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};
Expand Down Expand Up @@ -70,6 +72,7 @@ pub enum Action {
AddKey(AddKeyAction),
DeleteKey(DeleteKeyAction),
DeleteAccount(DeleteAccountAction),
Delegate(SignedDelegateAction),
}

impl Action {
Expand Down Expand Up @@ -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)]
Copy link
Contributor

Choose a reason for hiding this comment

The 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The 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> {
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.

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)]
Expand Down
21 changes: 18 additions & 3 deletions core/primitives/src/views.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -954,6 +955,10 @@ pub enum ActionView {
DeleteAccount {
beneficiary_id: AccountId,
},
Delegate {
delegate_action: DelegateAction,
signature: Signature,
},
Comment on lines +1053 to +1056
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

}

impl From<Action> for ActionView {
Expand Down Expand Up @@ -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,
},
}
}
}
Expand Down Expand Up @@ -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,
})
}
})
}
}
Expand Down
3 changes: 3 additions & 0 deletions runtime/runtime-params-estimator/src/cost.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ pub enum Cost {
/// Subtract the base cost of creating a sir-receipt.
/// (TODO[jakmeier] Consider different account states.
ActionDeleteAccount,
/// Estimates `action_creation_config.delegate_cost` which is charged
/// for `DelegateAction` actions.
ActionDelegate,

/// Estimates `wasm_config.ext_costs.base` which is intended to be charged
/// once on every host function call. However, this is currently
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ fn runtime_fees_config(cost_table: &CostTable) -> anyhow::Result<RuntimeFeesConf
},
delete_key_cost: fee(Cost::ActionDeleteKey)?,
delete_account_cost: fee(Cost::ActionDeleteAccount)?,
delegate_cost: fee(Cost::ActionDelegate)?,
},
..actual_fees_config.clone()
};
Expand Down
Loading