feat(api): availabilities patch endpoint #692

AuHau · 2024-02-01T15:19:08Z

I split the work on this PR from the work on #467. It is a prerequisite for implementing persistent availabilities as there will need to be a way how to update and terminate them.

This PR brings several changes and breaking changes to the API endpoints:

(breaking) Changing the POST /sales/availability successful response type to 201
(breaking) Changing the Availability structure where the size property was replaced by totalSize property which indicates the total capacity of the availability and freeSize which indicate the capacity of the availability that is available for selling
Adding PATCH /sales/availability/{id} that allows updating the availability properties except the freeSize. The updated properties are then used only for asserting the new incoming requests. The currently running requests are not effected by the update availability
Adding GET /sales/availability/{id}/reservations which returns the list of Reservations that are linked to the Availability.

Other notable addition is the Optionalize() macro which allows you to convert object type into object type with its properties converted into Option[T].

Lastly, I have spent quite some time originally debugging an issue, where originally the PATCH endpoint was returning 204 No Content successful reply, but there seems to be some bug in either of nim-presto, chronos server implementation or HTTPClient which I have summarized in reproduction repository here and opted instead to use simple 200 response instead.

dryajov · 2024-02-20T15:08:51Z

We should report the bug in presto

AuHau · 2024-02-14T13:47:15Z

codex/sales/reservations.nim

+    totalSize* {.serialize.}: UInt256
+    freeSize* {.serialize.}: UInt256


I have decided to split the "size" tracking property into two parts "total size" and "free size", where the second indicates how much is left from the availability.

While the totalSize could be calculated from size + sum(reservation.size) (which is the direction I originally took), it seemed as more KISS approach to simply have that extra property.

codex/sales/reservations.nim

AuHau · 2024-02-14T13:54:39Z

codex/utils/options.nim

+    Option[input]
+
+
+template Optionalize*(t: typed): untyped =


I originally planned to include an option to add the serialize pragma to the newly constructed object type. However, I realized that it is not necessary for the JSON deserialization use case I was targeting. It can be incorporated in the future if the need arises.

AuHau · 2024-02-19T11:55:32Z

codex/sales/reservations.nim

-  x.availabilityId == y.availabilityId and
-  x.size == y.size and
-  x.requestId == y.requestId and
-  x.slotIndex == y.slotIndex
+  x.id == y.id
 proc `==`*(x, y: Availability): bool =
-  x.id == y.id and
-  x.size == y.size and
-  x.duration == y.duration and
-  x.maxCollateral == y.maxCollateral and
-  x.minPrice == y.minPrice
+  x.id == y.id


@emizzle I have changed the equal compersion to use only the ID as IMHO that is what ID is for. Do you think it might break something?

I think that should be ok. I'm not entirely sure if I remember correctly, but I think maybe ids didn't exist when Availability was first created, hence the need for comparing the properties.

I have a very vague recollection of just an id comparison not being enough, but for the life of me, I can't remember why 😅 I guess we'll find out one way or the other 😂

AuHau · 2024-02-21T09:16:11Z

We should report the bug in presto

For sure, I already reported it: status-im/nim-presto#77

markspanbroek

Very nice! I like the way you addressed the PATCH functionality, very elegant.

codex/rest/json.nim

codex/sales/reservations.nim

codex/utils/options.nim

tests/codex/sales/testreservations.nim

tests/integration/testIntegration.nim

emizzle

Awesome job Adam, lots of great work here!

codex/rest/api.nim

codex/sales/reservations.nim

emizzle · 2024-03-15T06:09:06Z

codex/rest/api.nim

+          if error of NotExistsError:
+            return RestApiResponse.error(Http404, "Availability not found")
+          else:
+            return RestApiResponse.error(Http500, error.msg)


Since this is used in multiple places, could we extract this into a template for better readability? eg

Suggested change

if error of NotExistsError:

return RestApiResponse.error(Http404, "Availability not found")

else:

return RestApiResponse.error(Http500, error.msg)

template availabilityNotFound(error) =

if error of NotExistsError:

RestApiResponse.error(Http404, "Availability not found")

else:

RestApiResponse.error(Http500, error.msg)

return availabilityNotFound(error)

codex/rest/api.nim

emizzle · 2024-03-15T06:12:38Z

codex/rest/api.nim

+      except CatchableError as exc:
+        trace "Excepting processing request", exc = exc.msg
+        return RestApiResponse.error(Http500)
+


Could also do a templated block instead of wrapping it all in try/catch. eg

template onError(body) = try: body except CatchableError as exc: trace "Excepting processing request", exc = exc.msg return RestApiResponse.error(Http500) onError: without contracts =? node.contracts.host: return RestApiResponse.error(Http503, "Sales unavailable") ....

emizzle · 2024-03-15T06:14:15Z

codex/sales/reservations.nim

-  x.availabilityId == y.availabilityId and
-  x.size == y.size and
-  x.requestId == y.requestId and
-  x.slotIndex == y.slotIndex
+  x.id == y.id
 proc `==`*(x, y: Availability): bool =
-  x.id == y.id and
-  x.size == y.size and
-  x.duration == y.duration and
-  x.maxCollateral == y.maxCollateral and
-  x.minPrice == y.minPrice
+  x.id == y.id


I have a very vague recollection of just an id comparison not being enough, but for the life of me, I can't remember why 😅 I guess we'll find out one way or the other 😂

codex/sales/reservations.nim

tests/codex/sales/testreservations.nim

emizzle · 2024-03-17T23:04:25Z

tests/integration/codexclient.nim

+  let url = client.baseurl & "/sales/availability/" & $availabilityId
+  var json = %*{}
+
+  if totalSize =? totalSize:
+    json["totalSize"] = %totalSize
+
+  if freeSize =? freeSize:
+    json["freeSize"] = %freeSize
+
+  if duration =? duration:
+    json["duration"] = %duration
+
+  if minPrice =? minPrice:
+    json["minPrice"] = %minPrice
+
+  if maxCollateral =? maxCollateral:
+    json["maxCollateral"] = %maxCollateral
+
+  client.http.patch(url, $json)


Serializing an optionalized type will output null for none fields, which are handled correctly in deserialization on the api side.

Suggested change

let url = client.baseurl & "/sales/availability/" & $availabilityId

var json = %*{}

if totalSize =? totalSize:

json["totalSize"] = %totalSize

if freeSize =? freeSize:

json["freeSize"] = %freeSize

if duration =? duration:

json["duration"] = %duration

if minPrice =? minPrice:

json["minPrice"] = %minPrice

if maxCollateral =? maxCollateral:

json["maxCollateral"] = %maxCollateral

client.http.patch(url, $json)

type OptRestAvailability = Optionalize(RestAvailability)

let availability = OptRestAvailability(totalSize: totalSize, freeSize: freeSize, duration: duration, minPrice: minPrice, maxCollateral: maxCollateral)

client.http.patch(url, availability.toJson)

Another thought: we could also consider exporting this type from rest/json instead of creating it each time.

I was banging my head on why integration tests were not passing and it seemed like there were no changes updated on the node's side, while when I tested it locally it worked. It turned out that yours suggestion is actually serializing into empty JSON object {} 😳

It is because the Optionalize does not preserve the serialize pragmas. Honestly, I don't want to tinker with the macro anymore as that would be another time blackhole, so I will reject your proposal, even though I like it more than the original.

If/when we find the need to extend the Optionalize to honor serialize pragmas then we can revisit this part.

No problem, I understand. For future reference, you can also annotate the type with {.serialize.} and then there's no need to annotate each property, ie all properties will be serialized by default.

emizzle · 2024-03-17T23:09:06Z

tests/integration/testIntegration.nim

+  test "updating non-existing availability":
+    let availability = client1.postAvailability(totalSize=140000.u256, duration=200.u256, minPrice=300.u256, maxCollateral=300.u256).get
+
+    client1.patchAvailability(availability.id, duration=100.u256.some, minPrice=200.u256.some, maxCollateral=200.u256.some)
+
+    let updatedAvailability = (client1.getAvailabilities().get).findItem(availability).get
+    check updatedAvailability.duration == 100
+    check updatedAvailability.minPrice == 200
+    check updatedAvailability.maxCollateral == 200
+    check updatedAvailability.totalSize == 140000
+    check updatedAvailability.freeSize == 140000
+
+    let nonExistingResponse = client1.patchAvailabilityRaw(AvailabilityId.example, duration=100.u256.some, minPrice=200.u256.some, maxCollateral=200.u256.some)
+    check nonExistingResponse.status == "404 Not Found"


Imo, all of these Availability integration tests should probably be handled by REST unit tests (which don't exist yet), instead of an integration test. Definitely out of scope of this PR obviously, but something we should look to implement in the future

AuHau · 2024-03-21T09:46:36Z

codex/sales/reservations.nim

+  let getResult = await self.get(key, Availability)
+
+  if getResult.isOk:
+    let oldAvailability = !getResult
+
+    # Sizing of the availability changed, we need to adjust the repo reservation accordingly
+    if oldAvailability.totalSize != obj.totalSize:
+      if oldAvailability.totalSize < obj.totalSize: # storage added
+        if reserveErr =? (await self.repo.reserve((obj.totalSize - oldAvailability.totalSize).truncate(uint))).errorOption:
+          return failure(reserveErr.toErr(ReserveFailedError))
+
+      elif oldAvailability.totalSize > obj.totalSize: # storage removed
+        if reserveErr =? (await self.repo.release((oldAvailability.totalSize - obj.totalSize).truncate(uint))).errorOption:
+          return failure(reserveErr.toErr(ReleaseFailedError))
+  else:
+    let err = getResult.error()
+    if not (err of NotExistsError):
+      return failure(err)


I know this is not the nicest construct, but I have not figured a way how to achieve this using some nice construct from Questionable, because I need to continue the execution of the proc in case when the error is NotExistsError and in the same time not to evaluate the sizing changes...

If you know of better way please let me know.

without availability =? await self.get, error: # do something with error # do something with availability

Hmmm, I know about this construct, but did not think of a way how I could wrap it on my use case. But you (again 😅) gave me idea :-D Thanks! Will fix it in follow-up PR.

AuHau · 2024-03-21T13:05:58Z

Oh shi*, I have no idea how did I add this to the merge queue 😳 Please review it anyhow and if there are some outstanding issues, I will tackle them in follow up PR.

codex/rest/api.nim

emizzle · 2024-03-25T03:13:13Z

codex/utils/options.nim

+  let tSym = genSym(nskType, "T")
+  nnkStmtList.newTree(
+      nnkTypeSection.newTree(
+        nnkTypeDef.newTree(tSym, newEmptyNode(), nnkObjectTy.newTree(newEmptyNode(), newEmptyNode(), fields))
+      ),
+      tSym
+  )


+proc addSerializePragma(typeNode: NimNode): NimNode = + nnkPragmaExpr.newTree( + typeNode, + nnkPragma.newTree( + newIdentNode("serialize") + ) + ) + let tSymAnnotated = tSym.addSerializePragma() let tree = nnkStmtList.newTree( nnkTypeSection.newTree( - nnkTypeDef.newTree(tSym, newEmptyNode(), nnkObjectTy.newTree(newEmptyNode(), newEmptyNode(), fields)) + nnkTypeDef.newTree(tSymAnnotated, newEmptyNode(), nnkObjectTy.newTree(newEmptyNode(), newEmptyNode(), fields)) ), tSym )

Nice! Did you test it? Does it work? Would you be willing to create PR for it?
IMHO I would just not be adding the pragma automatically to all the fields. Either, it should be smart enough to detect that the pragma was present on the original field and only then add it to the new type, or a simpler approach could be adding some flag to the Optionalize() which would enable this behavior.

AuHau force-pushed the feat/persistent-availabilities branch 2 times, most recently from 0044060 to b4e5e9a Compare February 14, 2024 11:54

AuHau force-pushed the feat/persistent-availabilities branch from 65f3061 to 0706677 Compare February 20, 2024 13:06

AuHau requested review from emizzle and markspanbroek February 20, 2024 15:14

AuHau marked this pull request as ready for review February 20, 2024 15:15

AuHau commented Feb 21, 2024

View reviewed changes

markspanbroek approved these changes Mar 12, 2024

View reviewed changes

AuHau force-pushed the feat/persistent-availabilities branch from bea520a to e32106b Compare March 13, 2024 12:41

AuHau enabled auto-merge March 13, 2024 12:41

emizzle reviewed Mar 15, 2024

View reviewed changes

emizzle reviewed Mar 17, 2024

View reviewed changes

AuHau added 6 commits March 20, 2024 16:50

feat(api): availabilities patch and reservations endpoints

cc8b91e

test: fixing tests and writing

bb93fbe

test: test reservations endpoint

4951241

chore: feedback implementation

2366ed3

chore: feedback implementation

294286c

test: fix integration tests

f928b2b

AuHau force-pushed the feat/persistent-availabilities branch from 82b5439 to f928b2b Compare March 21, 2024 09:40

AuHau commented Mar 21, 2024

View reviewed changes

AuHau requested review from emizzle and markspanbroek March 21, 2024 09:49

AuHau added this pull request to the merge queue Mar 21, 2024

Merged via the queue into master with commit de1714e Mar 21, 2024
12 checks passed

AuHau deleted the feat/persistent-availabilities branch March 21, 2024 11:55

AuHau mentioned this pull request Mar 21, 2024

refactor: cleanup patch availabilities #748

Draft

emizzle reviewed Mar 22, 2024

View reviewed changes

codex/rest/api.nim Show resolved Hide resolved

emizzle reviewed Mar 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(api): availabilities patch endpoint #692

feat(api): availabilities patch endpoint #692

AuHau commented Feb 1, 2024 •

edited

dryajov commented Feb 20, 2024

AuHau Feb 14, 2024

AuHau Feb 14, 2024

AuHau Feb 19, 2024

emizzle Mar 13, 2024

emizzle Mar 15, 2024 •

edited

AuHau commented Feb 21, 2024

markspanbroek left a comment

emizzle left a comment

emizzle Mar 15, 2024 •

edited

emizzle Mar 15, 2024

emizzle Mar 15, 2024 •

edited

emizzle Mar 17, 2024

AuHau Mar 21, 2024

emizzle Mar 21, 2024 •

edited

emizzle Mar 17, 2024 •

edited

AuHau Mar 21, 2024

emizzle Mar 21, 2024

AuHau Mar 21, 2024

AuHau commented Mar 21, 2024

emizzle Mar 25, 2024

AuHau Mar 26, 2024

		totalSize* {.serialize.}: UInt256
		freeSize* {.serialize.}: UInt256

feat(api): availabilities patch endpoint #692

feat(api): availabilities patch endpoint #692

Conversation

AuHau commented Feb 1, 2024 • edited

dryajov commented Feb 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emizzle Mar 15, 2024 • edited

Choose a reason for hiding this comment

AuHau commented Feb 21, 2024

markspanbroek left a comment

Choose a reason for hiding this comment

emizzle left a comment

Choose a reason for hiding this comment

emizzle Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emizzle Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emizzle Mar 21, 2024 • edited

Choose a reason for hiding this comment

emizzle Mar 17, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AuHau commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AuHau commented Feb 1, 2024 •

edited

emizzle Mar 15, 2024 •

edited

emizzle Mar 15, 2024 •

edited

emizzle Mar 15, 2024 •

edited

emizzle Mar 21, 2024 •

edited

emizzle Mar 17, 2024 •

edited