8000 refactor: check if beneficiary account exists, require confirmation by judith-Near · Pull Request #999 · near/near-cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content
This repository was archived by the owner on Oct 4, 2024. It is now read-only.

refactor: check if beneficiary account exists, require confirmation #999

Merged
merged 4 commits into from
Jun 28, 2022

Conversation

judith-Near
Copy link
Contributor
@judith-Near judith-Near commented Jun 27, 2022

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.

Copy link
Contributor
@BenKurrek BenKurrek left a comment

Choose a reason for hiding this comment

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

lgtm

@judith-Near judith-Near merged commit 4b4a787 into master Jun 28, 2022
@volovyks
Copy link
Collaborator

@judith-Near @BenKurrek looks like this PR broke tests.

@BenKurrek
Copy link
Contributor

@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) {
Copy link
Collaborator
@volovyks volovyks Jul 25, 2022

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.

Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Collaborator

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0