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.

Sign up for GitHub

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

Jump to bottom

ETH-implicit account support (old design) #10056

Closed
wants to merge 5 commits into from
Closed

ETH-implicit account support (old design) #10056

wants to merge 5 commits into from

Conversation

staffik
Copy link
Contributor

@staffik staffik commented Oct 31, 2023 •
edited
Loading

Part of #10018.

(Note 2023-11-17) This PR follows the old design. The next PR will incorporate changes required by the new design.
Regarding changes on the protocol level, the new design is different from the old one by:

  • On a first transfer to an ETH-implicit account, a Wallet Contract (see the new design NEP) should be added.
  • We will not add full access key on a first transfer from an ETH-implicit account. Instead, the ETH-implicit account will be used by calling the associated Wallet Contract that manages the account.

This PR introduces real protocol changes for supporting ETH-implicit addresses, following the preparatory PR.
On transfer to a ETH-implicit address, an account is added (if does not exist yet) on chain, without access key yet.
On a transaction from that address, if there is no access key yet, and the address is '0x' + keccak(pub_key)[12:32] derived from the public key, a full access key is added to that account with nonce set to the transaction nonce.

Summary:

  • Add InvalidPublicKeyForEthAddress error, that means that an ETH-implicit address was not derived from the provided public key.
  • Add derive_account_id_from_public_key function that returns '0x' + keccak256(public_key)[12:32].hex() if the public key type is SECP256K1.
  • action_implicit_account_creation_transfer: in case of EthImplicitAccount creates an account without an access key.
  • Update fees calculations to be the same as for NearImplicitAccount minus ActionCosts::add_full_access_key.
  • verify_and_charge_transaction, validate_delegate_action_key: if access key not found for transaction.public_key, and signer_id is EthImplicitAccountthat was derived from transaction.public_key then add full access key to that account. Newly created access key will have nonce set to transaction nonce.
  • Treat ETH-implicit account like implicit accounts, which mean we can use account_id.get_account_type().is_implicit() now. See Setup for ETH-implicit accounts #10020 (comment).
  • Update tests, add new tests.
  • Wrap changes in checked_feature!("stable", EthImplicit, protocol_version) and/or protocol_feature_eth_implicit (if necessary).

@staffik staffik mentioned this pull request Oct 31, 2023
Closed
8 tasks
@staffik staffik force-pushed the eth-implicit-account-support branch from 8e150b8 to 6e1c7b1 Compare November 1, 2023 11:53
@staffik staffik force-pushed the eth-implicit-account branch 3 times, most recently from f7ba202 to 861bdca Compare November 5, 2023 20:14
@staffik staffik force-pushed the eth-implicit-account-support branch 2 times, most recently from 0713461 to 48102ce Compare November 6, 2023 12:59
Base automatically changed from eth-implicit-account to master November 6, 2023 13:10
@staffik staffik force-pushed the eth-implicit-account-support branch 3 times, most recently from 6c79934 to da30e31 Compare November 7, 2023 08:14
@staffik
Copy link
Contributor Author

staffik commented Nov 7, 2023 •
edited
Loading

More questions:

  • Is it an issue that a named account could be a sub account of an ETH-implicit account?
  • Can ED25519 key be added to an ETH-implicit account?
  • Can Secp256k1 access_key be removed from ETH-implicit and ED25519 access key added instead? If yes, would the original Secp256k1 key lose its power to sign transactions from that account?

@birchmd wdyt?

@staffik staffik force-pushed the eth-implicit-account-support branch 3 times, most recently from 2c3f3a3 to d10bfc5 Compare November 8, 2023 12:55
@staffik staffik force-pushed the eth-implicit-account-support branch 7 times, most recently from 7cb592f to 1206d09 Compare November 8, 2023 15:41
@staffik staffik changed the title ETH-implicit account support [WIP] ETH-implicit account support Nov 8, 2023
@staffik staffik marked this pull request as ready for review November 8, 2023 15:43
@staffik staffik requested a review from a team as a code owner November 8, 2023 15:43
walnut-the-cat
walnut-the-cat reviewed Nov 10, 2023
View reviewed changes
core/primitives/src/test_utils.rs Show resolved Hide resolved
core/primitives/src/utils.rs Outdated Show resolved Hide resolved
@staffik staffik force-pushed the eth-implicit-account-support branch 9 times, most recently from 18de969 to ca32c1a Compare November 13, 2023 16:18
@walnut-the-cat
Copy link
Contributor

Sorry accidentally closed PR and reopened :( @staffik , please re-request review when ready so it can get attention from reviewers

@staffik staffik changed the title ETH-implicit account support ETH-implicit account support (old design) Nov 17, 2023
birchmd
birchmd reviewed Nov 17, 2023
View reviewed changes
core/primitives/src/utils.rs
Comment on lines +500 to +501
// TODO(eth-implicit) This functionality will be managed by `Wallet Contract`
// and no access key will be added.
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.

@staffik staffik closed this Nov 20, 2023
@staffik
Copy link
Contributor Author

staffik commented Nov 20, 2023

Closed, will reintroduce some changes in a new PR following the new design, for ease of development and reviewing.

@staffik staffik mentioned this pull request Nov 21, 2023
Merged
github-merge-queue bot pushed a commit that referenced this pull request Nov 24, 2023
@staffik
ETH-implicit account support (#10224)
1a5c286 
## Context
Tracking issue: #10018.
Design: near/NEPs#518 (comment).

### Goal
We want the NEAR Protocol's ecosystem to be closer with Ethereum by
integrating Web3 wallet support.
To accomplish that, some changes on the protocol level are needed with
an emphasis on user experience continuity.
Following the design, the main change on the protocol level would be to
have ETH-style addresses managed by a new `Wallet Contract` integrated
into the protocol.
For seamless onboarding of EVM users, the contract will be free of
charge.

### Previous work
This PR is built on top of a [preparatory
PR](#10020).
It reintroduces some changes from a [closed
PR](#10056) that follows an old
design.

## Summary
On a transfer to an ETH-implicit address:
1. An account is created at this address.
2. The account does not have any access key added. Thus, it is not
possible to make a transaction from this account directly.
3. The Wallet Contract (as a part of the protocol) is deployed to the
account. For this PR, an empty smart contract is used as a placeholder.

### Changes
- On a transfer to an ETH-implicit address, if an account does not exist
yet at this address:
   - Create account at this address.
- Deploy a `Wallet Contract` placeholder (empty contract now) to this
account.
- Update fee for a transfer to an ETH-implicit address to include
account creation cost. `Wallet Contract` deployment is free of charge
(user only pays for an increased storage).
- Tests:
- Test whether it is possible to create an ETH-implicit account by
making a transfer to that address, and whether further funds can be sent
to this account.
- Test that no access key is added to an ETH-implicit account (the
account is locked) and in consequence no transactions are possible from
this account.
- Guard the changes with `EthImplicitAccounts` protocol feature and
`eth_implicit_accounts` runtime config flag.

## Next work
- Discuss how the `Wallet Contract` should be integrated into the
protocol for efficiency.
- Use the `Wallet Contract` placeholder implementation that allows to
transfer funds only.
@staffik staffik mentioned this pull request Nov 29, 2023
Merged
@Ekleog-NEAR Ekleog-NEAR deleted the eth-implicit-account-support branch March 29, 2024 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walnut-the-cat walnut-the-cat walnut-the-cat left review comments

@birchmd birchmd birchmd left review comments

@wacban wacban Awaiting requested review from wacban

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@staffik @akhi3030 @walnut-the-cat @wacban @birchmd