Feat: price per byte #1078

marcinczenko · 2025-01-20T02:37:38Z

This PR is one of the PRs for issue #460. See #460 (comment).

Introduces the following naming changes:
- For StorageAsk:
  - pricePerBytePerSecond,
  - collateralPerByte
- For Availability:
  - minPricePerBytePerSecond
  - totalRemainingCollateral
  - maxCollateralPerByte (computed helper)
- StorageAsk helpers:
  - pricePerSlotPerSecond
  - pricePerSlot
  - collateralPerSlot
Fixes related computations and tests

emizzle

Great job Marcin! I've added a few suggestions.

I'll approve now, since there is nothing major blocking this, however it would be nice if the comments were responded to and/or addressed, in particular:

Fix for minPricePerBytePerSecond => collateralPerByte in the marketplacesuite
Add tests to testcancelled and testfinished that show the collateral passed into onCleanUp is correct.

codex/contracts/marketplace.nim

codex/sales/states/preparing.nim

codex/sales.nim

codex/sales/reservations.nim

codex/rest/api.nim

tests/codex/sales/testslotqueue.nim

tests/integration/marketplacesuite.nim

tests/integration/testmarketplace.nim

tests/codex/sales/states/testcancelled.nim

…> collateralPerByte

…icePerByte, and maxCollateralPerByte

…rnal interface

emizzle

Thanks for making the changes, Marcin.

emizzle · 2025-01-23T22:54:26Z

tests/codex/sales/testslotqueue.nim

+    check eventually (
+      onProcessSlotCalledWith ==
+      @[
+        (item3.requestId, item3.slotIndex),
+        (item2.requestId, item2.slotIndex),
+        (item1.requestId, item1.slotIndex),
+        (item0.requestId, item0.slotIndex),
+      ]
+    )


IIRC, i had issues with this passing consistently on slow CI systems due to popping of SlotQueueItems before the new ones are added. Hopefully this doesn't happen 👍

Oh, I see - so far it seems to be working ok. push is not async, so I thought it will not let it happen if we do not have any await in between. We could probably even use a helper that is already there proc push*(self: SlotQueue, items: seq[SlotQueueItem]): ?!void...

AuHau · 2025-01-23T15:08:19Z

codex/contracts/requests.nim

+func collateralPerSlot*(ask: StorageAsk): UInt256 =
+  ask.collateralPerByte * ask.slotSize


As we discussed yesterday, for the sake of explicitness, I would be cautious with this helper as the real collateral that the host has to pay is dependent on the slot's state.

I would suggest replacing this helper with a helper on the market abstraction that would return the correct collateral based on the slot's state. If I see it correctly, then this is used only at the filling state (except all the tests usages), where I exactly suggested replacing this in the review of Arnaud PR: #1083 (comment)

I am kind of fine with it, but could we maybe please handle it on a separate PR - or maybe on Arnoud's PR? I will not be able to work on in in the coming days I am behind with BitTorrent stuff...

Yeah, make sense - I am OK with that 👍

Thanks @AuHau!

codex/contracts/marketplace.nim

AuHau · 2025-01-23T15:15:38Z

openapi.yaml

          type: string
-          description: Maximum collateral user is willing to pay per filled Slot (in amount of tokens) as decimal string
+          description: Total remaining collateral (in amount of tokens) that can still be used for matching requests


"remaining" word kind of does not make sense in the context of POST, but I don't really have any idea how to improve it.

Yes, good catch, "remaining" should not be there. I introduced totalCollateral on the API level exactly for the reason you mention. On API you can only specify totalCollateral and not totalRemainingCollateral.

Now it is: description: Total collateral (in amount of tokens) that can be used for matching requests

marcinczenko self-assigned this Jan 20, 2025

marcinczenko marked this pull request as draft January 20, 2025 02:37

marcinczenko added the Marketplace See https://miro.com/app/board/uXjVNZ03E-c=/ for details label Jan 20, 2025

marcinczenko mentioned this pull request Jan 20, 2025

Availability minPrice should be per byte #460

Closed

marcinczenko force-pushed the feat/price-per-byte branch 3 times, most recently from dedbed5 to cde6676 Compare January 23, 2025 01:46

marcinczenko marked this pull request as ready for review January 23, 2025 03:24

marcinczenko requested review from benbierens, emizzle and AuHau January 23, 2025 03:25

emizzle approved these changes Jan 23, 2025

View reviewed changes

AuHau mentioned this pull request Jan 23, 2025

Expose missed proofs counter #1058

Open

marcinczenko added 13 commits January 23, 2025 19:27

openAPI: StorageRequestCreation: reward => pricePerByte, collateral =…

dcc08e7

…> collateralPerByte

purchasing: reward => pricePerByte, collateral => collateralPerByte

4ea3aa3

Updates availabilities and reservations to use totalCollateral, minPr…

cac313e

…icePerByte, and maxCollateralPerByte

Uses correct div operator when operating on UInt256

294c873

proposal updating totalCollateral in availability

bc6bc89

makes sure that reading currentCollateral happens before freeing slot

71c1849

Updates naming

6bfab05

fixes tests: unit and contracts

1b405bd

uses feat/price-per-byte branch for codex-contracts-eth

0ebd0c5

temporarily disables integration tests on CI

858d9cc

introduces high level <<totalCollateral>> property for a cleaner exte…

fa9bdac

…rnal interface

updates integration tests

13851c5

Applies review comments

befcace

marcinczenko force-pushed the feat/price-per-byte branch from cde6676 to befcace Compare January 23, 2025 18:41

emizzle approved these changes Jan 23, 2025

View reviewed changes

2-towns mentioned this pull request Jan 24, 2025

feat(marketplace): indicate that slot is being repaired when trying to download #1083

Merged

AuHau reviewed Jan 24, 2025

View reviewed changes

Updates description of totalCollateral in SalesAvailability

284f020

AuHau approved these changes Jan 24, 2025

View reviewed changes

updates codex-contracts-eth (price-per-byte)

dacb5be

AuHau approved these changes Jan 24, 2025

View reviewed changes

veaceslavdoina added this pull request to the merge queue Jan 24, 2025

Merged via the queue into master with commit 962fc1c Jan 24, 2025
20 checks passed

veaceslavdoina deleted the feat/price-per-byte branch January 24, 2025 18:32

emizzle mentioned this pull request Feb 11, 2025

maxCollateral should be linked to "per byte" instead of "per slot" #602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: price per byte #1078

Feat: price per byte #1078

marcinczenko commented Jan 20, 2025

emizzle left a comment •

edited by marcinczenko

Loading

emizzle left a comment

emizzle Jan 23, 2025

marcinczenko Jan 23, 2025 •

edited

Loading

AuHau Jan 23, 2025

marcinczenko Jan 24, 2025 •

edited

Loading

AuHau Jan 24, 2025

marcinczenko Jan 24, 2025

AuHau Jan 23, 2025

marcinczenko Jan 24, 2025

		func collateralPerSlot*(ask: StorageAsk): UInt256 =
		ask.collateralPerByte * ask.slotSize

Feat: price per byte #1078

Feat: price per byte #1078

Conversation

marcinczenko commented Jan 20, 2025

emizzle left a comment • edited by marcinczenko Loading

Choose a reason for hiding this comment

emizzle left a comment

Choose a reason for hiding this comment

emizzle Jan 23, 2025

Choose a reason for hiding this comment

marcinczenko Jan 23, 2025 • edited Loading

Choose a reason for hiding this comment

AuHau Jan 23, 2025

Choose a reason for hiding this comment

marcinczenko Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

AuHau Jan 24, 2025

Choose a reason for hiding this comment

marcinczenko Jan 24, 2025

Choose a reason for hiding this comment

AuHau Jan 23, 2025

Choose a reason for hiding this comment

marcinczenko Jan 24, 2025

Choose a reason for hiding this comment

emizzle left a comment •

edited by marcinczenko

Loading

marcinczenko Jan 23, 2025 •

edited

Loading

marcinczenko Jan 24, 2025 •

edited

Loading