Refactor backend

Question

Refactor backend

Closed this issue a year ago · 15 comments

sideninja commented 2 years ago

Refactor backend implementation following the steps:

move emulator related functionality to emulator instead of backend ( there is no difference there )
emulator with go-sdk types can service flowkit and other consumers
we can rename the backend to something more meaningful, and use flow-go types there. ( as rest API and grpc API directly using that ) maybe backend can be 'adapter' ( adapting emulator to grpc and rest API )

cc @bluesign

Answer 1 · 2022-11-17T10:26:53.000Z

@sideninja I started this, but a few things came up to clear up.

Emulator has mixed usage of flow-go and go-sdk types ( especially block is flow-go typed ) I think this is because go-sdk doesn't have good block representation.
storage backend is using flow-go types.

This complicates the problem a bit, any ideas?

Answer 2 · 2022-11-18T18:10:09.000Z

I feel that flow-emulator should be using native flow-go types all around the codebase (like it already does for most) and then we should have a package that would accept and return go-sdk types that can be used by codebases importing that directly.

I feel the backend could be refactored into blockchain, since a lot of things are already part of it, and they can use flow-go types. Then we could have an adapter layer that would consume and produce go-sdk types that would just wrap the blockchain interface.
But honestly this is really quick idea, I would have to dig more to have a better plan.

Answer 3 · 2022-11-18T18:21:37.000Z

I feel that flow-emulator should be using native flow-go types all around the codebase (like it already does for most)

This part I agree 100%, go-sdk types are more like API response mapped. Not possible to extend them without confusing people.

I feel the backend could be refactored into blockchain, since a lot of things are already part of it, and they can use flow-go types.

Yeah as long as we use flow-go it is all good there.

we should have a package that would accept and return go-sdk types that can be used by codebases importing that directly.

This would be nice, but not sure, if it creates some limits for future. Imagine:

Let's say block, and when we expose emulator we wanted to expose View[0] from Block
we cannot add View to go-sdk.Block, then we would confuse people using it as AN client ( they will complain View is coming empty etc )

I think go-sdk types are a bit dead end to be honest for this reason. But I will do the flow-go type and blockchain-backend merge as a start.

[0]: View I gave as an example, maybe there is View in go-sdk Block, I don't recall now.

Answer 4 · 2022-11-18T18:39:19.000Z

I think go-sdk types are a bit dead end to be honest for this reason.

The thing I worry is that there's a lot of added functionality Go SDK can support on their types, like build transactions, signing etc (that's not part of flow-go types and probably never will be) so if we want to make Go SDK easy to use it will probably have to have it's own types and somehow then those types needs to work with flow-emulator and other networks. We could possibly make that work in go-sdk too. Not sure yet.

Answer 5 · 2023-01-18T20:22:20.000Z

@sideninja, I take another stab at this, and this time I think it is good.

Basically:

blockchain: flow-go types
adapter with blockchain: grpc and rest serving
backend with blockchain: go-sdk types interface ( we can move this to go-sdk too )

What is missing:

logging ( where we should put the logging, adapter level seems the logical choice )
I think emulator has some missing info recently added, like blockID for transactionResult etc ( need to add at storage level )
tests needs update ( not too hard though )

I will try to make a draft PR tomorrow, maybe we can talk over it

Answer 6 · 2023-02-14T17:08:51.000Z

@sideninja I did this finally but there is a new problem here.

I made 2 adapters (sdk, grpc) working on Emulator interface ( Blockchain is our implementation )

Everything was good till I come to tests, as we moved a lot of stuff around, almost all tests have to be rewritten, but more than that, I think logic of the tests also have to change.

I need a bit guidance here about how to write the tests. Mock emulator then test? It seems a bit pointless, considering we have test for convert. I mean SDK layer is basically wrapping call to emulator, converting inputs and outputs. But I am a bit testing noob.

Also I need I think some deep-compare library if I mock emulator, ( I give let's say flow block, it will convert to sdk block, then I need to again convert it to flow block, but those objects are not comparable afaik )

Answer 7 · 2023-02-15T09:44:42.000Z

@bluesign do you have a link so I can check the code to give better advice?

Answer 8 · 2023-02-15T13:07:02.000Z

ah sorry @sideninja, I pushed now to https://github.com/bluesign/flow-emulator/tree/reorgBackend

Answer 9 · 2023-04-11T11:24:09.000Z

@sideninja I need little help here with how to make testing.

Answer 10 · 2023-05-05T09:12:20.000Z

I think this is a good approach. What specifically you need help with testing (sorry for late reply).

Answer 11 · 2023-05-05T09:19:00.000Z

No worries @sideninja. Main problem I had was almost all tests has to rewritten.

Also for example some tests were seemed redundant; for example: if we have some function wrapper for emulator function, such as: get input, convert input, call emulator function, convert result, return result. When we have test for convert functions and emulator, it seems no sense to test this, but on the other hand it seems we should test this anyway.

In general I am a bit clueless how to write tests after refactor. ( probably need to rewrite most )

Answer 12 · 2023-05-05T14:13:02.000Z

I think adapters should be tested by using a mocked version of the emulator, it should only test whether it adapts to the expected type, using the emulator mock to just return a simple type.
The emulator tests should mostly be reused from the transactions_test, script_test, account_test. I believe they should work by just replacing blockchain with emulator type and making sure correct types are used.
For generating emulator mocks in case of adapter tests you can use the https://github.com/vektra/mockery and you can use Go SDK for creating SDK types https://github.com/onflow/flow-go-sdk/blob/master/test/entities.go
For Flow Go types you can use fixtures here https://github.com/onflow/flow-go/blob/master/utils/unittest/fixtures.go
Let me know more specifically how I can help here.

Answer 13 · 2023-05-06T14:13:28.000Z

thanks @sideninja, I will jump into this, I think I have some understanding now, I think better I ask for advice when some situation arises.

Answer 14 · 2023-05-06T19:23:20.000Z

ok after 5 hours, I ended up with something as a starter: https://github.com/bluesign/flow-emulator/tree/reOrg

Missing adapter tests; SDK adapter test I somehow sandwiched into old blockchain tests. I felt like they should be ok but not sure. Coverage seems to increase 5-6%. ( I will dig into it )

Answer 15 · 2023-05-08T07:23:26.000Z

I think that looks good generally. Blockchain tests were integration tests even before anyway. I would add unit tests on the adapter tho, especially the access by mocking the blockchain out (by the newly defined interface).