search

cowprotocol / dune-bridge

Other
4 stars 3 forks source link

Fix Quote Parsing #13

Closed bh2smith closed 2 years ago

bh2smith commented 2 years ago

According to the lengthy discussion had on #12 it was concluded that the most practical versioning for Quotes was the eliminate the possibility of including QuoteV1 type data in QuoteV2. This change is still pending implementation to the spec (a request made to the front end team). Follow the progress in slack

Working from App Data Schema (but pending changes)

bh2smith commented 2 years ago

Depends on merge of: https://github.com/cowprotocol/cow-sdk/pull/43

alfetopito commented 2 years ago

From what I understood, we want to deny deserialization of quote data if it has extra fields.

If that is the case, then there are two changes that I think need to be done:

1. Add `quote_id` field to `QuoteV1`

2. Add `deny_unknown_fields` to the `#[serde(...)]` attribute

I disagree. If you can get the info you need from the fields present, I wouldn't fail in case there's garbage in the request. JSON schema doesn't fail if there are extra fields not specified in the schema. (at least on sdk-unit tests and schema validation it's accepted)

nlordell commented 2 years ago

I disagree.

Ah ok, fair point. My understanding is that might lead to inconsistencies with the computed app data hash (i.e. deserializing > serializing > hashing will be different that the original content ID hash).

bh2smith commented 2 years ago

My understanding is that might lead to inconsistencies with the computed app data hash (i.e. deserializing > serializing > hashing will be different that the original content ID hash).

This is a valid concern. Although on this side of the application we merely fetch existing content. The fact that we fail to parse and ignore all field is a bit alarming, although we don't actually compute hash anything here.

I definitely think your point is worth considering.

alfetopito commented 2 years ago

I disagree.

Ah ok, fair point. My understanding is that might lead to inconsistencies with the computed app data hash (i.e. deserializing > serializing > hashing will be different that the original content ID hash).

My understanding is that, as Ben confirms, here we'll be simply reading (de-serializing) the data, not the other way around. If we are talking about creating hashes, I then agree with your point of view

bh2smith commented 2 years ago

Thanks all for the feedback! Its shipping time.