BE-594 | Delivery package #525
Conversation
Implements various gRPC, http related helper functions
WalkthroughThe changes introduce three new files: Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (1)
delivery/http/http.go (1)
13-17: LGTM: DefaultClient is well-implemented with a suggestion.The DefaultClient is correctly implemented with a reasonable timeout and OpenTelemetry instrumentation. This is a good practice for creating a reusable HTTP client with built-in observability.
Consider making the timeout configurable, perhaps through an environment variable or configuration file. This would allow for easier adjustments in different environments without code changes.
Example implementation:
var defaultTimeout = 5 * time.Second func init() { if envTimeout := os.Getenv("HTTP_CLIENT_TIMEOUT"); envTimeout != "" { if d, err := time.ParseDuration(envTimeout); err == nil { defaultTimeout = d } } } var DefaultClient = &http.Client{ Timeout: defaultTimeout, Transport: otelhttp.NewTransport(http.DefaultTransport), }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (2)
delivery/grpc/grpc_test.gois excluded by!**/*_test.godelivery/http/get_test.gois excluded by!**/*_test.go
📒 Files selected for processing (3)
- delivery/grpc/grpc.go (1 hunks)
- delivery/http/get.go (1 hunks)
- delivery/http/http.go (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
delivery/http/get.go (1)
1-11: LGTM: Function signature and imports are well-defined.The
Getfunction signature is clear and follows Go conventions. The use ofcontext.Contextallows for proper cancellation and timeout management. Returning[]byteprovides flexibility for the caller to handle the response body as needed.delivery/http/http.go (2)
4-10: LGTM: Import statements are correctly updated.The new imports are necessary for the implementation of the DefaultClient and its OpenTelemetry instrumentation.
Line range hint
20-45: LGTM: Existing functionality is preserved.The introduction of the DefaultClient does not affect the existing interfaces and methods in the file. This approach allows for gradual adoption of the new HTTP client in the codebase without introducing breaking changes.
delivery/grpc/grpc.go (1)
36-36: Ensure the codec name matches the expected encoding.The
Name()method ofOsmomathCodecreturns"gogoproto". Verify that this name aligns with the expected codec name in the gRPC framework. Mismatches may lead to the codec not being used as intended.Run the following script to check registered codecs:
Ensure that the application recognizes
"gogoproto"as the codec name.
|
| func NewClient(grpcEndpoint string) (*Client, error) { | ||
| customCodec := &OsmomathCodec{parentCodec: encoding.GetCodec("proto")} | ||
|
|
||
| grpcOpts := []grpc.DialOption{ | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithStatsHandler(otelgrpc.NewClientHandler()), | ||
| grpc.WithDefaultCallOptions(grpc.ForceCodec(customCodec)), |
There was a problem hiding this comment.
I'm confused - why do we need this? Cosmos SDK should have all of the GRPC-related packages and libraries with the appropriate codec registered
There was a problem hiding this comment.
Why we need to do this here when in the rest of the places we were able to avoid this custom codec logic:
sqs/domain/passthrough/passthrough_grpc_client.go
Lines 72 to 78 in ad494d3
There was a problem hiding this comment.
That's a great question! Custom codec is required to fix a panic that would occur unmarshaling to osmomath.LegacyDec data types otherwise. I have also documented it in the source code here.
For example:
panic: invalid Go type math.LegacyDec for field osmosis.txfees.v1beta1.QueryEipBaseFeeResponse.base_fee
goroutine 4056 [running]:
google.golang.org/protobuf/internal/impl.newSingularConverter({0x460fc10, 0x32bfd60}, {0x46114b0, 0xc0108f5400})
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/internal/impl/convert.go:142 +0x9ad
google.golang.org/protobuf/internal/impl.NewConverter({0x460fc10, 0x32bfd60}, {0x46114b0, 0xc0108f5400})
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/internal/impl/convert.go:60 +0x8f
google.golang.org/protobuf/internal/impl.fieldInfoForScalar({0x46114b0, 0xc0108f5400}, {{0x2ec85e8, 0x7}, {0x0, 0x0}, {0x460fc10, 0x32bfd60}, {0x2ec85f0, 0x7f}, ...}, ...)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/internal/impl/message_reflect_field.go:270 +0x193
google.golang.org/protobuf/internal/impl.(*MessageInfo).makeKnownFieldsFunc(0xc0108fa9a0, {0xffffffffffffffff, {0x0, 0x0}, 0xffffffffffffffff, {0x0, 0x0}, 0xffffffffffffffff, {0x0, 0x0}, ...})
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/internal/impl/message_reflect.go:80 +0x78a
google.golang.org/protobuf/internal/impl.(*MessageInfo).makeReflectFuncs(0xc0108fa9a0, {0x460fc10, 0x2e28b20}, {0xffffffffffffffff, {0x0, 0x0}, 0xffffffffffffffff, {0x0, 0x0}, 0xffffffffffffffff, ...})
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/internal/impl/message_reflect.go:42 +0x58
google.golang.org/protobuf/internal/impl.(*MessageInfo).initOnce(0xc0108fa9a0)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/internal/impl/message.go:90 +0x1b0
google.golang.org/protobuf/internal/impl.(*MessageInfo).init(...)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/internal/impl/message.go:72
google.golang.org/protobuf/internal/impl.(*messageReflectWrapper).ProtoMethods(0x458b1e0?)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/internal/impl/message_reflect_gen.go:162 +0x25
google.golang.org/protobuf/proto.protoMethods(...)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/proto/proto_methods.go:19
google.golang.org/protobuf/proto.UnmarshalOptions.unmarshal({{}, 0x1, 0x1, 0x0, {0x4595590, 0xc00028a300}, 0x2710}, {0xc0108c2ab0, 0x12, 0x12}, ...)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/proto/decode.go:95 +0xe8
google.golang.org/protobuf/proto.Unmarshal({0xc0108c2ab0, 0x12, 0x12}, {0x458b1e0?, 0xc0108f2ad0?})
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/proto/decode.go:57 +0x5d
google.golang.org/grpc/encoding/proto.codec.Unmarshal({}, {0xc0108c2ab0?, 0x12?, 0x12?}, {0x31246a0?, 0xc0101bd888?})
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/encoding/proto/proto.go:56 +0x65
google.golang.org/grpc.recv(0xc0108f2a80?, {0x7f8d7829f7c0, 0x6551e00}, 0xc0101bd8b0?, {0x0?, 0x0?}, {0x31246a0, 0xc0101bd888}, 0x3124a60?, 0xc00ca5f218, ...)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/rpc_util.go:839 +0xc2
google.golang.org/grpc.(*csAttempt).recvMsg(0xc0108b12b0, {0x31246a0, 0xc0101bd888}, 0x7f8d7829f7c0?)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/stream.go:1086 +0x289
google.golang.org/grpc.(*clientStream).RecvMsg.func1(0x0?)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/stream.go:929 +0x1f
google.golang.org/grpc.(*clientStream).withRetry(0xc010d96000, 0xc00ca5f338, 0xc00ca5f380)
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/stream.go:778 +0x13a
google.golang.org/grpc.(*clientStream).RecvMsg(0xc010d96000, {0x31246a0?, 0xc0101bd888?})
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/stream.go:928 +0x110
google.golang.org/grpc.invoke({0x45c5038?, 0xc001a199e0?}, {0x336b69f?, 0x2?}, {0x3124a60, 0x6551e00}, {0x31246a0, 0xc0101bd888}, 0x7f8d7077ce88?, {0x0, ...})
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/call.go:73 +0xcb
google.golang.org/grpc.(*ClientConn).Invoke(0xc00205ac08, {0x45c5038?, 0xc001a199e0?}, {0x336b69f?, 0xc00ca5f500?}, {0x3124a60?, 0x6551e00?}, {0x31246a0?, 0xc0101bd888?}, {0x0, ...})
/home/deividas/go/pkg/mod/google.golang.org/[email protected]/call.go:37 +0x23f
github.com/osmosis-labs/osmosis/v26/x/txfees/types.(*queryClient).GetEipBaseFee(0xc0108f2570, {0x45c5038, 0xc001a199e0}, 0x6551e00, {0x0, 0x0, 0x0})
/home/deividas/go/pkg/mod/github.com/osmosis-labs/osmosis/[email protected]/x/txfees/types/query.pb.go:585 +0xc8
github.com/osmosis-labs/sqs/domain/cosmos/tx.CalculateFeeCoin({0x45c5038, 0xc001a199e0}, 0x32ea097?, 0x2bf58)
/home/deividas/go/src/github.com/deividaspetraitis/sqs/domain/cosmos/tx/tx.go:195 +0x9a
github.com/osmosis-labs/sqs/domain/cosmos/tx.BuildTx({0x45c5038, 0xc001a199e0}, 0xc001b122c8, {0x459bf28?, 0xc00181e5e8?}, {{0x45dffc0, 0xc001b0dbc0}, {0x4605038, 0xc00009d440}, {0x45dd338, ...}, ...}, ...)
/home/deividas/go/src/github.com/deividaspetraitis/sqs/domain/cosmos/tx/tx.go:66 +0x21c
github.com/osmosis-labs/sqs/ingest/usecase/plugins/orderbook/claimbot.sendBatchClaimTx({0x45c5038, 0xc001a199e0}, {0x459bf28, 0xc00181e5e8}, 0xc001b122c8, {0x4581fc0, 0xc000cd72c0}, {0xc0025dfbc0, 0x3f}, {0xc000bfb080, ...})
/home/deividas/go/src/github.com/deividaspetraitis/sqs/ingest/usecase/plugins/orderbook/claimbot/tx.go:91 +0x2cf
github.com/osmosis-labs/sqs/ingest/usecase/plugins/orderbook/claimbot.(*claimbot).processBatchClaimOrders(0xc001624d80, {0x45c5038, 0xc001a199e0}, {{0xc002c93680, 0x56}, {0xc002c936d7, 0x44}, 0x78a, {0xc0025dfbc0, 0x3f}}, ...)
/home/deividas/go/src/github.com/deividaspetraitis/sqs/ingest/usecase/plugins/orderbook/claimbot/plugin.go:137 +0x245
github.com/osmosis-labs/sqs/ingest/usecase/plugins/orderbook/claimbot.(*claimbot).ProcessEndBlock(0xc001624d80, {0x45c5038?, 0xc000cbf7d0?}, 0x1559520, {0xc000d174a0?, 0xc000cbfb60?, 0xc000cbfb00?})
/home/deividas/go/src/github.com/deividaspetraitis/sqs/ingest/usecase/plugins/orderbook/claimbot/plugin.go:117 +0x4d2
github.com/osmosis-labs/sqs/ingest/usecase.(*ingestUseCase).executeEndBlockProcessPlugins(0xc0012c1420, {0x45c5038, 0xc000cbf7d0}, 0x1559520, {0xc000d174a0?, 0xc000cbfb60?, 0xc000cbfb00?})
/home/deividas/go/src/github.com/deividaspetraitis/sqs/ingest/usecase/ingest_usecase.go:481 +0xed
created by github.com/osmosis-labs/sqs/ingest/usecase.(*ingestUseCase).ProcessBlockData in goroutine 162
/home/deividas/go/src/github.com/deividaspetraitis/sqs/ingest/usecase/ingest_usecase.go:199 +0xe05
exit status 2
There was a problem hiding this comment.
But LegacyDec is just a string, and we have other messages / queries that function without custom codec.
I'm still unsure why this is necessary.
Is it possible to import this client:
https://github.com/osmosis-labs/osmosis/blob/09ff5a2d72306f5b5a4b4dd29996cb5ab37136d7/x/txfees/keeper/grpc_query.go#L25-L28
and then use this method:
https://github.com/osmosis-labs/osmosis/blob/09ff5a2d72306f5b5a4b4dd29996cb5ab37136d7/x/txfees/keeper/grpc_query.go#L89-L92
There was a problem hiding this comment.
I may be missing some key details, but it seems that constructing txfees.NewQuerier would much more involving since it expects Keeper as parameter:
type Keeper struct {
storeKey storetypes.StoreKey
accountKeeper types.AccountKeeper
bankKeeper types.BankKeeper
poolManager types.PoolManager
protorevKeeper types.ProtorevKeeper
distributionKeeper types.DistributionKeeper
consensusKeeper types.ConsensusKeeper
dataDir string
paramSpace paramtypes.Subspace
}
As a reference I was using both node/sqs repos to find examples of how to construct NewQuerier in simple way, but with no results, also I have doubts whether this apporoach would resolve the issue, because it uses same QueryEipBaseFeeResponse as we do here in SQS. This data structure has field BaseFee of a type osmomath.LegacyDec and I am getting this error with following command too:
❯ osmosisd q txfees base-fee --grpc-addr 127.0.0.1:9090 --grpc-insecure
panic: invalid Go type math.LegacyDec for field osmosis.txfees.v1beta1.QueryEipBaseFeeResponse.base_fee
goroutine 1 [running]:
google.golang.org/protobuf/internal/impl.newSingularConverter({0x639d528, 0x5678e00}, {0x639ecb8, 0xc001c6fb00})
google.golang.org/[email protected]/internal/impl/convert.go:142 +0x9ad
google.golang.org/protobuf/internal/impl.NewConverter({0x639d528, 0x5678e00}, {0x639ecb8, 0xc001c6fb00})
google.golang.org/[email protected]/internal/impl/convert.go:60 +0x8f
google.golang.org/protobuf/internal/impl.fieldInfoForScalar({0x639ecb8, 0xc001c6fb00}, {{0x5207b1e, 0x7}, {0x0, 0x0}, {0x639d528, 0x5678e00}, {0x5207b26, 0x7f}, ...}, ...)
google.golang.org/[email protected]/internal/impl/message_reflect_field.go:270 +0x193
google.golang.org/protobuf/internal/impl.(*MessageInfo).makeKnownFieldsFunc(0xc0028f2000, {0xffffffffffffffff, {0x0, 0x0}, 0xffffffffffffffff, {0x0, 0x0}, 0xffffffffffffffff, {0x0, 0x0}, ...})
google.golang.org/[email protected]/internal/impl/message_reflect.go:80 +0x78a
google.golang.org/protobuf/internal/impl.(*MessageInfo).makeReflectFuncs(0xc0028f2000, {0x639d528, 0x51447c0}, {0xffffffffffffffff, {0x0, 0x0}, 0xffffffffffffffff, {0x0, 0x0}, 0xffffffffffffffff, ...})
google.golang.org/[email protected]/internal/impl/message_reflect.go:42 +0x58
google.golang.org/protobuf/internal/impl.(*MessageInfo).initOnce(0xc0028f2000)
google.golang.org/[email protected]/internal/impl/message.go:90 +0x1b0
google.golang.org/protobuf/internal/impl.(*MessageInfo).init(...)
google.golang.org/[email protected]/internal/impl/message.go:72
google.golang.org/protobuf/internal/impl.(*messageReflectWrapper).ProtoMethods(0x630a4a0?)
google.golang.org/[email protected]/internal/impl/message_reflect_gen.go:162 +0x25
google.golang.org/protobuf/proto.protoMethods(...)
google.golang.org/[email protected]/proto/proto_methods.go:19
google.golang.org/protobuf/proto.UnmarshalOptions.unmarshal({{}, 0x1, 0x1, 0x0, {0x6315d20, 0xc0002b0120}, 0x2710}, {0xc00288a798, 0x12, 0x12}, ...)
google.golang.org/[email protected]/proto/decode.go:95 +0xe8
google.golang.org/protobuf/proto.Unmarshal({0xc00288a798, 0x12, 0x12}, {0x630a4a0?, 0xc0028bb4c0?})
google.golang.org/[email protected]/proto/decode.go:57 +0x5d
google.golang.org/grpc/encoding/proto.codec.Unmarshal({}, {0xc00288a798?, 0x12?, 0x12?}, {0x54b12c0?, 0xc002e82dc0?})
google.golang.org/[email protected]/encoding/proto/proto.go:56 +0x65
google.golang.org/grpc.recv(0xc00300e3c0?, {0x7fec4c175698, 0x88b2560}, 0xc00118a918?, {0x0?, 0x0?}, {0x54b12c0, 0xc002e82dc0}, 0x54b1680?, 0x0, ...)
google.golang.org/[email protected]/rpc_util.go:839 +0xc2
google.golang.org/grpc.(*csAttempt).recvMsg(0xc002f9fa00, {0x54b12c0, 0xc002e82dc0}, 0x7fec4c175698?)
google.golang.org/[email protected]/stream.go:1086 +0x289
google.golang.org/grpc.(*clientStream).RecvMsg.func1(0x0?)
google.golang.org/[email protected]/stream.go:929 +0x1f
google.golang.org/grpc.(*clientStream).withRetry(0xc00309d200, 0xc000e56108, 0xc000e56150)
google.golang.org/[email protected]/stream.go:778 +0x13a
google.golang.org/grpc.(*clientStream).RecvMsg(0xc00309d200, {0x54b12c0?, 0xc002e82dc0?})
google.golang.org/[email protected]/stream.go:928 +0x110
google.golang.org/grpc.invoke({0x6348f48?, 0xc002fc0360?}, {0x5747b24?, 0x4?}, {0x54b1680, 0x88b2560}, {0x54b12c0, 0xc002e82dc0}, 0x18?, {0x88b2560, ...})
google.golang.org/[email protected]/call.go:73 +0xcb
google.golang.org/grpc.(*ClientConn).Invoke(0xc0029ccc08, {0x6348f48?, 0xc002fc0360?}, {0x5747b24?, 0x15?}, {0x54b1680?, 0x88b2560?}, {0x54b12c0?, 0xc002e82dc0?}, {0x88b2560, ...})
google.golang.org/[email protected]/call.go:37 +0x23f
github.com/cosmos/cosmos-sdk/client.Context.Invoke({{0x0, 0x0, 0x0}, {0x6374338, 0xc002fc0030}, 0xc0029ccc08, {0x0, 0x0}, {0x6392e10, 0xc001318fb0}, ...}, ...)
github.com/cosmos/[email protected]/client/grpc_query.go:61 +0x785
github.com/osmosis-labs/osmosis/v26/x/txfees/types.(*queryClient).GetEipBaseFee(0xc0030b6030, {0x6348f48, 0xc002fc0360}, 0x88b2560, {0x88b2560, 0x0, 0x0})
github.com/osmosis-labs/osmosis/v26/x/txfees/types/query.pb.go:585 +0xc8
reflect.Value.call({0x5271780?, 0xc0030b6030?, 0xc0030b6030?}, {0x56a5414, 0x4}, {0xc000e57660, 0x2, 0x639d528?})
reflect/value.go:596 +0xca6
reflect.Value.Call({0x5271780?, 0xc0030b6030?, 0x0?}, {0xc000e57660?, 0x0?, 0x0?})
reflect/value.go:380 +0xb9
github.com/osmosis-labs/osmosis/osmoutils/osmocli.callQueryClientFn({0x6348f48, 0xc002fc0360}, {0x56b6fd0, 0xd}, {0x6320500, 0x88b2560}, {0x5271780?, 0xc0030b6030?})
github.com/osmosis-labs/osmosis/[email protected]/osmocli/query_cmd_wrap.go:158 +0x274
github.com/osmosis-labs/osmosis/osmoutils/osmocli.BuildQueryCli[...].func3({0xc002f7cc30, 0x0, 0x3})
github.com/osmosis-labs/osmosis/[email protected]/osmocli/query_cmd_wrap.go:187 +0x1ae
github.com/spf13/cobra.(*Command).execute(0xc002c76608, {0xc002f7cbd0, 0x3, 0x3})
github.com/spf13/[email protected]/command.go:985 +0xaca
github.com/spf13/cobra.(*Command).ExecuteC(0xc0027e6308)
github.com/spf13/[email protected]/command.go:1117 +0x3ff
github.com/spf13/cobra.(*Command).Execute(...)
github.com/spf13/[email protected]/command.go:1041
github.com/spf13/cobra.(*Command).ExecuteContext(...)
github.com/spf13/[email protected]/command.go:1034
github.com/cosmos/cosmos-sdk/server/cmd.Execute(0xc0027e6308, {0x56abd2e, 0x8}, {0xc001107b18, 0x18})
github.com/cosmos/[email protected]/server/cmd/execute.go:34 +0x187
main.main()
github.com/osmosis-labs/osmosis/v26/cmd/osmosisd/main.go:16 +0x37
This issue is documented on cosmos-sdk too: cosmos/cosmos-sdk#18430 (comment)
This PR was opened as part of bigger claimbot feature to keep PRs small, focused and more manageable and implements various gRPC, http related helper functions used in claimbot feature: PR-524.
osmomath.Dec, for example.httppackage implements a few helper funcitons used implementing HTTP clients, for example.Summary by CodeRabbit
New Features
Bug Fixes