pokt-network / pocket

Official implementation of the Pocket Network Protocol v1
https://pokt.network
MIT License
63 stars 33 forks source link

[BUG] Int casting issue when sending tx #783

Closed Olshansk closed 1 year ago

Olshansk commented 1 year ago

Description

Summary generated by Reviewpad on 23 May 23 21:42 UTC

This pull request includes a cleanup and bug fix in module.go that utilizes the shared codec module when marshaling data sent over the wire, and it introduces a hacky workaround to cast a uint32 to a string since it is being passed in as a json.Number through some codeflow. Additionally, the changelog has been updated to reflect these changes.

Origin Document

The error encountered is:

node1.consensus  | 9:11PM level=INFO broadcasting message to network PocketEnvelope={"content":{"type_url":"type.googleapis.com/utility.TxGossipMessage","value":"CtsBCl0KJ3R5cGUuZ29vZ2xlYXBpcy5jb20vdXRpbGl0eS5NZXNzYWdlU2VuZBIyChSZcEXn+ZM7GobRUrdy0giO+GK4dxIUmYBAWIreXuPWzSzOdGeuEMvjmJgaBDEwMDASFDExOTk1NjAzMDY1Mzk4NTAxNTM1GmQKIHuXVS389g22ddtxtHRZjRmsJu41Qe9N1GcGWwPkDEN8EkBMzGmDb7CQn1nbCnl6w9B1/mbzM8PVPnjRDGJMZFxoc8rx+h6O8U1PBE6bdT5srrWBXkhp78g0wjmnMZRGH8MA"}} module=p2p
node1.consensus  | echo: http: panic serving 172.20.0.1:49906: interface conversion: interface {} is json.Number, not string
node1.consensus  | goroutine 484 [running]:
node1.consensus  | net/http.(*conn).serve.func1()
node1.consensus  |  /usr/local/go/src/net/http/server.go:1850 +0xb8
node1.consensus  | panic({0xe17d80, 0x4005759dd0})
node1.consensus  |  /usr/local/go/src/runtime/panic.go:890 +0x260
node1.consensus  | net/http.(*timeoutHandler).ServeHTTP(0x4005736fc0, {0x1463e40, 0x40036cd4a0}, 0x4002f05900)
node1.consensus  |  /usr/local/go/src/net/http/server.go:3412 +0x6f8
node1.consensus  | github.com/labstack/echo/v4/middleware.TimeoutConfig.ToMiddleware.func1.1({0x1480bf8, 0x4003062aa0})
node1.consensus  |  /go/src/github.com/pocket-network/vendor/github.com/labstack/echo/v4/middleware/timeout.go:125 +0x218
node1.consensus  | github.com/labstack/echo/v4/middleware.RequestLoggerConfig.ToMiddleware.func1.1({0x1480bf8, 0x4003062aa0})
node1.consensus  |  /go/src/github.com/pocket-network/vendor/github.com/labstack/echo/v4/middleware/request_logger.go:219 +0xe8
node1.consensus  | github.com/labstack/echo/v4.(*Echo).ServeHTTP(0x4000128240, {0x1464d10?, 0x40000e2620}, 0x4002f05900)
node1.consensus  |  /go/src/github.com/pocket-network/vendor/github.com/labstack/echo/v4/echo.go:646 +0x430
node1.consensus  | net/http.serverHandler.ServeHTTP({0x1460cf8?}, {0x1464d10, 0x40000e2620}, 0x4002f05900)
node1.consensus  |  /usr/local/go/src/net/http/server.go:2947 +0x2cc
node1.consensus  | net/http.(*conn).serve(0x40058f80a0, {0x1466138, 0x4002f2d9b0})
node1.consensus  |  /usr/local/go/src/net/http/server.go:1991 +0x544
node1.consensus  | created by net/http.(*Server).Serve
node1.consensus  |  /usr/local/go/src/net/http/server.go:3102 +0x43c

The following video captures

https://github.com/pokt-network/pocket/assets/1892194/0b90a0f5-74ba-4f59-8042-4a9b9341c56e

The correct fix is to identify where/how a json.Number is propagated to this field, but as a temporary hacky workaround, it was cast to a string. See this StackOverflow answer for more details:

Screenshot 2023-05-23 at 2 36 45 PM

Issue

NA

Type of change

Please mark the relevant option(s):

List of changes

Testing

Required Checklist

If Applicable Checklist

Olshansk commented 1 year ago

@bryanchriswhite I'm going to merge this in just to unblock things but if you can look at // HACK(#783): Figure out why we need a conversion here (in the code) at some point in the future, would appreciate it!