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

Partially fix Issue #151: CLI account caching #640

Merged
merged 1 commit into from
Feb 7, 2018

Conversation

aautushka
Copy link

We have extensive experience with cli_wallet and we find that the cached account_object could easily go stale thus ruining any further transactions, meaning the new transactions issued with cli_wallet would contain out-of-date information, this way you can loose your data (votes, keys, etc) and your money (you'll need to issue more transactions to restore the previous valid state).

When working simultaneously with cli_wallet and the web UI the issue only exacerbates: now you just can't switch from one tool to another, you're basically bound to the UI. The only safe workaround is to kill wallet.json and re-import the keys and do this every time you want to use cli_wallet.

We think there is no better solution other than removing the caching altogether. Granted, this would incur some extra RPC requests but it's a small pay for the predactability and stability of the essential tool.

Copy link
Member

@abitmore abitmore left a comment

Choose a reason for hiding this comment

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

Looks good.
BTW the issue was described in #151 .
Mention the issue number in a comment links the issue with the PR (clickable on github), but not if it's only in the title.
//Update: my fault. Mentioning in a review doesn't link either.

@abitmore
Copy link
Member

abitmore commented Feb 5, 2018

Please be aware that list_my_accounts still returns cached data.

@abitmore
Copy link
Member

abitmore commented Feb 5, 2018

#151

@aautushka
Copy link
Author

@abitmore Thanks. I gather we should've fixed list_my_accounts as well, but it needs some more coding, besides the function is not that useful in our experience.

@abitmore
Copy link
Member

abitmore commented Feb 6, 2018

Thanks. Added notes to OP of #151. Feel free to ask if have any question.

@oxarbitrage oxarbitrage self-assigned this Feb 6, 2018
@oxarbitrage oxarbitrage self-requested a review February 6, 2018 14:22
@oxarbitrage oxarbitrage removed their assignment Feb 6, 2018
Copy link
Member

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

i fully agree with this change. the cache don't work properly and it is better to have it removed from everywhere for more node calls. this is def a good start. good job, i am merging your code now, it builds ok and works as expected.

@oxarbitrage oxarbitrage merged commit 49e8869 into bitshares:develop Feb 7, 2018
@abitmore abitmore added this to the Next Non-Consensus-Changing Release milestone Feb 9, 2018
@abitmore abitmore added the cli label Feb 9, 2018
@abitmore abitmore changed the title Issue #151 Partially fix Issue #151: CLI account caching Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants