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

Fix: Get account by index instead of name #1135

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

manavdesai27
Copy link
Contributor

Fixes #968

Applied a temporary fix regarding the issue.

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2023

Codecov Report

Base: 69.53% // Head: 69.41% // Decreases project coverage by -0.13% ⚠️

Coverage data is based on head (01e3b0c) compared to base (57f5956).
Patch coverage: 4.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1135      +/-   ##
==========================================
- Coverage   69.53%   69.41%   -0.13%     
==========================================
  Files         158      158              
  Lines       26598    26643      +45     
==========================================
- Hits        18494    18493       -1     
- Misses       8104     8150      +46     
Impacted Files Coverage Δ
lib/client/wallet.js 54.19% <0.00%> (-0.85%) ⬇️
lib/wallet/wallet.js 68.12% <0.00%> (-1.08%) ⬇️
lib/wallet/rpc.js 48.40% <4.34%> (-0.93%) ⬇️
lib/wallet/http.js 52.66% <8.33%> (-0.88%) ⬇️
lib/golomb/reader.js 86.79% <0.00%> (-1.89%) ⬇️
lib/golomb/golomb.js 92.00% <0.00%> (-1.60%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@haxwell
Copy link

haxwell commented Jan 29, 2023

I think this PR should include tests.

@manavdesai27
Copy link
Contributor Author

manavdesai27 commented Jan 30, 2023

I think this PR should include tests.

Could you point me in the right direction as to which files I should look into regarding the same.
Edit: Don't we already have the test for getting account info, in wallet-http-test.js at line 86?

@manavdesai27
Copy link
Contributor Author

manavdesai27 commented Jan 31, 2023

I have created separate cli options to get account information via name and index, bot for rpc and http.

  • If there is an account name, with name being an integer, and we use get accountbyname, it would get the account whose name is 7, not index.
  • But if I use get accountbyindex, it would provide the account with accountIndex as 7

Screenshot from 2023-01-31 12-06-18
Screenshot from 2023-01-31 12-06-03
Screenshot from 2023-01-31 12-05-19

@manavdesai27
Copy link
Contributor Author

@pinheadmz could you please review this pull request?

@@ -608,6 +618,7 @@ class CLI {
this.log(' $ abandon [hash]: Abandon a transaction.');
this.log(' $ account create [account-name]: Create account.');
this.log(' $ account get [account-name]: Get account details.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please specify that this method gets account by name.

/**
* Get wallet account.
* @param {Number} id
* @param {Number|String} accountIndex
Copy link
Collaborator

Choose a reason for hiding this comment

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

The account index is supposed to be a Number, not a String.

@@ -901,6 +912,10 @@ class Wallet extends EventEmitter {
return this.client.getAccount(this.id, account);
}

getAccountIndex(account) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need JSDocs here.

acct = num;
}

const account = await req.wallet.getAccount(acct);
Copy link
Collaborator

Choose a reason for hiding this comment

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

You will have to create a new method in the wallet/wallet.js file and name it getAccountByAccountIndex as there is already a method named getAccountIndex. I also ask you to name all the references of getAccountIndex to getAccountByAccountIndex.

const addrs = [];

if (name === '')
name = 'default';
let acct = valid.get(0, 'default');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem right @pinheadmz could you please take a look.

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

Successfully merging this pull request may close these issues.

Get accounts by index instead of name
4 participants