PROTO-3189 Removed ignite, GRPC/RPC, data model restructure, refactor and cleanup / PROTO-3037 #108
Conversation
xmariachi
changed the title
xmariachi
requested review from
guilherme-brandao,
kpeluso,
felipead and
spooktheducks
|
It would be nice to see some automated unit or integration tests, or at the very least document how you tested things manually, including testing scenarios, etc... These are very profound changes... |
xmariachi
changed the title
xmariachi
changed the title
Update descrption with more details about the testing done. |
Signed-off-by: Kenny <[email protected]>
| // TODO: Investigate and ecide if we want to keep this | ||
| // spin up goroutine for monitoring and reconnect purposes - TODO test and configure | ||
| // go func() { | ||
| // for { | ||
| // state := chain.Client.GRPCClient.GetState() | ||
| // if state == connectivity.TransientFailure || state == connectivity.Shutdown { | ||
| // fmt.Println("GRPC Connection lost, attempting to reconnect...") | ||
| // for { | ||
| // if chain.Client.GRPCClient.WaitForStateChange(context.Background(), state) { | ||
| // break | ||
| // } | ||
| // time.Sleep(10 * time.Second) | ||
| // } | ||
| // } | ||
| // time.Sleep(10 * time.Second) | ||
| // } | ||
| // }() | ||
| return grpcConnection, nil | ||
| } | ||
|
|
||
| // An interceptor that logs the gRPC request, for debugging purposes | ||
| // func loggerHeaderInterceptor() grpc.UnaryClientInterceptor { | ||
| // return func( | ||
| // ctx context.Context, | ||
| // method string, | ||
| // req, reply interface{}, | ||
| // cc *grpc.ClientConn, | ||
| // invoker grpc.UnaryInvoker, | ||
| // opts ...grpc.CallOption, | ||
| // ) error { | ||
| // log.Info().Str("method", method).Msg("Intercepting gRPC request") | ||
| // return invoker(ctx, method, req, reply, cc, opts...) | ||
| // } | ||
| // } |
There was a problem hiding this comment.
what are pros/cons for including?
There was a problem hiding this comment.
I left code in for ease of use on debugging. But there is no need to set up a headerInterceptor on prod code if nothing is done with it. It could just be deleted though, since it is already in the codebase history if needed :D
Re. the commented goroutine for the grpc conn health, it is something I've seen in other places. Works with and without it for what I've seen, so I'm reluctant to add it without fully getting the pro/cons of it.
There was a problem hiding this comment.
@felipead @spooktheducks @fernandofcampos you may have opinions here
lib/repo_query_account.go
Outdated
There was a problem hiding this comment.
probs should also be tied to the wallet. perhaps there should be a distinct wallet package in our golang allora sdk that groups all the functionality i'm calling out. then this repo and others can utilize it. if you agree, please add these notes to the go-sdk project
There was a problem hiding this comment.
yeah, I called it auth and the Wallet and WalletConfig may be in. But yeah, its name may also be wallet if so, for clarity, since it's the major guest there.
xmariachi
changed the title
|
Pushed new changes:
|
| @@ -160,8 +78,30 @@ func (node *NodeConfig) SendDataWithRetry(ctx context.Context, req sdktypes.Msg, | |||
| case ErrorProcessingFees: | |||
| // Error has not been handled, just mark as recalculate fees on this iteration | |||
| log.Info().Msg("Insufficient fees, marking fee recalculation on tx broadcasting for retrial") | |||
| recalculateFees += 1 | |||
| // TODO Handle fee and "out of gas" error differently | |||
Purpose of Changes and their Description
Walletto keep all data and functions re. wallet.TESTING
Testing has been launching it manually via multi-topic, with 1 woker and reputer per topic, configuration. Using testnet endpoints from Foundation, L5 and Polkachu.
The only errors that have been observed are the expected
account sequence mismatchwhich are working fine on retry.One time it has been observed that
No other errors observed during several hours, which adds to the previous #98 testing (which provides the connection multi-connection backbone) which was run over a weekend with the same results, so confidence is high.
Also of course existent unit tests and mocks have been modified accordingly.
Are these changes tested and documented?
Unreleasedsection ofCHANGELOG.md?