Skip to content
This repository has been archived by the owner on Sep 12, 2019. It is now read-only.

Delete wallet #6

Merged
merged 3 commits into from
Nov 26, 2014
Merged

Delete wallet #6

merged 3 commits into from
Nov 26, 2014

Conversation

deckar01
Copy link
Contributor

Update delete_wallet.js.
Expose delete on wallet prototype.
Gave up on adding a test and added a stub description.

I will need to bump the version and cut a release so we can pull the new feature into the client.

Fixes #5.

@deckar01 deckar01 added the pr label Nov 25, 2014
@bartekn
Copy link
Contributor

bartekn commented Nov 25, 2014

The only problem I can see is that after running wallet.delete users will still be able to send requests using other methods (that will fail because stellar-wallet will return not_found). One of possible solutions is to throw WalletNotFound in all methods when server returns not_found. The other solution would be to add deleted flag to Wallet object and throw WalletNotFound before even sending request to stellar-wallet server.

@deckar01
Copy link
Contributor Author

@bartekn I don't see the utility in adding a bunch of code to explicitly keep a deleted state and return errors. The wallet server already rejects the request for us if someone tries to keep using the wallet after deleting it.

@bartekn
Copy link
Contributor

bartekn commented Nov 25, 2014

The wallet server already rejects the request for us if someone tries to keep using the wallet after deleting it.

OK, so still we need to add code that throws WalletNotFound/Forbidden in some methods. I wasn't aware of delete functionality when I was developing them.

@deckar01
Copy link
Contributor Author

Instead of hard coding the errors we expect into every protocol request, we should pass all errors to a middleware error handler. This will allow us to catch errors properly even if we don't expect them ahead of time.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.16%) when pulling 366cc9a on delete-wallet into a146d75 on master.

@deckar01: This is likely due to errors.getProtocolError() not having any tests.

@deckar01
Copy link
Contributor Author

I added errors.getProtocolError() which matches the response code of "fail"ed requests to the proper error objects.

@bartekn
Copy link
Contributor

bartekn commented Nov 26, 2014

👍 We can add some tests later. I've created GH issue for this: #9.

bartekn added a commit that referenced this pull request Nov 26, 2014
@bartekn bartekn merged commit e427ac4 into master Nov 26, 2014
@jedmccaleb jedmccaleb removed the pr label Nov 26, 2014
@bartekn bartekn deleted the delete-wallet branch November 28, 2014 13:19
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.

Implement delete wallet
4 participants