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

NEP-491: Non-refundable storage #491

Merged
merged 14 commits into from
Oct 1, 2024
Merged

Conversation

@jakmeier jakmeier changed the title NEP: Non-refundable storage NEP-491: Non-refundable storage Jul 25, 2023
@jakmeier jakmeier marked this pull request as ready for review July 26, 2023 11:23
@jakmeier jakmeier requested a review from a team as a code owner July 26, 2023 11:23
@jakmeier
Copy link
Contributor Author

I believe this is ready to start the NEP process.
@near/nep-moderators can you please take a look if it meets the standards to be moved to REVIEW?

cc @DavidM-D @akhi3030

@frol
Copy link
Collaborator

frol commented Jul 29, 2023

Thank you @jakmeier for submitting this NEP.

As a moderator, I reviewed this NEP and it meets the proposed template guidelines. I am moving this NEP to the REVIEW stage and would like to ask the @near/wg-protocol working group members to assign 2 Technical Reviewers to complete a technical review (see expectations below).

Just for clarity, Technical Reviewers play a crucial role in scaling NEAR ecosystem as they provide their in-depth expertise in the niche topic while work group members can stay on guard of the NEAR ecosystem. The discussions may get too deep and it would be inefficient for each WG member to dive into every single comment, so NEAR Developer Governance designed this process that includes subject matter experts helping us to scale by writing a summary with the raised concerns and how they were addressed.

Technical Review Guidelines
  • First, review the proposal within one week. If you have any suggestions that could be fixed, leave them as comments to the author. It may take a couple of iterations to resolve any open comments.

  • Second, once all the suggestions are addressed, produce a Technical Summary, which helps the working group members make a weighted decision faster. Without the summary, the working group will have to read the whole discussion and potentially miss some details.

Technical Summary guidelines:

  • A recommendation for the working group if the NEP is ready for voting (it could be approving or rejecting recommendation). Please note that this is the reviewer's personal recommendation.

  • A summary of benefits that surfaced in previous discussions. This should include a concise list of all the benefits that others raised, not just the ones that the reviewer personally agrees with.

  • A summary of concerns or blockers, along with their current status and resolution. Again, this should reflect the collective view of all commenters, not just the reviewer's perspective.

Here is a nice example and a template for your convenience:


### Recommendation

Add recommendation

### Benefits

* Benefit

* Benefit

### Concerns

| # | Concern | Resolution | Status |

| - | - | - | - |

| 1 | Concern | Resolution | Status |

| 2 | Concern | Resolution | Status |

Please tag the @near/nep-moderators once you are done, so we can move this NEP to the voting stage. Thanks again.

@frol frol added WG-protocol Protocol Standards Work Group should be accountable A-NEP A NEAR Enhancement Proposal (NEP). S-review/needs-wg-to-assign-sme A NEP that needs working group to assign two SMEs. labels Jul 29, 2023
@jakmeier
Copy link
Contributor Author

@near/wg-protocol reminder, can you please assign SMEs as frol asked 2 weeks ago? This effort is entirely blocked on your input here. Thanks!

Copy link

@norwnd norwnd left a comment

Choose a reason for hiding this comment

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

Hi, just some newbie comments here, feel free to ignore if I don't make much sense.

neps/nep-0491.md Outdated Show resolved Hide resolved
neps/nep-0491.md Outdated Show resolved Hide resolved
Comment on lines +329 to +330
- How should a wallet display non-refundable balances? (proposal: up to wallet
providers, probably a new separate field)
Copy link

@norwnd norwnd Aug 10, 2023

Choose a reason for hiding this comment

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

Do wallets even need to display non-refundable balance to users (what for) ? My impression was "non-refundable balance" is working specifically towards hiding the complexity of storage-staking from users, is this even an achievable goal ? Blockchain explorers would probably need to display "non-refundable balance" though at least for info/debugging purposes.

On a similar topic, the name "non-refundable balance" might be unnecessarily vague given the intent of this proposal, wouldn't something like "dedicated storage balance" or "storage-tied balance"/"storage-restricted balance" better describe the concept ? Since it's gonna show up in blockchain explorers this makes it a bit more self-explanatory, I think. Unless it does become a more generic concept eventually (in which case more generic name is warranted ofc).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @norwnd , thanks for your input, it's very much welcome! :)

Do wallets even need to display non-refundable balance to users (what for) ?

Not displaying it is a valid choice for a wallet IMO. But right now, I assume most wallets are doing some kind of computation to show how much of the balance is currently open for withdrawal, vs the amount that's reserved to cover storage staking. If they have this computation, they need to update how it works once this proposal gets through.

My impression was "non-refundable balance" is working specifically towards hiding the complexity of storage-staking from users, is this even an achievable goal ?

Interesting question. The original motivation here isn't so much to simplify things. It's more about enabling a certain use case. But I can see how, if presented correctly, we can maybe make it simpler from a user point of view, as they don't even have to know there is storage staking for their account. I like that!

wouldn't something like "dedicated storage balance" or "storage-tied balance"/"storage-restricted balance" better describe the concept ?

I'm open to name changes. I very much dislike "non-refundable balance". But I would like to have a clear distinction between "refundable balance that's currently locked due to storage usage" (which we already have today) and the new concept of "balance that can only ever be used for storage in this account and will be burnt when the account is deleted". In your suggestions, it seems like the line between the two is blurry.

I've considered "storage credits", but that sounds like you can trade them between accounts.

Another idea: Perhaps we should just "purchase" an amount of bytes, and add a field like free_bytes, as in, no storage staking required for these bytes. Sounds a bit crazy to me but maybe that would make the concept easier to grasp. WDYT?

Copy link

Choose a reason for hiding this comment

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

In your suggestions, it seems like the line between the two is blurry.

Yep, it was just to illustrate that point, your free_bytes concept feels much better (alternative names to consider: purchased/prepayed/bought+_+bytes/storage perhaps).

Copy link

@norwnd norwnd Aug 11, 2023

Choose a reason for hiding this comment

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

The original motivation here isn't so much to simplify things. It's more about enabling a certain use case. But I can see how, if presented correctly, we can maybe make it simpler from a user point of view, as they don't even have to know there is storage staking for their account.

Long term though, UX-wise both for users and devs the "Alternative: Move away from storage staking" is still probably desirable, if achievable in sustainable manner.

@birchmd
Copy link
Contributor

birchmd commented Aug 10, 2023

@bowenwang1996 I'm not sure who to nominate here as a technical reviewer on behalf of the protocol working group. Do you have someone in mind?

@akhi3030
Copy link
Contributor

@bowenwang1996 I'm not sure who to nominate here as a technical reviewer on behalf of the protocol working group. Do you have someone in mind?

@birchmd, @bowenwang1996: I am happy to volunteer.

Comment on lines +40 to +43
Non-refundable storage staking is a further improvement over
[NEP-448](https://github.com/near/NEPs/pull/448) (Zero Balance Accounts) which
addressed the same issue but is limited to 770 bytes per account. By lifting the
limit, sponsored accounts can be used in combination with smart contracts.
Copy link

Choose a reason for hiding this comment

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

Perhaps the following part could use a bit more details (expanded on, in some section below):

By lifting the limit

with respect to how it interacts with "free 770 bytes per account" concept. Because strictly speaking, the way I understand this prop, it's not exactly a generalization of #448 but rather a standalone feature that extends it conceptually.

It seems both features interplay nicely together, just want to make sure it doesn't create unnecessary burden for developers when they decide which one to go with.

@bowenwang1996
Copy link
Collaborator

As a protocol working group member, I nominate @akhi3030 and @birchmd as SME reviewers for this NEP.

Copy link
Contributor

@birchmd birchmd left a comment

Choose a reason for hiding this comment

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

Thanks for the proposal @jakmeier ! I am providing my SME review as requested by @bowenwang1996

I appreciate your thoroughness in describing alternatives to this proposal. You anticipated many of the questions I would have asked!

My remaining concern is if the additional protocol complexity added by this change is justified by the claimed benefit of removing the incentive to spam and delete free accounts. It is true that it removes the "first-order" incentive for this behaviour, but there are "second-order" incentives a bad actor could have. For example, someone who holds many NEAR tokens may be incentivized to cause a large number of NEAR tokens to be taken out of circulation as a means of inflating the NEAR price. Or perhaps a competitor to the business offering the free accounts would be happy to drain the funds of their rival to gain market advantage.

My main point is that business that are offering pre-funded accounts to users will probably still need to implement some kind of spam protection themselves because there could still be bad actors that benefit from spamming. And if it is true that all businesses are implementing this spam protection themselves then do we also need this non-refundable protection in the protocol?

I understand this is a somewhat subjective question. If the community is asking for this feature then it probably provides value to them. But I just want to make sure we are recognizing that this is not a silver bullet to solve free account spam issues.

@jakmeier
Copy link
Contributor Author

@birchmd thanks for the review! You raise a very good question! I completely agree that we should carefully consider how important the feature is and weigh it against added complexity in the protocol.

@DavidM-D can you maybe make a stronger case for why we need a solution to the storage-refund problem? Specifically answering the comment by @birchmd .

@wacban
Copy link
Contributor

wacban commented Aug 28, 2023

someone who holds many NEAR tokens may be incentivized to cause a large number of NEAR tokens to be taken out of circulation as a means of inflating the NEAR price

This is always going to be possible regardless of this proposal. Anyone holding near can just "not use it" or even go as far as "burn" it e.g. by sending it to an unusable account like implicit account with address 00000.... .

What I would worry about is the malicious case where an attacker drains the funds allocated for free accounts by spamming. For that reason I agree that we need spam protection. I don't think it's feasible as part of protocol as we cannot really tell apart spamming from increased traffic. I would rather see the business implement its own, app level, solution to this problem.

@wacban
Copy link
Contributor

wacban commented Aug 28, 2023

To put my two cents in, I am OK with the current proposal but I would preferr the "Allow refunds to original sponsor" variation. I consider this a more future-proof implementation. Based on the NEP it seems like it is not needed today but it is also not harmful to the users. The drawback is the added complexity but in my opinion it's worth it.

@frol frol removed the S-review/needs-wg-to-assign-sme A NEP that needs working group to assign two SMEs. label Aug 28, 2023
@bowenwang1996
Copy link
Collaborator

As a working group member, I lean towards rejecting this proposal for the following reasons:

  • What Marcelo proposed in this comment seems like a sensible solution that does not require changes to the protocol. It does create a bit more work on the application level, but we already did similar work in the past to support multisig.
  • It seems to me that the main issue this proposal tries to address is the cost of deploying a smart contract on user accounts, which is a nontrivial amount that users can take back by deleting their account. I definitely agree that this is a problem but I'd rather that we tackle this specific problem through approaches like global smart contracts, which would also be needed down the line for web3 wallet support. As a result, I am not sure how much value this proposal would provide.
  • This proposal introduces an additional 16 bytes of storage per account, which is roughly a 20% overhead. I think we should always be very careful with putting more data in the state to avoid unnecessarily growth in state size.

@mm-near
Copy link

mm-near commented Dec 13, 2023

As a working group member, I lean towards rejecting this proposal:

  • Let's start by rolling out the proxy account feature and then see what the community thinks. If there are many scenarios where it doesn't quite fit, we can always circle back to discuss this NEP again.
  • Introducing a new Action within the system carries considerable implications across various components, including wallets and other related systems. Such an addition should be considered a measure of last resort especially for actions which involve the movement of tokens. It presents a potential opportunity for a grief attack, where an attacker could deceive a user into creating an account and 'ReserveStorage' for a disproportionately large amount of tokens.

@bowenwang1996
Copy link
Collaborator

Posting on behalf on the protocol working group:

  • We have some concerns regarding this protocol change, including not being the best option to address the problem and introducing a new action that needs support by all tooling
  • At the same time, the option of not making the protocol change poses its own challenges and could be more complex to adopt since it requires support from different wallets.

Considering that storage refund is a major blocker for a few important applications, notably fast auth account recovery, the protocol working group agreed to approve this NEP to unblock them.

@walnut-the-cat
Copy link
Contributor

walnut-the-cat commented Dec 15, 2023

During Protocol working group meeting, @mm-near suggested updating the action name to clarify that token will be burnt when reserving storage.

The currently proposed name is ReserveStorage and I would like to collect feedback and get alignment on what the new name should be.

To keep the discussion moving, here is my proposal on naming principles

  • Purpose first then consequence. Action name should emphasize what the action is for first before listing side effects.
  • Avoid confusing words. Stake or reserve shall be avoid as they imply non-permanent, reversible behavior.
  • Stay simple. There may be multiple outcomes and side effects of the action, but focus on the most critical ones.

When it comes to the actual naming, my brain doesn't go further than something like GrantStorageByBurningToken. If anyone has a suggestion/thought, happy to hear.

cc. @staffik

@Sarah-NEAR
Copy link

Sarah-NEAR commented Dec 15, 2023

Thank you to everyone who attended the sixth Protocol Work Group call today! The work group members reviewed the NEP and reached the following consensus:

Status: Approved

👍 @birchmd : https://github.com/near/NEPs/pull/491#issuecomment-1850078450
👍 @mfornet : #491 (comment)
👎 @mm-near : #491 (comment)
👎 @bowenwang1996: #491 (comment)

After further discussion, Bowen broke the tie and the working group approved the NEP.

Meeting Recording:
https://bit.ly/483Wo6X

@jakmeier Thank you for authoring this NEP!

Next Steps:
@walnut-the-cat finalize decision on renaming ReserveStorage

@Sarah-NEAR Sarah-NEAR added S-approved A NEP that was approved by a working group. and removed S-voting/final-comment-period A NEP in the final REVIEW stage. Last chance to bring up concerns before the voting call. labels Dec 15, 2023
@walnut-the-cat
Copy link
Contributor

During Protocol working group meeting, @mm-near suggested updating the action name to clarify that token will be burnt when reserving storage.

The currently proposed name is ReserveStorage and I would like to collect feedback and get alignment on what the new name should be.

To keep the discussion moving, here is my proposal on naming principles

* _Purpose_ first then _consequence_. Action name should emphasize what the action is for first before listing side effects.

* Avoid confusing words. _Stake_ or _reserve_ shall be avoid as they imply non-permanent, reversible behavior.

* Stay simple. There may be multiple outcomes and side effects of the action, but focus on the most critical ones.

When it comes to the actual naming, my brain doesn't go further than something like GrantStorageByBurningToken. If anyone has a suggestion/thought, happy to hear.

cc. @staffik

@jakmeier , @birchmd , @mfornet , @DavidM-D , please share your thoughts on the naming :)

@birchmd
Copy link
Contributor

birchmd commented Dec 19, 2023

@walnut-the-cat I like your guidelines for naming! My only issue with GrantStorageByBurningToken is how long it is. Would StoragePurchase work as a name? "Purchase" can sometimes have the connotation of the possibility of a refund, but not always (whereas I think "reserve" essentially always has the connotation of being able to cancel).

Another idea could be to intentionally make the name only make sense when part of a larger transaction (since this type of action can only happen as part of a batch transaction creating a new account). Maybe call it WithNonrefundableStorage. "With" at the front is meant to signify it is a sort of extra action. In a transaction definition it might look nice

Transaction {
    actions: [
        Create { account_id: "..." },
        WithNonrefundableStorage { amount: "..." },
        Deploy { code: "..." }
    ],
    ...
}

@jakmeier
Copy link
Contributor Author

jakmeier commented Dec 20, 2023

WithNonrefundableStorage is neat. I like how it makes it obvious it is not a general purpose action.

Some people didn't like "nonrefundable" to begin with. But I just can't think of something that sounds nicer without becoming more vague. The alternatives I found in the discussion are "dedicated storage balance", "storage-tied balance", "storage-restricted balance" by @norwnd in their comment in August. To me, these sound more vague than nonrefundable storage but I think it's worth brining them up again here.
And I acknowledge that it can be confusing for users to see a balance called "nonrefundable storage" listed in their wallet.

StoragePurchase would also work. But I imagine that can also lead to confusion. Some people might see this action in the docs and conclude they have to purchase storage explicitly every time, when in reality this is only needed in very specific circumstances. The same goes for an BurnForStorage which was proposed a month ago by @frol among other suggestions.

In conclusion, I personally think WithNonrefundableStorage is the most purpose driven and the least confusing of all the names proposed so far. It's not the simplest name but it feels like a trade-off worth making. And for UX, I would push the problem to wallet implementations. They can always present a term that's easier to understand to their users if they don't want to use "nonrefundable storage" in their UI.

@frol
Copy link
Collaborator

frol commented Dec 21, 2023

WithNonrefundableStorage is neat. I like how it makes it obvious it is not a general purpose action (it looks out of place along side with "CreateAccount", "DeleteAccount", "Transfer", ... verbs).

This semantically limits this action to account creation context (at least in my head), and I don't see a reason to have this limitation. I don't like the idea of introducing "WithNonrefundableStorage" as an Action.

One suggestion that was not considered here is to introduce a more generic extendible action (draft naming ConfigureAccount):

{
  "actions": {
    "CreateAccount": {},
    "Transfer": {
      "deposit": "500100",
    },
    "AddKey": ...,
    "ConfigureAccount": [
      {
        "property": "NONREFUNDABLE_STORAGE",
        "value": "500000"
      }
    ]
  }
}

The benefits I see here:

  • It is more future-proof and potentially other properties could be set this way
  • The Transfer action still includes full transferred amount, so existing tooling won't need to have a special treatment (we already need to handle FunctionCall.deposit and Transfer.deposit, so it is not a huge problem, but may smooth out the adoption)
  • It communicates the intent cleaner

The downside probably rules the benefits out:

  • It seems that this action should only be called during account creation or by the account itself, so in order to increase the NONREFUNDABLE_STORAGE after the account is created, we would need to (1)Transfer from a business account and (2) "ask" user to submit ConfigureAccount action - we have a disconnect here.

@birchmd
Copy link
Contributor

birchmd commented Dec 21, 2023

A general ConfigureAccount action is a nice idea. I agree it makes the change more extensible in the future. But it feels like a bit of scope creep in this NEP so I wonder if it is too late to include this modification to the proposal.

This semantically limits this action to account creation context

Yes, this was my intention. The current NEP says

If a non-refundable transfer arrives at an account that already exists, it will fail and the funds are returned to the predecessor.

so I wanted the name of the action to reflect this.

@jakmeier
Copy link
Contributor Author

Yeah the proposal as it stands actually only works for account creation.

Using it in more general ways might sound appealing. But as discussed in the public WG meeting, there is no desire to use it more generally and actually it is quite a lot more work to do. Putting in this extra work would be energy spent on the wrong priorities. Among other reasons, because the long-term solution to storage costs probably will look different from today's design.

@frol
Copy link
Collaborator

frol commented Dec 21, 2023

Yeah the proposal as it stands actually only works for account creation.
But it feels like a bit of scope creep in this NEP so I wonder if it is too late to include this modification to the proposal.

I don't want to drop bombs here, so I will leave it up to the WG. Another naming idea is CreateAccountWithNonrefundableStorage, so at least it is aligned with the other actions being verbs.

@staffik
Copy link

staffik commented Jan 3, 2024

Yeah the proposal as it stands actually only works for account creation.
But it feels like a bit of scope creep in this NEP so I wonder if it is too late to include this modification to the proposal.

I don't want to drop bombs here, so I will leave it up to the WG. Another naming idea is CreateAccountWithNonrefundableStorage, so at least it is aligned with the other actions being verbs.

For a named account creation the transaction would be:

CreateAccount
CreateAccountWithNonrefundableStorage
...

that looks a bit weird to me.

The original (temporary) name for the action was TransferV2 and the existing implementation allows to create implicit accounts with non-refundable storage (using TransferV2 as the only action in a transaction). We can disallow it but I don't see a reason to add this limitation. However, WithNonrefundableStorage as the only action in a transaction would look weird too.

@staffik
Copy link

staffik commented Jan 4, 2024

What about NonrefundableStorageTransfer ?
It conveys 3 points:

  • it is a transfer
  • it is for storage
  • it is non-refundable

It is quite close to WithNonrefundableStorage yet it is a verb and it makes sense both in

CreateAccount
NonrefundableStorageTransfer
...

and as a standalone action for implicit account creation.

It is quite long but I could not find a better alternative to the Nonrefundable part. I considered LockedStorageTransfer but it might suggest that it can be unlocked later.

@staffik
Copy link

staffik commented Jan 11, 2024

For anyone interested, the PR is ready for review near/nearcore#9600.
I've used NonrefundableStorageTransfer action name because lack of a feedback, but it can be changed easily during the review process.

@bowenwang1996
Copy link
Collaborator

@staffik @jakmeier question: it looks like the nonrefundable balance is essentially burnt but the burning is not recorded on the protocol level. What was the rationale behind this decision vs explicitly burning the balance and record permanent_storage_bytes on the account (instead of nonrefundable) and modify the invariant to be

amount + locked >= max(0, storage_usage - permanent_storage_bytes) * storage_amount_per_byte

This way the burning of tokens is reflected on the protocol level (total supply is decreased). At the same time, it is less confusing for users and integration partners who need to understand what nonrefundable means

@staffik
Copy link

staffik commented Feb 18, 2024

@staffik @jakmeier question: it looks like the nonrefundable balance is essentially burnt but the burning is not recorded on the protocol level. What was the rationale behind this decision vs explicitly burning the balance and record permanent_storage_bytes on the account (instead of nonrefundable) and modify the invariant to be

amount + locked >= max(0, storage_usage - permanent_storage_bytes) * storage_amount_per_byte

This way the burning of tokens is reflected on the protocol level (total supply is decreased). At the same time, it is less confusing for users and integration partners who need to understand what nonrefundable means

I think one rationale was to make it easier if we decide to implement returning the nonrefundable balance to the sponsor. But otherwise, permanent_storage_bytes looks much better to me.

github-merge-queue bot pushed a commit to near/nearcore that referenced this pull request Feb 21, 2024
## Context
NEP: near/NEPs#491
* 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
near/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.

---------

Co-authored-by: Jure Bajic <[email protected]>
Co-authored-by: Adam Chudaś <[email protected]>
@jakmeier
Copy link
Contributor Author

@staffik @jakmeier question: it looks like the nonrefundable balance is essentially burnt but the burning is not recorded on the protocol level. What was the rationale behind this decision vs explicitly burning the balance and record permanent_storage_bytes on the account (instead of nonrefundable) and modify the invariant to be

amount + locked >= max(0, storage_usage - permanent_storage_bytes) * storage_amount_per_byte

This way the burning of tokens is reflected on the protocol level (total supply is decreased). At the same time, it is less confusing for users and integration partners who need to understand what nonrefundable means

@bowenwang1996 good point!

The question if non-refundable storage is still part of the total supply was first raised very early on. I cannot remember strong opinions on either side. Even the NEP decision was not completely clear about this, IIRC. And I believe the final decision was essentially made implicitly in the implementation in near/nearcore#9600, where I had a placeholder implementation for the reference implementation with a TODO which now morphed into the final implementation.

For arguments speaking for not counting it as burnt:

  1. As @staffik mentioned, at first we were not sure if we want to return the value back to the sponsor, hence I started with this in the reference implementation.
  2. With this implementation, whether an account is created using a non-refundable or a refundable transfer is neutral in terms of reported total supply. This means the value of an account can still be computed as liquid tokens + tokens for storage, and if we sum up all account values we get the total supply. This is a nice property from a storage-economic point of view: the total storage remains bounded by the total supply.

But after the NEP discussion finished, it also seemed to me that we should maybe just consider the token value burnt. It makes more sense from a user's perspective. Which is also reflected in my comment from Jan 27:

// 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?

I don't see a reason why we couldn't still change this if everyone here agrees it makes more sense to burn.

@bowenwang1996
Copy link
Collaborator

@staffik @jakmeier thanks for the context. Given the discussion throughout this NEP, it doesn't seem like we will end up implementing return of balance to the sponsor given its complexity. So should we just change the implementation to properly reflect that the nonrefundable balance is burnt?

@staffik
Copy link

staffik commented Feb 27, 2024

@staffik @jakmeier thanks for the context. Given the discussion throughout this NEP, it doesn't seem like we will end up implementing return of balance to the sponsor given its complexity. So should we just change the implementation to properly reflect that the nonrefundable balance is burnt?

If no one disagrees, I think we should change the implementation. I will just confirm with Coinbase if they are ok with this change.

github-merge-queue bot pushed a commit to near/nearcore that referenced this pull request Mar 28, 2024
According to the discussion:
near/NEPs#491 (comment)
we decided to use `permanent_storage_bytes` account field instead of
`nonrefundable`.
Non-refundable storage transfer amount will now we explicitly burnt and
the nonrefundable storage will now be reflected as
`permanent_storage_bytes`. This way is much less confusing for users and
integration partners.
@flmel flmel merged commit 8345bc9 into near:master Oct 1, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-NEP A NEAR Enhancement Proposal (NEP). S-approved A NEP that was approved by a working group. WG-protocol Protocol Standards Work Group should be accountable
Projects
Status: APPROVED NEPs
Development

Successfully merging this pull request may close these issues.