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

fix: runtime panic #2836

Merged
merged 5 commits into from
Mar 7, 2022
Merged

fix: runtime panic #2836

merged 5 commits into from
Mar 7, 2022

Conversation

notanatol
Copy link
Contributor

@notanatol notanatol commented Mar 4, 2022

Checklist

Description

Fixes the /chequebook runtime panic.


This change is Reviewable

@@ -125,6 +126,9 @@ func (s *Service) Configure(overlay swarm.Address, p2p p2p.DebugService, pingpon
s.chequebook = chequebook
s.swapEnabled = swapEnabled
s.swap = swap
if !chainEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

This extra parameter can be avoided by either initialising swap to noOpSwap before passing here or maybe checking if the swap == nil.

@@ -145,3 +149,54 @@ func (s *Service) ServeHTTP(w http.ResponseWriter, r *http.Request) {

h.ServeHTTP(w, r)
}

type noOpSwap struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

So I went through the APIs to see how they are handled. Seems like there are a few errors like ErrNoChequebook which are being handled. I wonder if we should retain the errors here instead of returning empty responses? If API expects to send No Chequebook error if there is no chequebook, then this would sort of break that no?

@aloknerurkar aloknerurkar added this to the 1.5.0 milestone Mar 7, 2022
@notanatol notanatol merged commit 95af6f5 into master Mar 7, 2022
@notanatol notanatol deleted the fix-chequebook branch March 7, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants