Make Desktop's DTOs reflect status-go types again

[osmaczko]

Description

DTO types that are defined in src/app_service/service/*/dto/*.nim should mirror the types in status-go. This used to be the case, but it has diverged over time. This is causing confusion, making the code hard to follow and difficult to reason about, which in turn negatively affects code extensibility and maintainability. This issue is particularly evident when data from status-go needs extension or adaptation.

As an example, ChatDto's members field does not reflect status-go's members field of Chat.

status-go:

type Chat struct {
...
	Members []ChatMember `json:"members"`
}

type ChatMember struct {
	ID string `json:"id"`
	Admin bool `json:"admin"`
}

status-desktop:

type ChatDto* = object
...
  members*: seq[ChatMember]
  
type ChatMember* = object
  id*: string
  joined*: bool
  role*: MemberRole
  airdropAccount*: RevealedAccount

This discrepancy is resulting in the following logic:

proc toChatDto*(jsonObj: JsonNode): ChatDto =
...
  var membersObj: JsonNode
  if(jsonObj.getProp("members", membersObj)):
    if(membersObj.kind == JArray):
      # during group chat updates
      for memberObj in membersObj:
        result.members.add(toGroupChatMember(memberObj))
    elif(membersObj.kind == JObject):
      # on a startup, chat/channel creation
      for memberId, memberObj in membersObj:
        result.members.add(toChatMember(memberObj, memberId))

As can be seen, toChatDto is being called on different JSONs. It's impossible to understand the intent from the code without inspecting these JSONs, the flows, and where they differ.

Proposal:

• Always ensure that status-go types are mirrored 1-to-1 in DTOs.
• Create domain types where needed. For example:

src/app_service/service/*/domain/*.nim

type ChatMember* = object
    pubKey*: string
    case kind*: DetailsKind
    of kCommunity:
      communityDetails*: CommunityDetails
    of kGroupChat:
      groupChatDetails*: GroupChatDetails

• ❓ Keep DTOs internal and use domain types across the app. For example: introduce Chat type with fields relevant to status-desktop and use ChatDto only for parsing and services processing.

[0x-r4bbit]

I generally agree with the sentiment. I guess part of why this is so inconsistent is also because it would mean even more boilerplate code to get values to and from the UI layer.

For example, the TokenPermissionDto objects are created in various places coming from multiple directions.
You either get them from status-go, in which case most of the fields match 1:1, in other cases however, you receive data from the UI and need to construct a Dto from that so it can be passed back to the service.

^ In this case we'd actually need to introduce yet another type that is able to adapt between service layer <-> view layer.

I'm not saying I'm disagreeing, quite the opposite, but often times it's already a pain to introduce new data structures just to get some primitive value carried all the way to the user interface.

I guess once all necessary domain types are in place, this will be less annoying. But it'll be a significant amount of work to get there.

[osmaczko]

The evolution of ChatMember:

type ChatMember* = object
  id*: string
  admin*: bool
  joined*: bool

 type ChatMember* = object
   id*: string
   admin*: bool
   joined*: bool
+  roles*: seq[int]

 type ChatMember* = object
   id*: string
-  admin*: bool
   joined*: bool
-  roles*: seq[int]
+  role*: MemberRole

 type ChatMember* = object
   id*: string
   joined*: bool
   role*: MemberRole
+  airdropAccount*: RevealedAccount

[osmaczko]

I think the root of the problem is the type inconsistency coming from status-go. status-go signals chat changes through protocol's Chat, while explicit fetch is done through API's Chat. Sadly, they are not the same. For instance, they differ in how members are represented:

protocol:

// ChatMember represents a member who participates in a group chat
type ChatMember struct {
        // ID is the hex encoded public key of the member
        ID string `json:"id"`
        // Admin indicates if the member is an admin of the group chat
        Admin bool `json:"admin"`
}

api:

type Member struct {
        // Community Role
        Role protobuf.CommunityMember_Roles `json:"role,omitempty"`
        // Joined indicates if the member has joined the group chat
        Joined bool `json:"joined"`
}

status-desktop assumes ChatDto is always the same for both. That's why we ended up with Frankenstein ChatMember that represents both of them and neither of them accurately.

[osmaczko]

I generally agree with the sentiment. I guess part of why this is so inconsistent is also because it would mean even more boilerplate code to get values to and from the UI layer.

For example, the TokenPermissionDto objects are created in various places coming from multiple directions. You either get them from status-go, in which case most of the fields match 1:1, in other cases however, you receive data from the UI and need to construct a Dto from that so it can be passed back to the service.

^ In this case we'd actually need to introduce yet another type that is able to adapt between service layer <-> view layer.

Does TokenPermissionDto contain fields status-go is not aware of?

I'm not saying I'm disagreeing, quite the opposite, but often times it's already a pain to introduce new data structures just to get some primitive value carried all the way to the user interface.

I agree. In ideal world, domain types should be DTOs. We wouldn't need extra types if status-go is consistent and sends what we need.

I guess once all necessary domain types are in place, this will be less annoying. But it'll be a significant amount of work to get there.

Right. I believe we can go hybrid. Treat DTOs as domain types as long as they are mapped 1 to 1 with what status-go represents and introduce domain types only when needed, a good example of that: https://github.com/status-im/status-desktop/blob/master/src/app_service/service/contacts/dto/contact_details.nim (now I realized it shouldn't be inside the dto subfolder).

[0x-r4bbit]

Does TokenPermissionDto contain fields status-go is not aware of?

Ah no, it's not exactly that. But there are to*Dto() methods that try to account for both sides of the coin. This can be confusing too, but I just realized it's not exactly the same thing you've been exploring here.