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

invalid transaction hashes crashes btcwallet #405

Merged
merged 1 commit into from Apr 6, 2016

Conversation

gmelika
Copy link
Contributor

@gmelika gmelika commented Apr 4, 2016

this is for issue #404

@gmelika gmelika changed the title invalid invalid transaction hashes crashes btcwallet invalid transaction hashes crashes btcwallet Apr 4, 2016
@jrick
Copy link
Member

jrick commented Apr 4, 2016

Thanks for spotting this. You're right that TxDetails will return a nil transaction and a nil error when the transaction is not found.

Since the error case is handled by returning "not found" to the caller, could you first check that the error is not nil, and encode the error to a string to report the error, and then check if txDetails is nil after the error check?

@gmelika
Copy link
Contributor Author

gmelika commented Apr 4, 2016

separated the error messages based on your feedback. I basically copied the error text from another place in the code that did that same check

@jrick
Copy link
Member

jrick commented Apr 6, 2016

Minor style nitpick: can you split the string over several lines (concatenating them with +) so this fits in under 80 columns?

FWIW this is not my preferred style and I admit to breaking it in some places for practical reasons (like the rpcserver package) but here there's no real reason not to keep it similar to the surrounding code.

After that is changed, this gets my OK.

@gmelika
Copy link
Contributor Author

gmelika commented Apr 6, 2016

no problem. I fixed it, and squashed the commits

@jrick
Copy link
Member

jrick commented Apr 6, 2016

Looks like you need to run go fmt on it.

@gmelika
Copy link
Contributor Author

gmelika commented Apr 6, 2016

meh, this looks simple enough to commit as is. what could possibly go wrong, anyway

  • Anonymous developer 😄

@jrick jrick merged commit eefc610 into btcsuite:master Apr 6, 2016
jrick added a commit to jrick/btcwallet that referenced this pull request Dec 5, 2016
While here, go ahead and update all of the decred deps, and go-spew.

This is required to pull in a change to dcrjson.  Refs btcsuite#405.
jrick added a commit to jrick/btcwallet that referenced this pull request Dec 5, 2016
While here, go ahead and update all of the decred deps, and go-spew.

This is required to pull in a change to dcrjson.  Refs btcsuite#405.
jrick added a commit to jrick/btcwallet that referenced this pull request Dec 5, 2016
While here, go ahead and update all of the decred deps, and go-spew.

This is required to pull in a change to dcrjson.  Refs btcsuite#405.
jrick added a commit to jrick/btcwallet that referenced this pull request Dec 5, 2016
While here, go ahead and update all of the decred deps, and go-spew.

This is required to pull in a change to dcrjson.  Refs btcsuite#405.
jrick added a commit to jrick/btcwallet that referenced this pull request Dec 5, 2016
While here, go ahead and update all of the decred deps, and go-spew.

This is required to pull in a change to dcrjson.  Refs btcsuite#405.
jrick added a commit to jrick/btcwallet that referenced this pull request Dec 5, 2016
While here, go ahead and update all of the decred deps, and go-spew.

This is required to pull in a change to dcrjson.  Refs btcsuite#405.
jrick added a commit to jrick/btcwallet that referenced this pull request Dec 5, 2016
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.

None yet

2 participants