feat: servicer module implementation #19
Conversation
Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
Co-authored-by: Redouane Lakrache <[email protected]>
bryanchriswhite
force-pushed
the
servicer-module
branch
from
58d1f2f
to
40e307b
Compare
| @@ -0,0 +1,22 @@ | |||
| syntax = "proto3"; | |||
There was a problem hiding this comment.
Remove this and import from the session module
| @@ -0,0 +1,6 @@ | |||
| package types | |||
There was a problem hiding this comment.
Try import "github.com/cometbft/cometbft/rpc/core" instead and see if that works?
There was a problem hiding this comment.
I don't see a block type in that package but I do see one in github.com/cometbft/cometbft/types. 👍
| return AppModule{ | ||
| AppModuleBasic: NewAppModuleBasic(cdc), | ||
| keeper: keeper, | ||
| accountKeeper: accountKeeper, | ||
| bankKeeper: bankKeeper, | ||
|
|
||
| relayer: relayer, |
There was a problem hiding this comment.
I spent a little while talking to ChatGPT about keepers vs modules and this was the ultimate conclusion:
Conclusion:
In summary, add to the keeper when dealing with business logic and state management and add to the module when dealing with user interfaces, message handling, and blockchain lifecycle events. It promotes a clean separation of concerns, where the module manages interactions and the keeper manages state.
I'm still working through this code but consider if this should be part of the keeper (i.e. for state management and state transitions) instead.
There was a problem hiding this comment.
I looked at one of the most complex modules in the cosmos-sdk (staking) and even they don't update the module but only the keeper:
https://github.com/cosmos/cosmos-sdk/blob/main/x/staking/module.go#L100
https://github.com/cosmos/cosmos-sdk/blob/main/x/staking/keeper/keeper.go#L29
| } | ||
|
|
||
| func NewAppModule( | ||
| cdc codec.Codec, | ||
| keeper keeper.Keeper, | ||
| accountKeeper types.AccountKeeper, | ||
| bankKeeper types.BankKeeper, | ||
| // TODO_THIS_COMMIT: use an enum or something | ||
| clientCtx cosmosClient.Context, | ||
| factory tx.Factory, |
There was a problem hiding this comment.
If we already have the clientCtx, can we just initialize txf via txf, err := tx.NewFactoryCLI(clientCtx, flags())?
| @@ -146,3 +154,10 @@ func (am AppModule) BeginBlock(_ sdk.Context, _ abci.RequestBeginBlock) {} | |||
| func (am AppModule) EndBlock(_ sdk.Context, _ abci.RequestEndBlock) []abci.ValidatorUpdate { | |||
| return []abci.ValidatorUpdate{} | |||
| } | |||
|
|
|||
| // AddModuleInitFlags implements servertypes.ModuleInitFlags interface. | |||
| func AddModuleInitFlags(startCmd *cobra.Command) { | |||
There was a problem hiding this comment.
Is x/poktroll still a "special" module even after the changes?
Is there a way to move all this logic out into app.go?
| } | ||
|
|
||
| if servicerEnabled || applicationEnabled || portalEnabled { | ||
| modules = append(modules, actorModule) |
There was a problem hiding this comment.
In the future, I was thinking each service can act as multiple actors. I think it would also simplify the logic by initializing all of them (none of the if else statements) and it would just be a noop inside the actual module if the flagis off.
| serverCtx := server.GetServerContextFromCmd(cmd) | ||
| // TODO_THIS_COMMIT: factor out keys to constants. | ||
| serverCtx.Viper.Set("actorMode", "servicer") | ||
| serverCtx.Viper.Set("clientCtx", initClientCtx) |
There was a problem hiding this comment.
I see what you're doing here and seems like a good first approach (assuming we clean up the code), but I do want to try to avoid using viper for it.
An idea (I don' know if it works) is to potentially leverage app.go:
func (app *App) RegisterAPIRoutes(apiSvr *api.Server, apiConfig config.APIConfig) {
clientCtx := apiSvr.ClientCtx
// Register new tx routes from grpc-gateway.
authtx.RegisterGRPCGatewayRoutes(clientCtx, apiSvr.GRPCGatewayRouter)
// Register new tendermint queries routes from grpc-gateway.
tmservice.RegisterGRPCGatewayRoutes(clientCtx, apiSvr.GRPCGatewayRouter)
// Register node gRPC service for grpc-gateway.
nodeservice.RegisterGRPCGatewayRoutes(clientCtx, apiSvr.GRPCGatewayRouter)
// Register grpc-gateway routes for all modules.
ModuleBasics.RegisterGRPCGatewayRoutes(clientCtx, apiSvr.GRPCGatewayRouter)
// register app's OpenAPI routes.
docs.RegisterOpenAPIService(Name, apiSvr.Router)
}What if we add a customservicerService.RegisterRelayRoutes(clientCtx, cmdContext), and have it idiometically manage things under the hood?
This isn't a very well formed idea yet, so I'm still thinking through things...
| relays utils.Observable[*types.Relay] | ||
| sessions utils.Observable[*types.Session] | ||
| cometClient cosmosClient.TendermintRPC | ||
| txConfig cosmosClient.TxConfig |
There was a problem hiding this comment.
I really believe all of these should be part of the keeper (if we do keep them) and the minder is just an instance there as well.
| return err | ||
| } | ||
|
|
||
| txBcastResult, err := m.cometClient.BroadcastTxCommit(context.TODO(), txBz) |
There was a problem hiding this comment.
This does seem to be the right approach here: https://docs.cosmos.network/v0.50/core/transactions#cometbft-rpc
| // Sign tx!! | ||
| if err := tx.Sign( | ||
| m.factory, | ||
| "SOME KEY NAME!!!!", |
|
Superseded by #24 |
This is a port of the servicer from #9 to a cosmos module.