-
Notifications
You must be signed in to change notification settings - Fork 93
refactor: check if beneficiary account exists, require confirmation #999
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@judith-Near @BenKurrek looks like this PR broke tests. |
@volovyk-s oop yea it looks like the tests have to be rewritten? |
console.log(`Account ${options.accountId} for network "${options.networkId}" was deleted.`); | ||
const beneficiaryAccount = await near.account(options.beneficiaryId); | ||
// beneficiary account does not exist if there are no access keys | ||
if (!(await beneficiaryAccount.getAccessKeys()).length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@judith-Near If the account has 0 AC. it does not mean it does not exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- geAccessKeys() can return
null
, it will crash the execution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah... I misunderstood what's happening here. I was under the impression that this would throw an error if the account doesn't exist and you're trying to query for the keys. That's on me. Apologies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe it will. It's better to test it. But logic seems wrong anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed here: https://github.com/near/near-cli/pull/1009/files
related to issue #994
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.