Skip to content
This repository has been archived by the owner on Jun 10, 2022. It is now read-only.

response payload mismatch between websocket monitor response payload #42

Closed
rg911 opened this issue Jun 14, 2019 · 11 comments
Closed

response payload mismatch between websocket monitor response payload #42

rg911 opened this issue Jun 14, 2019 · 11 comments
Assignees
Labels
bug Something isn't working

Comments

@rg911
Copy link
Contributor

rg911 commented Jun 14, 2019

Symptom

Rest response payload mismatch between websocket monitor response payload e.g. transaction confirmed and Rest endpoint response payload account/{publicKey}/transactions.

Example (AccountPropertyAddress Transaction):
  • Monitor response is using modificationType inside modifications array
    modifications: 
    [ { modificationType: 1,
        value: '903EFA58C54681F5FBA106055676178960E4510944EA06ACBB' } ]
    
  • Rest endpoint (transaction list) e.g. account/{publicKey}/transactions uses type
  modifications: 
  [ { type: 1,
      value: '903EFA58C54681F5FBA106055676178960E4510944EA06ACBB' } ]

Affect:

  • Account Property Transactions (all 3) : modificationType -> type
  • Account Link Transaction: actionType -> type
  • Address Alias Transaction: actionType -> type
  • Mosaic Alias Transaction: actionType -> type
@rg911 rg911 added the bug Something isn't working label Jun 14, 2019
@rg911 rg911 changed the title Response payload on AccountPropertyTransaction mismatch. response payload mismatch between websocket monitor response payload Jun 15, 2019
@Vektrat
Copy link
Contributor

Vektrat commented Jun 27, 2019

two things:

  1. modifcationType is type in mongo, however, it is modificationType in the catbuffer schemas, so the discrepancy is not on REST hands i believe
  2. actionType where is this? in the mongo collection we can only see the (transaction) type, and then, what i believe is what you are asking about, action - are you talking about monitor? REST endpoints? in the endpoints we are currently using aliasAction and linkAction

@rg911
Copy link
Contributor Author

rg911 commented Jun 27, 2019

The main issue is when the payload schemas are different between API call response and websocket messages. I don't think it really matters about the names as long as they are consistent.

@Vektrat
Copy link
Contributor

Vektrat commented Jul 2, 2019

  1. is related to rename account_properties to restriction_account to match server and catbuffer #49 though the discrepancy is there, depending on the decision expect this to be fixed by that issue

@gimre-xymcity
Copy link
Member

@Vektrat @rg911

It's crucial to understand, that what is reported via REST apis is a STATE not transactions, so i.e. there's:
modifications: type, cause that is what is inside mongodb
HOWEVER, what is reported via websockets are TRANSACTIONS and IIRC mapping is done on REST layer, and generally (and that would be ideal) field names should match names that are used in SDKs.

I'm not sure if it's a good idea to make both of these the same.

@rg911
Copy link
Contributor Author

rg911 commented Jul 2, 2019

To be more specific:
looking at this line
https://github.com/nemtech/catapult-rest/blob/e07c49a9c90e759cdc10b5799e82eda4214b37e5/catapult-sdk/src/plugins/accountLink.js#L44

If mongodb has 'action' instead of 'linkAction', then the websocket payload will return different payload I'd thought??

@Vektrat
Copy link
Contributor

Vektrat commented Jul 2, 2019

If mongodb has 'action' instead of 'linkAction', then the websocket payload will return different payload I'd thought??

mongodb data is exposed directly to the REST API as json, websocket data comes from the server, not mongodb

@rg911
Copy link
Contributor Author

rg911 commented Jul 2, 2019

If mongodb has 'action' instead of 'linkAction', then the websocket payload will return different payload I'd thought??

mongodb data is exposed directly to the REST API as json, websocket data comes from the server, not mongodb

Then it's a bit of pain on the SDK as RESP repos and ws are sharing the same serialisation / serialisation code, that's the root cause of the issue. IMO, we should expect consistent transaction schema from the REST, as SDK won't talk to server directly.

@Vektrat
Copy link
Contributor

Vektrat commented Jul 2, 2019

Then it's a bit of pain on the SDK as RESP repos and ws are sharing the same serialisation / serialisation code, that's the root cause of the issue. IMO, we should expect consistent transaction schema from the REST, as SDK won't talk to server directly.

well but this is what gimre is saying, even though in both cases we are talking about transactions, for the REST API (MongoDB), this is state, and the WS return deserialized transactions - even though it is conceptually the same, they are treated as different type entities, and the SDK seems to be trying to use the same schemas for both of them

@rg911
Copy link
Contributor Author

rg911 commented Jul 2, 2019

well but this is what gimre is saying, even though in both cases we are talking about transactions, for the REST API (MongoDB), this is state, and the WS return deserialized transactions - even though it is conceptually the same, they are treated as different type entities, and the SDK seems to be trying to use the same schemas for both of them

🤔 Lower level implementations like the SDK shouldn't care the origin of the data source. Payload format on the 'same` entity should be consistent. Although they are conceptually different in terms of their origin, but they are the same from the output point of view, and they both get mapped at the REST level. Majority of the fields in transactions are all matched, I don't see a reason why this field cannot be matched? Thinking about not only the SDK, other implementations will also face the same issue. I don't think it's user friendly.

@gimre-xymcity
Copy link
Member

gimre-xymcity commented Jul 2, 2019

Payload format on the 'same` entity should be consistent.

Unless we're talking of different things that is NOT same entity. (need to check)

@gimre-xymcity
Copy link
Member

after discussion on slack, changes that will be done to mongodb mappers (this might affect rest schemas):

tx change
AccountLink action -> linkAction
ModifyMultisig type -> modificiationType
AddressAlias action -> aliasAction
MosaicAlias action -> aliasAction
AccountRestriction type -> restrictionType

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants