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

Replace the pass-through querier with an extension API #5325

Closed
3 tasks
srdtrk opened this issue Dec 6, 2023 · 2 comments · Fixed by #5345
Closed
3 tasks

Replace the pass-through querier with an extension API #5325

srdtrk opened this issue Dec 6, 2023 · 2 comments · Fixed by #5345
Assignees
Labels
08-wasm audit Feedback from implementation audit

Comments

@srdtrk
Copy link
Member

srdtrk commented Dec 6, 2023

Summary

This proposal recommends removing the pass-through querier from the 08-wasm module in ibc-go and replacing it with an extension API for the default querier, focusing initially on Stargate queries. The change addresses concerns regarding user complexity, lack of standardization, and API inconsistency. It introduces a .AddWhitelistedQuery method to 08-wasm's keeper, allowing for a controlled, gRPC based whitelist-based querying mechanism. This modification is aimed at aligning 08-wasm with wasmd practices, enhancing the module's usability and consistency with existing standards.

Problem Definition

In wasmd, queriers are not typically exposed to the public API. However, an exception was made for the 08-wasm querier in #5192.
Upon review and consultation with @webmaster128, several concerns have been raised about this approach:

  1. Complexity for Users: Proper implementation of the querier requires detailed knowledge and care, such as reporting of gas values in CosmWasm gas units
  2. Lack of Standardization: Different querier standards across chains may result in some contracts being incompatible with certain chains.
  3. API Inconsistency: There is a disparity in API access; for instance, the querier in wasmd can access sdk.Context, whereas the pass-through querier in ibc-go cannot.

Proposal

To align with wasmd's practices, we propose the removal of the pass-through querier from 08-wasm. Instead, we suggest implementing an extension API for the 08-wasm's default querier. Initially, we plan to support only Stargate (protobuf gRPC) queries, enabling contracts to perform any whitelisted protobuf gRPC query and receive responses accordingly.

To facilitate this, we'll introduce a .AddWhitelistedQuery method in the 08-wasm keeper. This method will allow adding queries to a default, initially empty, whitelist of queries.

Modifications

  1. The GRPCQueryRouter needs to be passed to the 08-wasm through a new QueryRouter interface.
// `internal/ibcwasm/expected_interfaces.go`

type QueryRouter interface {
	// Route returns the GRPCQueryHandler for a given query route path or nil
	// if not found
	Route(path string) baseapp.GRPCQueryHandler
}
// `internal/ibcwasm/wasm.go`

var (
	vm WasmEngine

-	querier wasmvm.Querier
+	queryRouter QueryRouter

	// ...
)
// SetQueryRouter sets the query router for the 08-wasm module.
// It panics if the query router is nil.
func SetQueryRouter(router QueryRouter) {
	if router == nil {
		panic(errors.New("query router must be not nil"))
	}
	queryRouter = router
}  
  
// GetQueryRouter returns the query router for the 08-wasm module.
func GetQueryRouter() QueryRouter {
	return queryRouter
}

We then add a new QueryRouter field to the NewKeeper in the 08-wasm module:

func NewKeeperWithVM(
	cdc codec.BinaryCodec,
	storeService storetypes.KVStoreService,
	clientKeeper types.ClientKeeper,
	authority string,
	vm ibcwasm.WasmEngine,
-	querier wasmvm.Querier,
+	queryRouter ibcwasm.QueryRouter,
) Keeper {
	// ...
+	ibcwasm.SetQueryRouter(queryRouter)
	// ...
}
// app.go

if mockVM != nil {
	// NOTE: mockVM is used for testing purposes only!
	app.WasmClientKeeper = wasmkeeper.NewKeeperWithVM(
		appCodec, runtime.NewKVStoreService(keys[wasmtypes.StoreKey]), app.IBCKeeper.ClientKeeper,
-		authtypes.NewModuleAddress(govtypes.ModuleName).String(), mockVM, nil,
+		authtypes.NewModuleAddress(govtypes.ModuleName).String(), mockVM, app.GRPCQueryRouter(),
	)
} else {
	app.WasmClientKeeper = wasmkeeper.NewKeeperWithConfig(
		appCodec, runtime.NewKVStoreService(keys[wasmtypes.StoreKey]), app.IBCKeeper.ClientKeeper,
-		authtypes.NewModuleAddress(govtypes.ModuleName).String(), wasmConfig, nil,
+		authtypes.NewModuleAddress(govtypes.ModuleName).String(), wasmConfig, app.GRPCQueryRouter(),
	)
}
  1. Default querier be updated to use the QueryRouter interface for Stargate queries. (See #5310)

  2. A whitelist of queries should be added to the 08-wasm module as a constant variable or a module parameter, pending further discussion.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged/assigned
@srdtrk srdtrk added audit Feedback from implementation audit 08-wasm needs discussion Issues that need discussion before they can be worked on labels Dec 6, 2023
@aeryz
Copy link
Contributor

aeryz commented Dec 6, 2023

I think that it's a great addition to have but I think we could support both custom queries and stargate queries at the same time by having an interface similar to what x/wasm module provides:

accepted := wasmkeeper.AcceptedStargateQueries{
	"/cosmos.auth.v1beta1.Query/Account": &authtypes.QueryAccountResponse{},
}
querierOpts := wasmkeeper.WithQueryPlugins(
	&wasmkeeper.QueryPlugins{
		Stargate: wasmkeeper.AcceptListStargateQuerier(accepted, app.GRPCQueryRouter(), appCodec),
	})

This way, users can provide both custom query and accepted stargate queries.

We can also restrict the above interface a bit more and instead of getting the stargate querier implementation as a parameter, we can make the accept list, QueryRouter and custom query function configurable and have a wasmvm.Querier implementation similar to what @srdtrk has in his PoC with a minor custom query addition such as:

// Query implements the wasmvmtypes.Querier interface.
func (q *DefaultQuerier) Query(request wasmvmtypes.QueryRequest, gasLimit uint64) ([]byte, error) {
	sdkGas := VMGasRegister.FromWasmVMGas(gasLimit)

	subCtx, _ := q.Ctx.WithGasMeter(storetypes.NewGasMeter(sdkGas)).CacheContext()

	// make sure we charge the higher level context even on panic
	defer func() {
		q.Ctx.GasMeter().ConsumeGas(subCtx.GasMeter().GasConsumed(), "contract sub-query")
	}()

	if request.Stargate != nil {
		return handleStargateQuery(subCtx, request.Stargate, e.acceptList, e.router) // NEW: Provide the user defined accept list and router
	} else if request.Custom != nil {
                return q.customQueryHandler(subCtx, request.Custom) // NEW: Pass execution to the user defined custom query handler
        }
	return nil, wasmvmtypes.UnsupportedRequest{Kind: "unsupported query type"}
}

@webmaster128
Copy link
Member

The most important change here is to internalize the wasmvm.Querier instance. It is designed for the wasmvm<->wasmd communication and not safe to expose to 08-wasm users. Once there is a single Querier implementation in 08-wasm, you can design all sorts of plugin systems on top. In wasmd this is done via the QueryPlugins but the API can be different here.

@srdtrk srdtrk self-assigned this Dec 7, 2023
@srdtrk srdtrk removed the needs discussion Issues that need discussion before they can be worked on label Dec 7, 2023
@crodriguezvega crodriguezvega added this to the 08-wasm/v0.1.0 milestone Dec 7, 2023
@crodriguezvega crodriguezvega moved this to In progress in ibc-go Dec 7, 2023
@github-project-automation github-project-automation bot moved this from In progress to Done in ibc-go Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
08-wasm audit Feedback from implementation audit
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants