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: Implicit account creation #3251

Merged
merged 14 commits into from
Sep 2, 2020
Merged

feat: Implicit account creation #3251

merged 14 commits into from
Sep 2, 2020

Conversation

evgenykuzyakov
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov commented Aug 26, 2020

This PR implements near/NEPs#71

It contains a bunch of boilerplate code to pass the protocol version to VM and tests.

Logical changes:

  • Bumps protocol version to 35
  • Post protocol version, when a transfer action is executed on the 64-length hex like account ID as a single action in a transaction, it creates a new account if the account doesn't exist and add the ED25519 public key from the account ID hex representation.
  • If a CreateAccount action attempts to create 64-length hex like account, it fails.
  • Refunds don't automatically create accounts, because refunds are free and we don't want some type of abuse. NOTE: Account deletion with beneficiary creates a refund, so it'll not create a new account.
  • TransferAction fee cost on such 64-len hex accounts include CreateAction and AddFullAccessKey costs
  • The fee is also updated when calling a promise.
  • VMLogic::new change is backward compatible until we release a new near-sdk version. So it shouldn't break master after pushing it.
  • Bump near-vm-* versions to 2.2.0.

TODO:

  • Add integration test for transfer from promise
  • Add migration test for transfer to 64len
  • Uncomment VMLogic::new for near-sdk MockedBlockchain

Test plan:

  • CI
  • Added integration test and migration test

@gitpod-io
Copy link

gitpod-io bot commented Aug 26, 2020

@codecov
Copy link

codecov bot commented Aug 27, 2020

Codecov Report

Merging #3251 into master will decrease coverage by 0.07%.
The diff coverage is 94.16%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3251      +/-   ##
==========================================
- Coverage   87.03%   86.96%   -0.08%     
==========================================
  Files         216      216              
  Lines       42119    42317     +198     
==========================================
+ Hits        36659    36800     +141     
- Misses       5460     5517      +57     
Impacted Files Coverage Δ
chain/chain/src/test_utils.rs 91.29% <ø> (-0.93%) ⬇️
chain/chain/src/types.rs 95.83% <ø> (ø)
core/primitives/src/version.rs 100.00% <ø> (ø)
neard/src/runtime.rs 85.94% <0.00%> (-0.11%) ⬇️
runtime/near-vm-logic/src/types.rs 33.33% <ø> (ø)
runtime/near-vm-runner/src/runner.rs 100.00% <ø> (ø)
runtime/near-vm-runner/tests/test_rs_contract.rs 94.35% <ø> (ø)
runtime/near-vm-runner/tests/test_ts_contract.rs 96.22% <0.00%> (ø)
runtime/near-vm-runner/tests/test_utils/mod.rs 97.05% <ø> (ø)
runtime/runtime-standalone/src/lib.rs 94.84% <ø> (ø)
... and 33 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c4f8d4...7873581. Read the comment docs.

&& action_receipt.signer_id == receipt.receiver_id
{
try_refund_allowance(
if let Some(account) = account.as_mut() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Execution of transfer action

@@ -132,7 +152,18 @@ pub fn exec_fee(config: &RuntimeFeesConfig, action: &Action) -> Gas {
cfg.function_call_cost.exec_fee()
+ cfg.function_call_cost_per_byte.exec_fee() * num_bytes
}
Transfer(_) => cfg.transfer_cost.exec_fee(),
Transfer(_) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated exec fees

@@ -88,7 +92,18 @@ pub fn total_send_fees(
cfg.function_call_cost.send_fee(sender_is_receiver)
+ cfg.function_call_cost_per_byte.send_fee(sender_is_receiver) * num_bytes
}
Transfer(_) => cfg.transfer_cost.send_fee(sender_is_receiver),
Transfer(_) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updated send fee

@@ -509,19 +552,47 @@ pub(crate) fn check_account_existence(
action: &Action,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Logic for checking whether the current action is allowed.

@@ -343,6 +348,44 @@ pub(crate) fn action_create_account(
});
}

pub(crate) fn action_implicit_account_creation_transfer(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Execution of transfer action with implicit account creation

@@ -1358,6 +1389,25 @@ impl<'a> VMLogic<'a> {

let (receipt_idx, sir) = self.promise_idx_to_receipt_idx_with_sir(promise_idx)?;

if self.current_protocol_version >= IMPLICIT_ACCOUNT_CREATION_PROTOCOL_VERSION {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We charge more base fees for transfer on 64len hex account under new protocol

@evgenykuzyakov evgenykuzyakov changed the title WIP: Pass protocol versions feat: Implicit account creation Sep 1, 2020
@evgenykuzyakov evgenykuzyakov marked this pull request as ready for review September 1, 2020 21:51
@evgenykuzyakov
Copy link
Collaborator Author

Please use my comments to navigate logical changes in the code

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

is there a surjection between ed25519 keypairs and byte arrays of length 32?

core/primitives/src/utils.rs Outdated Show resolved Hide resolved
use near_vm_logic::{ReturnData, VMConfig, VMContext, VMOutcome};
use near_vm_runner::{run, VMError};

const LATEST_PROTOCOL_VERSION: ProtocolVersion = ProtocolVersion::MAX;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Runner doesn't have primitives, so I can't use real protocol version.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if Bowen asked why do you need to introduce this const. It seems ProtocolVersion::MAX is quite descriptive to use it inline.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see. @bowenwang1996 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was wondering whether using max here can cause problems. For example if we have some function that requires doing arithmetic on protocol version it might easily overflow.

@evgenykuzyakov
Copy link
Collaborator Author

is there a surjection between ed25519 keypairs and byte arrays of length 32?

Aa part of InMemorySigner we just convert bytes to a secretkey. It may not be a valid ristretto, but it's okay for a full access key.

Copy link
Collaborator

@bowenwang1996 bowenwang1996 left a comment

Choose a reason for hiding this comment

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

Can we run nightly tests on this PR?

runtime/near-vm-logic/src/utils.rs Outdated Show resolved Hide resolved
use near_vm_logic::{ReturnData, VMConfig, VMContext, VMOutcome};
use near_vm_runner::{run, VMError};

const LATEST_PROTOCOL_VERSION: ProtocolVersion = ProtocolVersion::MAX;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if Bowen asked why do you need to introduce this const. It seems ProtocolVersion::MAX is quite descriptive to use it inline.

runtime/runtime/src/actions.rs Show resolved Hide resolved
runtime/runtime/src/actions.rs Show resolved Hide resolved
runtime/runtime/src/config.rs Show resolved Hide resolved
runtime/runtime/src/lib.rs Show resolved Hide resolved
@evgenykuzyakov
Copy link
Collaborator Author

@evgenykuzyakov evgenykuzyakov merged commit 9d759ad into master Sep 2, 2020
@evgenykuzyakov evgenykuzyakov deleted the 64-len-accounts branch September 2, 2020 21:11
mhalambek pushed a commit that referenced this pull request Sep 7, 2020
This PR implements near/NEPs#71

It contains a bunch of boilerplate code to pass the protocol version to VM and tests.

Logical changes:
- Bumps protocol version to 35
- Post protocol version, when a transfer action is executed on the 64-length hex like account ID as a single action in a transaction, it creates a new account if the account doesn't exist and add the ED25519 public key from the account ID hex representation.
- If a CreateAccount action attempts to create 64-length hex like account, it fails.
- Refunds don't automatically create accounts, because refunds are free and we don't want some type of abuse. NOTE: Account deletion with beneficiary creates a refund, so it'll not create a new account.
- `TransferAction` fee cost on such 64-len hex accounts include `CreateAction` and `AddFullAccessKey` costs
- The fee is also updated when calling a promise.
- `VMLogic::new` change is backward compatible until we release a new `near-sdk` version. So it shouldn't break master after pushing it.
- Bump `near-vm-*` versions to `2.2.0`.

TODO:

- [x] Add integration test for transfer from promise
- [x] Add migration test for transfer to 64len
- [x] Uncomment `VMLogic::new` for `near-sdk` `MockedBlockchain`

## Test plan:
- CI
- Added integration test and migration test
chefsale pushed a commit that referenced this pull request Sep 7, 2020
This PR implements near/NEPs#71

It contains a bunch of boilerplate code to pass the protocol version to VM and tests.

Logical changes:
- Bumps protocol version to 35
- Post protocol version, when a transfer action is executed on the 64-length hex like account ID as a single action in a transaction, it creates a new account if the account doesn't exist and add the ED25519 public key from the account ID hex representation.
- If a CreateAccount action attempts to create 64-length hex like account, it fails.
- Refunds don't automatically create accounts, because refunds are free and we don't want some type of abuse. NOTE: Account deletion with beneficiary creates a refund, so it'll not create a new account.
- `TransferAction` fee cost on such 64-len hex accounts include `CreateAction` and `AddFullAccessKey` costs
- The fee is also updated when calling a promise.
- `VMLogic::new` change is backward compatible until we release a new `near-sdk` version. So it shouldn't break master after pushing it.
- Bump `near-vm-*` versions to `2.2.0`.

TODO:

- [x] Add integration test for transfer from promise
- [x] Add migration test for transfer to 64len
- [x] Uncomment `VMLogic::new` for `near-sdk` `MockedBlockchain`

## Test plan:
- CI
- Added integration test and migration test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants