Refactor services to use an interface

[neil-and-void]

Proposal

Refactor Service layer to be a single interface with the methods of its members ie GetBlocks, GetTransaction, etc.

[sideninja]

Let's define the whole interface here. Also make sure to link in the language server that will be affected and ping some of known users of flowkit like @bjartek

[neil-and-void]

Currently Services are defined as a struct:

type Services struct {
	Accounts     *Accounts
	Scripts      *Scripts
	Transactions *Transactions
	Keys         *Keys
	Events       *Events
	Collections  *Collections
	Project      *Project
	Blocks       *Blocks
	Status       *Status
	Snapshot     *Snapshot
}

The Proposal is to refactor the struct to be defined as follows:

type Services interface {
	GetAccount(address flow.Address) (*flow.Account, error)
	StakingInfo(address flow.Address) ([]map[string]interface{}, []map[string]interface{}, error)
	NodeTotalStake(nodeId string, chain flow.ChainID) (*cadence.Value, error)
	CreateAccount(
		signer *flowkit.Account,
		pubKeys []crypto.PublicKey,
		keyWeights []int,
		sigAlgo []crypto.SignatureAlgorithm,
		hashAlgo []crypto.HashAlgorithm,
		contractArgs []string,
	) (*flow.Account, error)
	AddContract(
		account *flowkit.Account,
		contractName string,
		contractSource []byte,
		updateExisting bool,
	) (*flow.Account, error)
	RemoveContract(
		account *flowkit.Account,
		contractName string,
	) (*flow.Account, error)
	GetTransactionStatus(
		id flow.Identifier,
		waitSeal bool,
	) (*flow.Transaction, *flow.TransactionResult, error)
	BuildTransaction(
		proposer flow.Address,
		authorizers []flow.Address,
		payer flow.Address,
		proposerKeyIndex int,
		code []byte,
		codeFilename string,
		gasLimit uint64,
		args []cadence.Value,
		network string,
		approveBuild bool,
	) (*flowkit.Transaction, error)
	SignTransaction(
		signer *flowkit.Account,
		payload []byte,
		approveSigning bool,
	) (*flowkit.Transaction, error)
	SendSignedTransaction(
		payload []byte,
		approveSend bool,
	) (*flow.Transaction, *flow.TransactionResult, error)
	SendTransaction(
		signer *flowkit.Account,
		code []byte,
		codeFilename string,
		gasLimit uint64,
		args []cadence.Value,
		network string,
	) (*flow.Transaction, *flow.TransactionResult, error)
	GetRLP(rlpUrl string) ([]byte, error)
	PostRLP(rlpUrl string, tx *flow.Transaction) error
	ExecuteScript(code []byte, args []cadence.Value, scriptPath string, network string) (cadence.Value, error)
	GenerateKeys(inputSeed string, sigAlgo crypto.SignatureAlgorithm) (crypto.PrivateKey, error)
	DecodeRLP(publicKey string) (*flow.AccountKey, error)
	DecodePEM(key string, sigAlgo crypto.SignatureAlgorithm) (*flow.AccountKey, error)
	GetEvent(events []string, startHeight uint64, endHeight uint64, blockCount uint64, workerCount int) ([]flow.BlockEvents, error)
	EventWorker(jobChan <-chan grpc.EventRangeQuery, results chan<- EventWorkerResult)
	GetCollection(id flow.Identifier) (*flow.Collection, error)
	InitProject(
		readerWriter flowkit.ReaderWriter,
		reset bool,
		global bool,
		sigAlgo crypto.SignatureAlgorithm,
		hashAlgo crypto.HashAlgorithm,
		serviceKey crypto.PrivateKey,
	) (*flowkit.State, error)
	DeployProject(network string, update bool) ([]*contracts.Contract, error)
	GetBlock(
		query string,
		eventType string,
		verbose bool,
	) (*flow.Block, []flow.BlockEvents, []*flow.Collection, error)
	GetLatestBlockHeight() (uint64, error)
	Ping(network string) (string, error)
	GetLatestProtocolStateSnapshot() ([]byte, error)
}

This will affect the following files in the cadence language server repo:

• https://github.com/onflow/cadence/blob/master/languageserver/integration/initialization.go
• https://github.com/onflow/cadence/blob/master/languageserver/integration/commands.go
• https://github.com/onflow/cadence/blob/master/languageserver/integration/resolving.go
• https://github.com/onflow/cadence/blob/master/languageserver/integration/integration.go

cc: @bjartek

[sideninja]

I would change Create -> CreateAccount Is this needed? prepareTransaction, if it's not exported then it probably shouldn't be part of it.

I think I would make sense to name the interface Flowkit. Not 100% sure about the naming, but I also not sure if I like it to be named Services.

Can you elaborate more on how will this allow testing and what's a suggested approach then on doing that part.

I would also take a look at the flowkit library improvement comments as discussed here: https://github.com/onflow/flow-cli/issues/493 We need to make sure these comments are reflected in this interface. One such example would be: we would need to remove the prompts in order to do that the SendTransaction would need to take in a handler function that would be called and in the case of CLI open a prompt.

[bluesign]

In the mean time maybe we can introduce context ? Prompts can work over context maybe too

[sideninja]

Yep, it's part of the list in the issue. Context should be the first arg for all of the methods.

I would even think about not exposing all the methods currently in the services since some are really very specific to the CLI. Maybe one such example would be: GetRLP/PostRLP, I think that is something if needed you would implement yourself.

[bluesign]

Btw @sideninja what do you think about getting config and projects out of flowkit ?

[sideninja]

Config as in accessing the state? That should already be possible, although not exposed by the proposed interface, but that might be fine. I don't want to only have one interface exposing all things in flowkit. There can be many so you can pick and choose what you need.

[bluesign]

Oh what I meant was moving config to CLI instead of keeping in flowkit.

From responsibilities point of view, it makes more sense. And actually API can evolve better for outside consumers if we lose this tight coupling.

[sideninja]

Honestly, I think an important part of the flowkit is flow.json. I see the flow.json to be the glue between different flow tools and having a library that knows how to parse it and modify it is important. I don't think that should be a CLI-specific thing. But, at the same time things can be refactored in a way that the "service" layer is less tightly coupled with that state/config.

[bluesign]

I think an important part of the flowkit is flow.json. I see the flow.json to be the glue between different flow tools and having a library that knows how to parse it and modify it is important.

Yeah this part is important, but also double edged sword a bit. If we couple loose though it can be optimum I agree.

[neil-and-void]

Can you elaborate more on how will this allow testing and what's a suggested approach then on doing that part.

The command functions in the cli accept a services struct but allowing them to accept an interface will make it easier to mock out for testing

// internal/blocks/get.go
 
func get(
	args []string,
	_ flowkit.ReaderWriter,
	_ command.GlobalFlags,
	services *services.Services,
) (command.Result, error) {...}

What we could do for testing is have a MockService struct that implements all the methods I defined above and pass that to each of the command functions. One thing I'm not too sure how to do is how to change the return values per each function for different test cases

[sideninja]

One thing I'm not too sure how to do is how to change the return values per each function for different test cases

You can use a service like https://github.com/golang/mock

[bjartek]

I have looked through overflow and these are the methods I use in there

script.go
78:	result, err := o.Services.Scripts.Execute(

event_fetcher_old.go
156:		blockHeight, err := e.OverflowState.Services.Blocks.GetLatestBlockHeight()
180:	blockEvents, err := e.OverflowState.Services.Events.Get(events, uint64(fromIndex), endIndex, e.EventBatchSize, e.NumberOfWorkers)

event_fetcher.go
75:		blockHeight, err := e.OverflowState.Services.Blocks.GetLatestBlockHeight()
97:	blockEvents, err := e.OverflowState.Services.Events.Get(events, uint64(fromIndex), endIndex, e.EventBatchSize, e.NumberOfWorkers)

state.go
40:	Services *services.Services
239:		if _, err := o.Services.Accounts.Get(account.Address()); err == nil {
244:		_, err := o.Services.Accounts.Create(
281:	contracts, err := o.Services.Project.Deploy(o.Network, false)
316:	return o.Services.Accounts.Get(rawAddress)
467:	block, _, _, err := o.Services.Blocks.GetBlock("latest", "", false)
473:	block, _, _, err := o.Services.Blocks.GetBlock(fmt.Sprintf("%d", height), "", false)
479:	block, _, _, err := o.Services.Blocks.GetBlock(blockId, "", false)

scripts_old.go
144:	result, err := f.Services.Scripts.Execute(

setup.go
113:	var service *services.Services
135:		service = services.NewServices(gw, state, logger)
146:		service = services.NewServices(gw, state, logger)
150:		Services:                            service,

interaction_builder.go
348:	tx, err := oib.Overflow.Services.Transactions.Build(
382:	ftx, res, err := oib.Overflow.Services.Transactions.SendSigned(txBytes, true)

so everything appears to be covered from my usage of this.