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

RPC: add actual_amount into account/ rpc call #1974

Closed
wants to merge 8 commits into from

Conversation

lexfrl
Copy link
Contributor

@lexfrl lexfrl commented Jan 17, 2020

closes #1811

@codecov
Copy link

codecov bot commented Jan 17, 2020

Codecov Report

Merging #1974 into staging will increase coverage by 0.74%.
The diff coverage is 82.61%.

Impacted file tree graph

@@             Coverage Diff             @@
##           staging    #1974      +/-   ##
===========================================
+ Coverage     86.6%   87.34%   +0.74%     
===========================================
  Files          167      169       +2     
  Lines        31752    33415    +1663     
===========================================
+ Hits         27498    29187    +1689     
+ Misses        4254     4228      -26
Impacted Files Coverage Δ
chain/network/src/test_utils.rs 95.34% <ø> (+3.39%) ⬆️
chain/client/tests/challenges.rs 95.62% <ø> (-0.09%) ⬇️
test-utils/testlib/src/node/mod.rs 100% <ø> (ø) ⬆️
chain/client/src/lib.rs 0% <ø> (ø) ⬆️
...ime/near-vm-runner/tests/test_invalid_contracts.rs 83.33% <ø> (ø) ⬆️
chain/client/src/types.rs 79.52% <ø> (-3.29%) ⬇️
near/tests/sync_nodes.rs 97.87% <ø> (-0.04%) ⬇️
chain/pool/src/types.rs 71.42% <ø> (ø) ⬆️
near/src/shard_tracker.rs 76.19% <ø> (ø) ⬆️
core/store/src/lib.rs 88.69% <0%> (-2.73%) ⬇️
... and 124 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 519fa84...abdd330. Read the comment docs.

@@ -186,7 +186,14 @@ impl User for RuntimeUser {
let state_update = self.client.read().expect(POISONED_LOCK_ERR).get_state_update();
self.trie_viewer
.view_account(&state_update, account_id)
.map(|account| account.into())
.map(|account| AccountView {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be clone()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you elaborate, please?

core/primitives/src/account.rs Outdated Show resolved Hide resolved
core/primitives/src/account.rs Outdated Show resolved Hide resolved
core/primitives/src/account.rs Outdated Show resolved Hide resolved
core/primitives/src/account.rs Outdated Show resolved Hide resolved
core/primitives/src/account.rs Outdated Show resolved Hide resolved
core/primitives/src/views.rs Outdated Show resolved Hide resolved
core/primitives/src/utils.rs Outdated Show resolved Hide resolved
account_length_baseline_cost_per_block,
storage_cost_byte_per_block,
);
let actual_charge = std::cmp::min(account.amount, charge);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably expose the full unpaid rent here and handle overflow in other places.

core/primitives/src/views.rs Outdated Show resolved Hide resolved
test-utils/testlib/src/user/runtime_user.rs Outdated Show resolved Hide resolved
@lexfrl lexfrl requested a review from evgenykuzyakov January 24, 2020 13:09
Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov left a comment

Choose a reason for hiding this comment

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

wait

Copy link
Collaborator

@evgenykuzyakov evgenykuzyakov left a comment

Choose a reason for hiding this comment

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

Do you mind removing:

let actual_charge = std::cmp::min(account.amount, charge);

And move it into the apply_rent?

@lexfrl
Copy link
Contributor Author

lexfrl commented Jan 24, 2020

Do you mind removing:

let actual_charge = std::cmp::min(account.amount, charge);

And move it into the apply_rent?

Btw, if we do:

let rent = calculate_rent(
        account_id,
        account,
        block_index,
        runtime_config.account_length_baseline_cost_per_block,
        runtime_config.storage_cost_byte_per_block,
    );
    let actual_charge = std::cmp::min(account.amount, rent);
    account.amount -= rent;
    account.storage_paid_at = block_index;

It means account can pay any rent with the amount left in his account? Or we have a check earlier? But if the code above guarantees that account.amount will never be less then rent, why we even do std::cmp::min?

@evgenykuzyakov
Copy link
Collaborator

That's right, but the account will likely be deleted before it pays the rent by someone who's watching account balances.

@lexfrl
Copy link
Contributor Author

lexfrl commented Jan 24, 2020

That's right, but the account will likely be deleted before it pays the rent by someone who's watching account balances.

Just curious, what is the incentive for this activity if all the balance will be spent to cover rent?

@evgenykuzyakov
Copy link
Collaborator

Just curious, what is the incentive for this activity if all the balance will be spent to cover rent?

The idea is you can kill the account earlier, before balance got to 0. E.g. you have poke_threshold which is about 1 day of rent. So you have an incentive to nuke the account before it loses it all due to rent.

@lexfrl
Copy link
Contributor Author

lexfrl commented Jan 24, 2020

The idea is you can kill the account earlier, before balance got to 0. E.g. you have poke_threshold which is about 1 day of rent. So you have an incentive to nuke the account before it loses it all due to rent.

So if nobody was fast enough to delete an account that incentive to delete disappears?

@evgenykuzyakov
Copy link
Collaborator

So if nobody was fast enough to delete an account that incentive to delete disappears?

It's still an incentive for node validators. Since they are still paying for storage.

runtime/runtime/src/actions.rs Outdated Show resolved Hide resolved
@lexfrl
Copy link
Contributor Author

lexfrl commented Jan 24, 2020

It's still an incentive for node validators. Since they are still paying for storage.

Will be this "collective" incentive enough to cover the DeleteAction transaction cost for a signer?

@evgenykuzyakov
Copy link
Collaborator

Will be this "collective" incentive enough to cover the DeleteAction transaction cost for a signer?

I hope so, otherwise we'll have to bump the poke_threshold

@MaksymZavershynskyi
Copy link
Contributor

Closing this PR as explained in #1811 in favor of #2007

@MaksymZavershynskyi
Copy link
Contributor

Reopening since we discussed that this is needed.

@chefsale chefsale deleted the fckt/actual_amount branch June 4, 2020 10:26
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.

4 participants