near / near-cli

General purpose command line tools for interacting with NEAR Protocol
https://docs.near.org
MIT License
193 stars 93 forks source link

Add Warning and Docs for Deleting Account to Transfer NFTs and FTs #994

Closed BenKurrek closed 7 months ago

BenKurrek commented 2 years ago

When an account is deleted and funds are sent to the beneficiary, we should mention that "Deleting an account does not automatically transfer NFTs and FT to the beneficiary address. Ensure to transfer assets before deleting"

This could be a mandatory "do you want to proceed? y / n" or just a warning that's logged such that if someone didn't know that, they could quickly re-create the account and recover the assets.

SoftwareAndOutsourcing commented 2 years ago

Related: https://github.com/near/docs/pull/1093

SoftwareAndOutsourcing commented 2 years ago

Related: https://github.com/near/near-api-js/pull/883

judith-Near commented 2 years ago

This PR adds a warning This method will delete your account. Beneficiary account must exist in order to transfer all Near tokens. Make sure to send all fungible tokens or NFTs that you own to the beneficiary account prior to deleting, as this method will only transfer NEAR tokens. Do you want to proceed? that the user must confirm.

Delete will also not run if beneficiary account does not exist.

SoftwareAndOutsourcing commented 2 years ago

The root issue seems https://github.com/near/nearcore/issues/7112.

Meanwhile that PR #999 could be a workaround among the docs update from https://github.com/near/docs/pull/1093 or https://github.com/near/docs/tree/newdocs.

I would suggest including in that PR message that the account must exist and be already initialized, so this line:

https://github.com/near/near-cli/blob/88fb66831a26cc82750861edb7bdea8650a29f95/index.js#L50

Can be written as:

chalk`{bold.white This method will delete your account. The Beneficiary account must exist and be already initialized in order to transfer all Near tokens or these will be lost. Make sure to send all fungible tokens or NFTs that you own to the beneficiary account prior to deleting, as this method will only transfer NEAR tokens. Do you want to proceed? {bold.green (y/n) }}`,
judith-Near commented 2 years ago

@SoftwareAndOutsourcing im not sure if 7112 is an issue, as delete is not meant to initialize a wallet. However this PR will add the warning and a confirmation to prevent misunderstanding.

SoftwareAndOutsourcing commented 2 years ago

As I posted there it could be more of a semantic question, but it's worth some thinking. Meanwhile is nice to see the added delete call check and the ongoing work on the updated docs. Thanks!

myklemykle commented 1 year ago

Hi,

Now that I have been reminded of this issue a couple hundred times, is there a way I can silence this warning when calling account.deleteAccount() from automated tests ?

I suggest that this message, being intended for English-speaking humans, would be better implemented at the UX layer of near-cli than at the API layer of near-api-js .

mfornet commented 1 year ago

@myklemykle you can set the env variable: NEAR_NO_LOGS=1 to silence the warning: See https://github.com/near/near-api-js/pull/883