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

Using decode_account instead of decode_hex in account_move RPC handler #1306

Merged
merged 1 commit into from Oct 19, 2018

Conversation

Projects
None yet
5 participants
@elliottminns
Copy link
Contributor

commented Oct 15, 2018

This patch resolves an issue with the RPC handler for account_move incorrectly decoding accounts by hex. According to https://github.com/nanocurrency/raiblocks/wiki/RPC-protocol#account-move this method should accept addresses rather than public keys and therefore needed decode_account instead.

Closes #666

@PlasmaPower

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

This is a breaking change, though most likely nobody is using this RPC.

@PlasmaPower

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2018

We could do error = account.decode_account (str) && account.decode_hex (str); (we also should have error handling here).

@clemahieu

This comment has been minimized.

Copy link
Collaborator

commented Oct 15, 2018

I’m fine with breaking this, accounts should never be moved around by hex because it discards the checksum.

The test RPC.account_move needs to be fixed.

@elliottminns elliottminns force-pushed the elliottminns:fix/account-move branch from 08c2a87 to e244b00 Oct 15, 2018

@elliottminns

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

Test RPC.account_move updated to match the new behaviour.

@rkeene rkeene added the bug label Oct 16, 2018

@rkeene rkeene requested a review from argakiig Oct 16, 2018

@rkeene rkeene added this to the V17.0 milestone Oct 16, 2018

@rkeene rkeene merged commit bf4cce6 into nanocurrency:master Oct 19, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.