-
Notifications
You must be signed in to change notification settings - Fork 627
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 27 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 |
---|---|---|
|
@@ -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 = [] | ||
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. small preference to add the nep_366 to the name (makes it a lot easier to find in the future). |
||
nightly = [ | ||
"nightly_protocol", | ||
"protocol_feature_fix_staking_threshold", | ||
"protocol_feature_fix_contract_loading_cost", | ||
"protocol_feature_reject_blocks_with_outdated_protocol_version", | ||
"protocol_feature_ed25519_verify", | ||
"protocol_feature_delegate_action", | ||
] | ||
|
||
nightly_protocol = [] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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 think we have the correct values now, right @jakmeier 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. 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 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. |
||
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
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. | ||
|
@@ -314,6 +318,14 @@ impl Display for ActionsValidationError { | |
f, | ||
"The attached amount of gas in a FunctionCall action has to be a positive number", | ||
), | ||
ActionsValidationError::DelegateActionCantContainNestedOne => write!( | ||
f, | ||
"DelegateAction must not contain another DelegateAction" | ||
), | ||
ActionsValidationError::DelegateActionMustBeOnlyOne => write!( | ||
f, | ||
"The actions can contain the ony one DelegateAction" | ||
) | ||
} | ||
} | ||
} | ||
|
@@ -397,6 +409,18 @@ 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 }, | ||
/// 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 { | ||
|
@@ -707,6 +731,12 @@ 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::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 |
---|---|---|
|
@@ -86,6 +86,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 |
---|---|---|
|
@@ -84,6 +84,11 @@ expression: store.get_config(*version) | |
"send_sir": 147489000000, | ||
"send_not_sir": 147489000000, | ||
"execution": 147489000000 | ||
}, | ||
"delegate_cost": { | ||
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. @jakmeier - are these changes expected? shouldn't the delegate cost be from the 'next' release onwards only? 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, 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) |
||
"send_sir": 2319861500000, | ||
"send_not_sir": 2319861500000, | ||
"execution": 2319861500000 | ||
} | ||
}, | ||
"storage_usage_config": { | ||
|
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.