[NEP-491] Non-refundable storage #9600

jakmeier · 2023-09-27T15:30:30Z

Context

Please note that action name was not agreed upon (see the end of the NEP discussion for more details). For now, I've used NonrefundableStorageTransfer name that was proposed in NEP-491: Non-refundable storage NEPs#491 (comment).

Goal

We aim to enable the sponsorship of accounts with arbitrary storage (for example, with a contract that exceeds the zero balance account limit). Currently, there is a financial incentive to deplete such a sponsor by deleting the account and then cashing in.

Summary

This PR introduces a non-refundable balance, which is a type of balance that can only be transferred to an account at the time of its creation. It is burned upon account deletion. This change eliminates the possibility of cashing out sponsored accounts.

Changes

Add protocol_feature_nonrefundable_transfer_nep491 and change nightly protocol version.
Introduce AccountVersion::V2 as the new default version.
Extend Account with a nonrefundable field that will only be used by accounts V2 or higher.
Introduce NonrefundableStorageTransfer action that behaves similar to Transfer but it modifies the nonrefundable instead of the amount account field. Both nonrefundable and amount are used to calculate the storage allowance.
Introduce new error type NonRefundableBalanceToExistingAccount that it is raised on an attempt to make a non-refundable transfer outside of a transaction that creates an account.
Use serialization sentinel (u128::MAX) as a first field during Borsh serialization in case of V2 accounts, to distinguish between V1 and V2 accounts. From V2, the version field is Borsh serialized and subsequent versions can use that field.
Add custom Serde deserialization that uses V1 version in case the version field is missing. This is required when we parse the mainnet genesis file, so that the mainnet genesis hash does not change.
Add non-refundable transfer tests.
Add serde/borsh (de)serialization tests for all types of account.
Update balance checker, other_burnt_amount is updated when deleting an account that had a non-refundable balance. Update related tests.
Minor refactors.

Original description of a draft implementation by @jakmeier

The purpose of this reference implementation is to show the proposed protocol changes in practice.
It should be sufficient to convince the reader that all technical challenges have been identified and that there is a solution.
(If that's your goal, I recommend looking at only the changes of the first two commits, as the following commits contain only non-essential changes, such as fixing tests.)

However, this is not a full implementation. (edit: finishing as much as possible has started, some items are ~~done~~)

~~tests are not fixed~~

~~boilerplate code changes are still left as todo!()~~

~~(rosetta) rpc node code does not compile~~

~~changes are not behind feature flags~~

~~some refactoring would make sense~~

need to implement more tests for the new feature

some questions regarding desired behavior need to be decided in NEP discussion (What happens on delete? Is a top-up after creation possible?)

naming should be decided as part of the NEP discussion => might require variable renaming

implementation details for ActionView / AccountView need to be decided (with whom?)

Implementation details around account parsing of old/new versions and its error handling needs to be agreed upon. The PR contains a suggestion.

The purpose of this reference implementation is to show the proposed protocol changes in practice. It should be sufficient to convince the reader that all technical challenges have been identified and that there is a solution. However, this is not a full implementation. - tests are not fixed - boilerplate code changes are left as todo!() - (rosetta) rpc node code does not compile - changes are not behind feature flags - some refactoring would make sense

introduces the error variant `NonRefundableBalanceToExistingAccount`

…reference_implementation

includes first integration test and TODOs for missing tests

jakmeier · 2023-09-27T15:32:04Z

This PR replaces #9346 from a branch in nearcore, allowing @jbajic and others to contribute as well.

akhi3030 · 2023-11-14T10:57:53Z

@jbajic: I will close this. We can reopen if and when we decide to progress on this approach for the NEP.

jbajic · 2023-11-14T12:36:29Z

@jbajic: I will close this. We can reopen if and when we decide to progress on this approach for the NEP.

@akhi3030 I should have updated the PR, but I will be working on it, as was decided in the conversation that happened online. Posting the relevant comment here near/NEPs#491 (comment)

…mplementation

jakmeier

@staffik Thank you for cleaning a lot of this up and making it all ready! It looks pretty good already.

I left several comments regarding changes I think you should consider. I think most things, if not all, could also be done in follow-up PRs. As long as everything is addressed (or dismissed) before the feature gets stabilized, it should be fine IMO.

But that's obviously your choice how you want to split the PRs. And a code owner will have to decide if it's really ready for merging to master.

jakmeier · 2024-01-27T22:56:03Z

chain/rosetta-rpc/src/adapters/mod.rs

@@ -352,6 +352,36 @@ impl From<NearActions> for Vec<crate::models::Operation> {
                    );
                }

+                #[cfg(feature = "protocol_feature_nonrefundable_transfer_nep491")]
+                // Note(jakmeier): Both refundable and non-refundable transfers are considered as available balance.


I don't remember checking that this specific behaviour is what we want. I think I've just put it in to have something in the prototype. But considering how the NEP discussion went, I wonder if it wouldn't make more sense to consider the nonrefundable balance burnt instead of showing it as balance on the receiving account?

Can we check with relevant parties how the nonrefundable storage balance should show up in the rosetta RPC?
Ideally, someone who feels responsible for the Rosetta RPC should review all Rosetta RPC changes.
I hope @firatNEAR or @walnut-the-cat can help to find the right person?

The main question to resolve:

When an account has nonrefundable storage balance, should it show up as balance in the Rosetta RPC adapter?

(oh and once this is resolved, I don't think we need to attribute the note to specific person, git blame should work better to find he source of a comment as it leads to the actual PR discussion)

Added a TODO comment to consider this before stabilizing.

I think this is a question that we should ask Coinbase since they own the spec. They usually are okay with making changes as long as they confirm from their side. @gmilescu could you help me with this, I am a bit out of loop with Coinbase.

This change should be fine the way Rosetta handles this scenario. Thanks for checking.

@nasmithan Could you take a look at this change?
In discussion we decided to burn the nonrefundable balance instead of showing it as balance on the receiving account.

Yup that should be fine

core/primitives-core/src/account.rs

core/primitives/src/test_utils.rs

runtime/runtime/src/balance_checker.rs

jakmeier · 2024-01-28T00:57:10Z

runtime/runtime/src/config.rs

+            #[cfg(feature = "protocol_feature_nonrefundable_transfer_nep491")]
+            NonrefundableStorageTransfer(_) => {
+                // Account for implicit account creation
+                transfer_send_fee(


I just noticed that we are now reusing the same gas cost parameter for two actions.
This seems inconsistent.

Something to consider before stabilizing.

Added a TODO comment to consider this before stabilizing.

@jakmeier Do you mean estimating cost for NonrefundableStorageTransfer action (which seems to be the same as for Transfer action IMO) or just introducing sth like ActionCosts::nonrefundable_storage_transfer with the same values as ActionCosts::transfer ?

I was thinking about the second option. Using the same numerical value makes sense to me.
Creating a new parameter is mostly for consistency, which makes it easier to understand gas profiles and do all kinds of gas analysis work.

runtime/runtime/src/lib.rs

staffik · 2024-01-30T13:32:01Z

@nagisa Could you take a look at this as a code owner?

nagisa

I wouldn't say I'm deeply familiar with any of the codebase areas touched by this PR, so this review is only a best effort outcome…

chain/rosetta-rpc/src/lib.rs

core/primitives-core/src/account.rs

core/primitives/src/test_utils.rs

integration-tests/src/tests/client/features/wallet_contract.rs

runtime/runtime/src/actions.rs

…mplementation

nagisa

I do not see any further problems with the implementation code. One non-trivial operational point is that the coverage report is not favourable to this PR – it would be ideal to make sure the uncovered lines get tests added for them. I don't expect a full coverage, but I see entire match branches not being covered, which ain't great.

We're also waiting on a resolution of that Coinbase question.

…mplementation

staffik · 2024-02-15T13:13:40Z

@nagisa The Coinbase question has been resolved: #9600 (comment)

jakmeier added 13 commits July 25, 2023 20:14

reject non-refundable transfer to existing account

f101996

introduces the error variant `NonRefundableBalanceToExistingAccount`

handle transferV2 in rosetta RPC

ab967fe

Merge remote-tracking branch 'upstream/master' into jakmeier/nep-491_…

75ca826

…reference_implementation

refactor duplicate implicit account creation check

1b01356

cfg everything, add mapping to action view

03da556

undo cfg on account version

256a873

fix test compilation errors

c85d091

reject new action in older protocol versions

1d4d270

includes first integration test and TODOs for missing tests

add nonrefundable field to AccountView

0a7fc9d

fix balance checker

b9f35ab

add basic transfer_v2 test

595e39f

propagate account view field in conversion

65304e9

jakmeier assigned jbajic Sep 27, 2023

jakmeier mentioned this pull request Sep 27, 2023

NEP-491 reference implementation #9346

Closed

akhi3030 closed this Nov 14, 2023

jbajic reopened this Nov 14, 2023

Jure Bajic and others added 4 commits December 7, 2023 15:15

Merge branch 'master' into nep-491_reference_implementation

0953f9a

Resolve merge issues

aa34ee1

Fix compilation errors

a52fdb4

Merge remote-tracking branch 'origin/master' into nep-491_reference_i…

d67e91a

…mplementation

staffik force-pushed the nep-491_reference_implementation branch 2 times, most recently from c528fee to 0568a23 Compare December 19, 2023 16:22

Build fixes

ae36bae

staffik force-pushed the nep-491_reference_implementation branch 3 times, most recently from 3c42704 to c16c135 Compare December 20, 2023 10:26

Add comments

5cbf7dc

jakmeier commented Jan 28, 2024

View reviewed changes

staffik added 2 commits January 29, 2024 13:16

Nit fixes

77b47d2

Disable multiple transfers on implicit account creation

c43bee3

Use protocol version for Account::new()

69a2287

staffik force-pushed the nep-491_reference_implementation branch from e29955d to 69a2287 Compare January 30, 2024 13:33

nagisa reviewed Jan 31, 2024

View reviewed changes

PR fixes

11f0b5e

staffik force-pushed the nep-491_reference_implementation branch from 49f41bf to 061a539 Compare February 1, 2024 16:14

Merge remote-tracking branch 'origin/master' into nep-491_reference_i…

efa4c98

…mplementation

staffik force-pushed the nep-491_reference_implementation branch from 061a539 to efa4c98 Compare February 2, 2024 07:11

staffik requested a review from nagisa February 2, 2024 07:36

staffik mentioned this pull request Feb 2, 2024

[ProjectTracking] Non-refundable storage #10551

Closed

7 tasks

Return error instead of panicking

0b1a097

nagisa reviewed Feb 2, 2024

View reviewed changes

akhi3030 removed their request for review February 5, 2024 10:02

staffik mentioned this pull request Feb 7, 2024

[ProjectTracking]: Non refundable storage near/near-one-project-tracking#8

Open

staffik added 3 commits February 15, 2024 10:21

Merge remote-tracking branch 'origin/master' into nep-491_reference_i…

29d0f31

…mplementation

build fixes

38a7937

Add tests

f49adef

staffik force-pushed the nep-491_reference_implementation branch from 80d296f to f49adef Compare February 15, 2024 12:50

staffik requested a review from nagisa February 15, 2024 13:12

nagisa approved these changes Feb 21, 2024

View reviewed changes

nagisa added this pull request to the merge queue Feb 21, 2024

Merged via the queue into master with commit 8ffa7f2 Feb 21, 2024
22 of 23 checks passed

nagisa deleted the nep-491_reference_implementation branch February 21, 2024 12:01

staffik mentioned this pull request Apr 8, 2024

[NEP-491] Different cost parameters for Transfer and NonrefundableStorageTransfer. #10977

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEP-491] Non-refundable storage #9600

[NEP-491] Non-refundable storage #9600

jakmeier commented Sep 27, 2023 •

edited by staffik

Loading

jakmeier commented Sep 27, 2023

akhi3030 commented Nov 14, 2023

jbajic commented Nov 14, 2023 •

edited

Loading

jakmeier left a comment

jakmeier Jan 27, 2024

staffik Jan 30, 2024

firatNEAR Feb 1, 2024

nasmithan Feb 14, 2024

staffik Mar 7, 2024

nasmithan Mar 7, 2024

jakmeier Jan 28, 2024

staffik Jan 30, 2024

staffik Feb 2, 2024

jakmeier Feb 2, 2024

staffik commented Jan 30, 2024

nagisa left a comment

nagisa left a comment •

edited

Loading

staffik commented Feb 15, 2024

[NEP-491] Non-refundable storage #9600

[NEP-491] Non-refundable storage #9600

Conversation

jakmeier commented Sep 27, 2023 • edited by staffik Loading

Context

Goal

Summary

Changes

Original description of a draft implementation by @jakmeier

jakmeier commented Sep 27, 2023

akhi3030 commented Nov 14, 2023

jbajic commented Nov 14, 2023 • edited Loading

jakmeier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staffik commented Jan 30, 2024

nagisa left a comment

Choose a reason for hiding this comment

nagisa left a comment • edited Loading

Choose a reason for hiding this comment

staffik commented Feb 15, 2024

jakmeier commented Sep 27, 2023 •

edited by staffik

Loading

jbajic commented Nov 14, 2023 •

edited

Loading

nagisa left a comment •

edited

Loading