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

Add & remove a key in key_auth #213

Merged
merged 14 commits into from
Aug 3, 2017
Merged

Add & remove a key in key_auth #213

merged 14 commits into from
Aug 3, 2017

Conversation

bonustrack
Copy link
Contributor

I have some trouble making this working and interested to have your feedback.

I created 2 helpers functions: addKeyAuth and removeKeyAuth for adding and removing a key inside user account key_auths array. It's using the operation account_update to do this change (

steemBroadcast.accountUpdate(
activeWif,
userAccount.name,
owner,
active,
posting,
userAccount.memo_key,
userAccount.json_metadata,
cb
);
) .

We have already the helpers functions addAccountAuth and removeAccountAuth that working well, but for some reason when i try to add a key instead of an account i got the error "Missing Active Authority". I've verified the private active WIF and account_update operation payload and they are valid.

Any idea what is wrong?

@bonustrack
Copy link
Contributor Author

Created an issue on steem repo steemit/steem#1272

@roadscape
Copy link
Contributor

From abit in above issue:

Perhaps a serialization issue. Accounts and keys should be sorted correctly. Currently keys are converted to addresses before sorting.

@bonustrack
Copy link
Contributor Author

Awesome it's finally working with Abit suggestion :)

weight = 1,
cb
) => {
api.getAccountsAsync([username]).then(([userAccount]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If this fails the callback won't be called, you want to use one of either promises or callbacks all the way

role = 'posting',
cb
) => {
api.getAccountsAsync([username]).then(([userAccount]) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

}
}

// key not exist in authorized list
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is slightly confusing to me, too many variables, but all good

) => {
api.getAccounts([username], (err, [userAccount]) => {
if (err) { return cb(err, null); }
if (!userAccount) { return cb('Invalid account name', null); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, the first argument should always be an Error instance


exports = module.exports = steemBroadcast => {
steemBroadcast.addAccountAuth = (
activeWif,
username,
authorizedUsername,
role = "posting",
role = 'posting',
weight = 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

what if the account has a different threshold?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the account has a weight_threshold of 2 then the newly added key will not be able to broadcast alone if the weight is not set to 2. I can change the default weight to use the same value as weight_threshold.

username,
'STM88CPfhCmeEzCnvC1Cjc3DNd1DTjkMcmihih8SSxmm4LBqRq5Y9', // @fabien public posting WIF
'posting',
1,
Copy link
Contributor

Choose a reason for hiding this comment

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

i would love for these arguments to be named instead of positional

@bonustrack
Copy link
Contributor Author

@sneak i've changed the arguments to be named instead of positional and now if weight is not specified it take the weight_thresold value as default weight.

@sneak
Copy link
Contributor

sneak commented Aug 2, 2017

awesome. in general i prefer named arguments vs positional for anything that takes more than 1-2 params. using names like signingKey instead of activeWif shows which key is which, too.

it makes it possible to audit calling code for correctness without knowing the api being called into.

@bonustrack
Copy link
Contributor Author

Yes it make sense, on the rest of the library both solutions (named and positional) are possible example:

steem.broadcast.transferWith(key, { from, to, amount, memo }, cb) // named
steem.broadcast.transfer(key, from, to, amount, memo, cb) // positional

I've renamed activeWif to signingKey also.

@gregory-latinier gregory-latinier self-requested a review August 3, 2017 15:05
Copy link

@gregory-latinier gregory-latinier left a comment

Choose a reason for hiding this comment

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

Adding and removing a key work

@bonustrack bonustrack merged commit 03af751 into master Aug 3, 2017
@bonustrack bonustrack deleted the add-key-auth branch August 3, 2017 16:04
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.

6 participants