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

ETH-implicit account support (old design) #10056

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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 Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions chain/chain/src/tests/simple_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ fn build_chain() {
// cargo insta test --accept -p near-chain --features nightly -- tests::simple_chain::build_chain
let hash = chain.head().unwrap().last_block_hash;
if cfg!(feature = "nightly") {
insta::assert_display_snapshot!(hash, @"CwaiZ4AmfJSnMN9rytYwwYHCTzLioC5xcjHzNkDex1HH");
insta::assert_display_snapshot!(hash, @"8WF4fG7WCM2ysZvFQAgEfTEfBovtULxnWeRpwAt3BTBJ");
} else {
insta::assert_display_snapshot!(hash, @"HJmRPXT4JM9tt6mXw2gM75YaSoqeDCphhFK26uRpd1vw");
}
Expand Down Expand Up @@ -78,7 +78,7 @@ fn build_chain() {

let hash = chain.head().unwrap().last_block_hash;
if cfg!(feature = "nightly") {
insta::assert_display_snapshot!(hash, @"Dn18HUFm149fojXpwV1dYCfjdPh56S1k233kp7vmnFeE");
insta::assert_display_snapshot!(hash, @"3iXi6BshQaPx9TsbDt5itAXUjnTQz9AR9pg2w349TFNj");
} else {
insta::assert_display_snapshot!(hash, @"HbQVGVZ3WGxsNqeM3GfSwDoxwYZ2RBP1SinAze9SYR3C");
}
Expand Down
11 changes: 10 additions & 1 deletion chain/jsonrpc/res/rpc_errors_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,8 @@
"MethodNameMismatch",
"RequiresFullAccess",
"NotEnoughAllowance",
"DepositWithFunctionCall"
"DepositWithFunctionCall",
"InvalidPublicKeyForEthAddress"
],
"props": {}
},
Expand Down Expand Up @@ -513,6 +514,14 @@
"subtypes": [],
"props": {}
},
"InvalidPublicKeyForEthAddress": {
"name": "InvalidPublicKeyForEthAddress",
"subtypes": [],
"props": {
"account_id": "",
"public_key": ""
}
},
"InvalidReceiptIndex": {
"name": "InvalidReceiptIndex",
"subtypes": [],
Expand Down
7 changes: 7 additions & 0 deletions core/crypto/src/signature.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,13 @@ impl PublicKey {
Self::SECP256K1(_) => panic!(),
}
}

pub fn unwrap_as_secp256k1(&self) -> &Secp256K1PublicKey {
match self {
Self::SECP256K1(key) => key,
Self::ED25519(_) => panic!(),
}
}
}

// This `Hash` implementation is safe since it retains the property
Expand Down
5 changes: 4 additions & 1 deletion core/crypto/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ impl PublicKey {
let keypair = ed25519_key_pair_from_seed(seed);
PublicKey::ED25519(ED25519PublicKey(keypair.public.to_bytes()))
}
_ => unimplemented!(),
KeyType::SECP256K1 => {
let secret_key = SecretKey::SECP256K1(secp256k1_secret_key_from_seed(seed));
PublicKey::SECP256K1(secret_key.public_key().unwrap_as_secp256k1().clone())
}
}
}
}
Expand Down
27 changes: 21 additions & 6 deletions core/primitives-core/src/runtime/fees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,10 @@
//! * sir -- sender is receiver. Receipts that are directed by an account to itself are guaranteed
//! to not be cross-shard which is cheaper than cross-shard. Conversely, when sender is not a
//! receiver it might or might not be a cross-shard communication.
use crate::checked_feature;
use crate::config::ActionCosts;
use crate::num_rational::Rational32;
use crate::types::{Balance, Gas};
use crate::types::{Balance, Gas, ProtocolVersion};
use enum_map::EnumMap;
use near_account_id::AccountType;

Expand Down Expand Up @@ -203,22 +204,29 @@ impl StorageUsageConfig {
}

/// Helper functions for computing Transfer fees.
/// In case of implicit account creation they always include extra fees for the CreateAccount and
/// In case of implicit account creation they include extra fees for the CreateAccount and
/// AddFullAccessKey (for NEAR-implicit account only) actions that are implicit.
/// We can assume that no overflow will happen here.
pub fn transfer_exec_fee(
cfg: &RuntimeFeesConfig,
implicit_account_creation_allowed: bool,
receiver_account_type: AccountType,
protocol_version: ProtocolVersion,
) -> Gas {
let transfer_fee = cfg.fee(ActionCosts::transfer).exec_fee();
match (implicit_account_creation_allowed, receiver_account_type) {
// Regular transfer to a named account.
(_, AccountType::NamedAccount) => transfer_fee,
// No account will be created, just a regular transfer.
(false, _) => transfer_fee,
// Currently, no account is created on transfer to ETH-implicit account, just a regular transfer.
(true, AccountType::EthImplicitAccount) => transfer_fee,
// Extra fee for the CreateAccount.
(true, AccountType::EthImplicitAccount) => {
if checked_feature!("stable", EthImplicit, protocol_version) {
transfer_fee + cfg.fee(ActionCosts::create_account).exec_fee()
} else {
transfer_fee
}
}
// Extra fees for the CreateAccount and AddFullAccessKey.
(true, AccountType::NearImplicitAccount) => {
transfer_fee
Expand All @@ -233,15 +241,22 @@ pub fn transfer_send_fee(
sender_is_receiver: bool,
implicit_account_creation_allowed: bool,
receiver_account_type: AccountType,
protocol_version: ProtocolVersion,
) -> Gas {
let transfer_fee = cfg.fee(ActionCosts::transfer).send_fee(sender_is_receiver);
match (implicit_account_creation_allowed, receiver_account_type) {
// Regular transfer to a named account.
(_, AccountType::NamedAccount) => transfer_fee,
// No account will be created, just a regular transfer.
(false, _) => transfer_fee,
// Currently, no account is created on transfer to ETH-implicit account, just a regular transfer.
(true, AccountType::EthImplicitAccount) => transfer_fee,
// Extra fee for the CreateAccount.
(true, AccountType::EthImplicitAccount) => {
if checked_feature!("stable", EthImplicit, protocol_version) {
transfer_fee + cfg.fee(ActionCosts::create_account).send_fee(sender_is_receiver)
staffik marked this conversation as resolved.
Show resolved Hide resolved
} else {
transfer_fee
}
}
// Extra fees for the CreateAccount and AddFullAccessKey.
(true, AccountType::NearImplicitAccount) => {
transfer_fee
Expand Down
4 changes: 3 additions & 1 deletion core/primitives-core/src/version.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,7 @@ pub enum ProtocolFeature {
/// NEP: https://github.com/near/NEPs/pull/509
#[cfg(feature = "protocol_feature_chunk_validation")]
ChunkValidation,
EthImplicit,
}

impl ProtocolFeature {
Expand Down Expand Up @@ -183,6 +184,7 @@ impl ProtocolFeature {
ProtocolFeature::SimpleNightshadeV2 => 135,
#[cfg(feature = "protocol_feature_chunk_validation")]
ProtocolFeature::ChunkValidation => 137,
ProtocolFeature::EthImplicit => 138,
}
}
}
Expand All @@ -195,7 +197,7 @@ const STABLE_PROTOCOL_VERSION: ProtocolVersion = 64;
/// Largest protocol version supported by the current binary.
pub const PROTOCOL_VERSION: ProtocolVersion = if cfg!(feature = "nightly_protocol") {
// On nightly, pick big enough version to support all features.
139
140
} else {
// Enable all stable features.
STABLE_PROTOCOL_VERSION
Expand Down
1 change: 1 addition & 0 deletions core/primitives/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ serde.workspace = true
serde_json.workspace = true
serde_with.workspace = true
serde_yaml.workspace = true
sha3.workspace = true
smart-default.workspace = true
stdx.workspace = true
strum.workspace = true
Expand Down
15 changes: 12 additions & 3 deletions core/primitives/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,8 @@ pub enum InvalidAccessKeyError {
},
/// Having a deposit with a function call action is not allowed with a function call access key.
DepositWithFunctionCall,
/// ETH-implicit `account_id` isn't derived from the `public_key`.
InvalidPublicKeyForEthAddress { account_id: AccountId, public_key: PublicKey },
}

/// Describes the error for validating a list of actions.
Expand Down Expand Up @@ -486,9 +488,9 @@ pub enum ActionErrorKind {
/// Error occurs when a new `ActionReceipt` created by the `FunctionCall` action fails
/// receipt validation.
NewReceiptValidationError(ReceiptValidationError),
/// Error occurs when a `CreateAccount` action is called on hex-characters
/// account of length 64. See implicit account creation NEP:
/// <https://github.com/nearprotocol/NEPs/pull/71>.
/// Error occurs when a `CreateAccount` action is called on a NEAR-implicit or ETH-implicit account.
/// See NEAR-implicit account creation NEP: <https://github.com/nearprotocol/NEPs/pull/71>.
/// Also, see ETH-implicit account creation NEP: <https://github.com/near/NEPs/issues/498>.
///
/// TODO(#8598): This error is named very poorly. A better name would be
/// `OnlyNamedAccountCreationAllowed`.
Expand Down Expand Up @@ -612,6 +614,13 @@ impl Display for InvalidAccessKeyError {
InvalidAccessKeyError::DepositWithFunctionCall => {
write!(f, "Having a deposit with a function call action is not allowed with a function call access key.")
}
InvalidAccessKeyError::InvalidPublicKeyForEthAddress { account_id, public_key } => {
write!(
f,
"ETH-implicit address {:?} isn't derived from the public_key {}",
account_id, public_key
)
}
}
}
}
Expand Down
12 changes: 12 additions & 0 deletions core/primitives/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,8 @@ pub fn create_user_test_signer(account_name: &AccountIdRef) -> InMemorySigner {
let account_id = account_name.to_owned();
if account_id == near_implicit_test_account() {
staffik marked this conversation as resolved.
Show resolved Hide resolved
InMemorySigner::from_secret_key(account_id, near_implicit_test_account_secret())
} else if account_id == eth_implicit_test_account() {
InMemorySigner::from_secret_key(account_id, eth_implicit_test_account_secret())
} else {
InMemorySigner::from_seed(account_id, KeyType::ED25519, account_name.as_str())
}
Expand All @@ -561,6 +563,16 @@ pub fn near_implicit_test_account_secret() -> SecretKey {
"ed25519:5roj6k68kvZu3UEJFyXSfjdKGrodgZUfFLZFpzYXWtESNsLWhYrq3JGi4YpqeVKuw1m9R2TEHjfgWT1fjUqB1DNy".parse().unwrap()
}

/// A fixed ETH-implicit account for which tests can know the private key.
pub fn eth_implicit_test_account() -> AccountId {
"0x96791e923f8cf697ad9c3290f2c9059f0231b24c".parse().unwrap()
}

/// Private key for the fixed ETH-implicit test account.
pub fn eth_implicit_test_account_secret() -> SecretKey {
"secp256k1:X4ETFKtQkSGVoZEnkn7bZ3LyajJaK2b3eweXaKmynGx".parse().unwrap()
}

impl FinalExecutionOutcomeView {
#[track_caller]
/// Check transaction and all transitive receipts for success status.
Expand Down
81 changes: 74 additions & 7 deletions core/primitives/src/utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@ use rand::distributions::Alphanumeric;
use rand::{thread_rng, Rng};
use serde;

use crate::account::AccessKey;
use crate::checked_feature;
use crate::errors::InvalidAccessKeyError;
use crate::hash::{hash, CryptoHash};
use crate::receipt::Receipt;
use crate::transaction::SignedTransaction;
Expand All @@ -17,8 +20,8 @@ use crate::version::{
CREATE_RECEIPT_ID_SWITCH_TO_CURRENT_BLOCK_VERSION,
};

use near_crypto::ED25519PublicKey;
use near_primitives_core::account::id::AccountId;
use near_crypto::{KeyType, PublicKey};
use near_primitives_core::account::id::{AccountId, AccountType};

use std::mem::size_of;
use std::ops::Deref;
Expand Down Expand Up @@ -469,23 +472,87 @@ where
Serializable(object)
}

/// Derives `AccountId` from `PublicKey``.
/// From `near-account-id` version `1.0.0-alpha.2`, `is_implicit` returns true for ETH-implicit accounts.
/// This function is a wrapper for `is_implicit` method so that we can easily differentiate its behavior
/// based on the protocol version.
pub fn account_is_implicit(account_id: &AccountId, protocol_version: ProtocolVersion) -> bool {
if checked_feature!("stable", EthImplicit, protocol_version) {
account_id.get_account_type().is_implicit()
} else {
account_id.get_account_type() == AccountType::NearImplicitAccount
}
}

/// Derives `AccountId` from `PublicKey`.
/// If the key type is ED25519, returns hex-encoded copy of the key.
pub fn derive_near_implicit_account_id(public_key: &ED25519PublicKey) -> AccountId {
hex::encode(public_key).parse().unwrap()
/// If the key type is SECP256K1, returns '0x' + keccak256(public_key)[12:32].hex().
pub fn derive_account_id_from_public_key(public_key: &PublicKey) -> AccountId {
match public_key.key_type() {
KeyType::ED25519 => hex::encode(public_key.key_data()).parse().unwrap(),
KeyType::SECP256K1 => {
use sha3::Digest;
let pk_hash = sha3::Keccak256::digest(&public_key.key_data());
format!("0x{}", hex::encode(&pk_hash[12..32])).parse().unwrap()
}
}
}

// TODO(eth-implicit) This functionality will be managed by `Wallet Contract`
// and no access key will be added.
Comment on lines +500 to +501
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 this code should be removed from this PR instead of being added now only to be removed later when we push the Wallet Contract. Adding and removing code creates unnecessary churn in my opinion. For example, not having this check_access_key function means we don't need the InvalidPublicKeyForEthAddress variant of InvalidAccessKeyError, and we don't need the big new integration tests around handling nonces at the access key level. I didn't look into it too deeply, but maybe we will also be able to remove some of the places in the runtime where the protocol version is being newly passed in as an argument.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the Wallet Contract placeholder I would have to remove perhaps the entire code from this PR to keep CI happy. I think there are 2 options:

  1. We merge this PR as a basis for the next PR.
  2. I close this PR and work on a new one with Wallet Contract in mind from the beginning.

I have no strong opinion which approach is better.
The first option requires more reviewers time but it can spot some issues earlier (by reviewers or nightly testing).
The second option looks clearer, as it be better visible within one PR what changes, and save reviewers time now.
Feel free to just leave some comments, that would help in both options.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the second approach better. Not just to save review time, but to save you time as well. I think starting fresh from the master branch should be easier for the new PR than trying to remove code you just added.

The overall structure of this PR looks good to me. I don't have any comments about this PR which might inform you for the new PR.

/// Checks if access key is missing. If not, returns Ok(access_key).
/// Otherwise, there is a chance that a full access key will be added now
/// for an ETH-implicit address. If none of these occurs, returns appropriate error.
pub fn check_access_key(
access_key: Option<AccessKey>,
account_id: &AccountId,
public_key: &PublicKey,
protocol_version: ProtocolVersion,
) -> Result<AccessKey, InvalidAccessKeyError> {
match access_key {
Some(access_key) => Ok(access_key),
None => {
if !checked_feature!("stable", EthImplicit, protocol_version)
|| account_id.get_account_type() != AccountType::EthImplicitAccount
{
return Err(InvalidAccessKeyError::AccessKeyNotFound {
account_id: account_id.clone(),
public_key: public_key.clone(),
});
}

if derive_account_id_from_public_key(public_key) == *account_id {
// TODO(eth-implicit) Is storage increase for that account (because we added access key) correctly handled?
// Ultimately, no access key would be added because `Wallet Contract` will manage this account.
return Ok(AccessKey::full_access());
} else {
// Provided public key is not the one from which the signer address was derived.
return Err(InvalidAccessKeyError::InvalidPublicKeyForEthAddress {
account_id: account_id.clone(),
public_key: public_key.clone(),
});
}
}
}
}

#[cfg(test)]
mod tests {
use super::*;
use near_crypto::{KeyType, PublicKey};

#[test]
fn test_derive_account_id_from_ed25519_public_key() {
let public_key = PublicKey::from_seed(KeyType::ED25519, "test");
let expected: AccountId =
"bb4dc639b212e075a751685b26bdcea5920a504181ff2910e8549742127092a0".parse().unwrap();
let account_id = derive_near_implicit_account_id(public_key.unwrap_as_ed25519());
let account_id = derive_account_id_from_public_key(&public_key);
assert_eq!(account_id, expected);
}

#[test]
fn test_derive_account_id_from_secp256k1_public_key() {
let public_key = PublicKey::from_seed(KeyType::SECP256K1, "test");
let expected: AccountId = "0x96791e923f8cf697ad9c3290f2c9059f0231b24c".parse().unwrap();
let account_id = derive_account_id_from_public_key(&public_key);
assert_eq!(account_id, expected);
}

Expand Down
Loading
Loading