Detach wallet

[altafan]

This contains a HUGE refactor of the daemon with the main purpose of detaching the embedded wallet and make it connect to an external ocean wallet instead. Secondary, this refactors the scaffolding and file naming for app and domain layers in order to increase the readability and maintainability of the codebase.

Changes

• All stuff related to Vault and Unspent domain/repo has been dropped.
• A new portable interface ~OceanWallet~ WalletService has been introduced.
• Strong refactor of how the grpc services are started to decrease overall code complexity
• Refactor of the env vars to configure the daemon at startup
• Updates to external pkgs in order to user latest go-elements and psetv2.
• New tdex-daemon/v2 proto that requires updates to clients such as the feeder or the dashboard that interact with operator interface.
• Updated e2e test in order to use dockerized services instead of compiled binaries.

Note: I'll provide more detailed clues about the macro changes listed above in later comments.

Closes #260. Closes #309. Closes #413. Closes #415. Closes #482. Closes #539. Closes #552. Closes #654. (Removed env vars to fix asset)

Please @tiero review.

[sekulicd]

Also maybe we should have docker_compose in root and have 'make dev' cmd in Makefile which would start tdexd and ocean

[altafan]

@sekulicd do you have any suggestion for the CodeQL failing action (InsecureSkipVerify)? I tried several things but none of them seemed to work. Not sure why now it's not working now. I didn't actually change that and before this PR it was not giving problems...

[tiero]

CodeQL

Shouldn't //nosec enough?

[altafan]

I solved by manually dismissing those alerts with "Won't fix" reason.

[altafan]

Changes to env vars

• Removed:
  • TRADE_EXPIRY_TIME - the expiration time is given by the ocean wallet when selecting the utxos. At the moment ocean protos don't expect this value to be set within the request message. Until we eventually commit this change in ocean, this env var won't be restored.
  • EXPLORER_* - The daemon doesn't make use of an explorer service anymore.
  • BASE_ASSET, QUOTE_ASSET - The daemon doesn't make use of fixed base/quote asset anymore. Any kind of market can be created.
  • NETWORK, NATIVE_ASSET - The daemon retrieves info about the network from the underlying ocean wallet.
  • CRAWL_INTERVAL - The daemon doesn't have any underlying component taking care of watching the blockchain anymore.
  • TRADE_SATS_PER_BYTE - Always defaults to 0.1 sats/VByte. This env var was removed for easiness, but it can (should?) be restored.
  • MAX_FAILING_REQUESTS, FAILING_RATIO - The daemon doesn't make use of any ratelimiter/circuitbreaker anymore.
  • RESCAN_* - Wallet restoration isn't yet available since it's a feature under development, but in general maybe better to provide these info in the request message.
• Renamed (this refactor was the opportunity to give some env vars a more expressive name):
  • DATA_DIR_PATH --> DATADIR
  • DEFAULT_FEE --> PERCENTAGE_FEE
  • SSL_KEY, SSL_CERT --> TRADE_TLS_KEY, TRADE_TLS_CERT
• Added:
  • WALLET_ADDR - address of the ocean wallet service to connect to
  • DB_TYPE - type of db to use (for now only badger, soon we will support also postgres)

[tiero]

The daemon doesn't make use of fixed base/quote asset anymore. Any kind of market can be created.

how do we account for it when restoring? how do we know which is base and which is quote? @altafan

[tiero]

This env var was removed for easiness, but it can (should?) be restored.

Maybe this should be calculated dynamically looking at meempool? user may want to turn off this as "priority" or not

[altafan]

how do we account for it when restoring? how do we know which is base and which is quote?

The user could pass a list of base assets when restoring a wallet. In general, my intention was to move this kind of config from env var to request message so the user can pass this info at runtime.

[altafan]

Maybe this should be calculated dynamically looking at meempool? user may want to turn off this as "priority" or not

Let's put this on pause for now. We'll create a dedicated ticket as soon as this PR gets merged so we can discuss about it further

[altafan]

Changes to tdex-daemon/v2 proto compared to v1:

• Removed
  • WalletService - this service was meant to be used as operator's personal wallet. He can now use directly ocean for this purpose.
  • OperatorService/Claim* rpc - not needed anymore because the daemon gets notified about deposits/withdrawals from the ocean wallet. This is a breaking change and requires clients making use of such apis (dashboard) to be updated accordingly.
  • OperatorService/GetMarketBalance rpc - removed since duplicate of GetMarketInfo.
  • OperatorService/GetMarketCollectedFees rpc - merged into GetMarketReport.
• Renamed:
  • WalletUnlockerService --> WalletService
  • WalletUnlockerService/IsReady --> WalletService/GetStatus
  • OperatorService/GetInfo --> WalletService/GetInfo
  • OperatorService/Get*Address --> OperatorService/Derive*Addresses - moved to plural since the rpc returns a list of newly derived addresses.
• Added:
  • WalletService/LockWallet rpc - useful to lock the daemon and its wallet without switching it off. This action disables every interface but the WalletService one like it was just started and waiting to be unlocked.
• Other changes:
  • Replaced all bytes message fields with string. The original bytes fields are now encoded as reversed hex strings.
  • Fixed all enums with *_UNSPECIFIED = 0 first value as per buf recommended practice.
  • Expect a list of Receivers in all Withdrawal rpcs to make them consistent.
  • Use int64 for all timestamp message fields.
  • Balance returned by OperatorService/GetMarketInfo is expressed as map of pairs <asset_hash, { unconfirmed_balance, confirmed_balance, locked_balance, total_balance }>

[tiero]

Changes to tdex-daemon/v2 proto compared to v1:

cc/ @Janaka-Steph

thanks to the versioning system of protobuf/buf, we are sure we can handle the v1>v2 migration

[Janaka-Steph]

• Getting GetMarketReport failure - RpcError: not implemented, method seems missing.

• Getting ListTrades failure - RpcError: Skip must be set to a positive number if page: { number: 0, size: 5 }. Number should be at least 1. What should be the behaviour of number: 0? Error message should be more explicit.

[sekulicd]

I want to re-discuss/challenge the way we are handling types in different layers in this PR.

Purpose of ports pkg is to isolate core from calls to 3rd party apps/infra. In this PR we moved almost all types to ports, even some that doesnt "leave" our core. I will use GetMarketReport method as an example and its arg TimeRange type. TimeRange is simple struct or DTO used only to pass some data around as a whole, as opposed to several individual variables. Before this TimeRange was defined as struct with its validation/toDate methods and now it is defined in ports as an interface. Representing DTO(in this case TimeRange) as an interface, introduces only unnecessary complexity. In this PR we are doing validation in upper layer(handler) which i find wrong as validation should happen in method using actual struct and not somewhere outside, since like this method is only "protected" in case it is called in this flow.

To summarise things i see problematic:

1. Representing simple structs as interfaces and then creating additional structs in different layers that implements that interface, dont see any pros only adding complexity
2. Moving types to ports that are not "leaving" core
3. Validation structs outside of method using actual struct, not protecting method using validated struct if called from somewhere else, eg. test

Please share your thoughts. @tiero @altafan

[tiero]

2. Moving types to ports that are not "leaving" core

I agree on this, better to have little code replicated, but at least we can swap all layers without breaking anything in the future.

Validation structs outside of method using actual struct

+1

[altafan]

* Getting `ListTrades failure - RpcError: Skip must be set to a positive number` if `page: { number: 0, size: 5 }`. Number should be at least 1. What should be the behaviour of `number: 0`? Error message should be more explicit.

@Janaka-Steph fixed.

[altafan]

Purpose of ports pkg is to isolate core from calls to 3rd party apps/infra. In this PR we moved almost all types to ports, even some that doesnt "leave" our core.

At the beginning, I didn't want to make the stuff defined in ports be used like cross-layer types. You have to consider that most of the interfaces defined in ports are related to the WalletService (ocean), like for example WalletAccount, TxInput, TxOutputs, etc.

Even if we get back to the prev solution (ie. each layer has its own types, even if exactly the same), most of those interface will stay there anyway because they're related to an external service.

Then, it turned out that most of these interfaces are very similar to the proto types used by the interfaces layer, while the app layer have to deal with them in any case since it has to interact with the external wallet. Because of these reasons, I decided to use the interfaces as cross-layer types.

The main advantage of this approach is that we reduce the amount of repeated code, therefore the redundancy of potential bugs, and also allows us to write validators only once for any of those types.

Validation structs outside of method using actual struct, not protecting method using validated struct if called from somewhere else, eg. test

I definitely agree with this and I'm glad it came out because I just forgot about it. As I said just above, it would be enough to add validators (and related unit tests) and use them in both interface and application layers where the types are used.