feat: deal tracker spade sync cron #57

vasco-santos · 2023-10-25T14:12:58Z

Adds deal tracker spade sync cron. As previously decided, this implements a diff based approach where newest Spade Oracle Data is fetched, compared with previous dowloaded one, generating a diff for insertion in DynamoDB

Decisions:

did not add this cron into filecoin-api repo given it is implementation detail, while other CRONs are part of the receipt chain
did not worry with pagination of deals in query, as well as memory consumption of the lambda for MVP
- looking at options to read and decompress zst files while streamming, so that we could avoid double memory to put in a Map. However, this would mean parsing lines without JSON richness which would be quite painful to implement
- current runs with real file provided by Spade endpoint fit well under the configured memory point, so we are likely safe until implementing our own Oracle (and we have the fallback of increasing Lambda memory if needed as a temporary solution...)
for now did not bother implementing a batch insert given they are also quite limited in dynamoDB and cost the same. But might reconsider that as fast follow up

Notes:

added a mock server to serve oracle files compressed as spade provides them (aka json.zst)

Needs:

feat: deal tracker implementation #55
convert read pieceCidV1 to pieceCidV2 before storing

seed-deploy · 2023-10-25T14:27:53Z

View stack outputs

pr57-w3filecoin-ApiStack

Name	Value
AggregateApiCustomDomain	https://pr57.filecoin.web3.storage
AggregateApiEndpoint	https://1xudr299pg.execute-api.us-east-2.amazonaws.com
DealTrackerApiCustomDomain	https://pr57.tracker.web3.storage
DealTrackerApiEndpoint	https://xpz8f0z5sk.execute-api.us-east-2.amazonaws.com

pr57-w3filecoin-DataStack

Name	Value
AggregateTableName	aggregate-store
BufferBucketName	buffer-store-pr57-0
DealStoreTableName	deal-store
InclusionTableName	inclusion-store
PieceTableName	piece-store
SpadeOracleBucketName	spade-oracle-store-pr57-0

pr57-w3filecoin-DealTrackerStack
pr57-w3filecoin-ProcessorStack

seed-deploy · 2023-10-26T12:33:09Z

Stack outputs updated

pr57-w3filecoin-ApiStack

Name	Value
AggregateApiCustomDomain	https://pr57.filecoin.web3.storage
AggregateApiEndpoint	https://1xudr299pg.execute-api.us-east-2.amazonaws.com
DealTrackerApiCustomDomain	https://pr57.tracker.web3.storage
DealTrackerApiEndpoint	https://xpz8f0z5sk.execute-api.us-east-2.amazonaws.com

pr57-w3filecoin-DataStack

Name	Value
AggregateTableName	aggregate-store
BufferBucketName	buffer-store-pr57-0
DealTrackerDealStoreTableName	deal-tracker-deal-store
InclusionTableName	inclusion-store
PieceTableName	piece-store
SpadeOracleBucketName	deal-tracker-spade-oracle-store-pr57-0

pr57-w3filecoin-DealTrackerStack
pr57-w3filecoin-ProcessorStack

alanshaw · 2023-10-26T14:16:51Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+      .pipe(ZSTDDecompress())
+  )
+  /** @type {SpadeOracle} */
+  const SpadeOracle = JSON.parse(toString(resDecompressed))


According to the readme you can just call resDecompressed.toString()

https://www.npmjs.com/package/stream-read-all

alanshaw · 2023-10-26T14:23:58Z

packages/core/src/store/spade-oracle-store.js

+      }
+    },
+    has: async (key) => {
+      const putCmd = new GetObjectCommand({


Suggested change

const putCmd = new GetObjectCommand({

const headCmd = new HeadObjectCommand({

alanshaw · 2023-10-26T14:29:17Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+  }
+
+  // Get updated spade oracle contracts
+  const getUpdatedSpadeOracle = await getSpadeOracleCurrentState(spadeOracleUrl)


Perhaps do this first, so we don't hold all the other state in memory for the duration. I assume this will take longer/be more likely to fail.

Can we swap "spade oracle state" for "deal archive"? So this would be for example:

Suggested change

const getUpdatedSpadeOracle = await getSpadeOracleCurrentState(spadeOracleUrl)

const { ok: nextArchive, error } = await fetchDealArchive(spadeOracleUrl)

alanshaw · 2023-10-26T14:34:42Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+
+  // Get diff of contracts
+  const diffOracleContracts = computeDiffOracleState({
+    // fallsback to empty map if not found


You can probably skip computing diff altogether if not found.

alanshaw · 2023-10-26T15:11:33Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+ */
+export async function spadeOracleSyncTick ({
+  dealStore,
+  spadeOracleStore,


We should name after what is being stored. So I'd call this dealArchiveStore or something.

alanshaw · 2023-10-26T15:15:03Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+  }
+
+  return {
+    ok: new Map(Object.entries(decode(getRes.ok.value))),


Why doesn't the store decode do this?

alanshaw · 2023-10-26T15:28:18Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+  spadeOracleUrl
+}) {
+  // Get previous recorded spade oracle contracts
+  const getPreviousSpadeOracle = await getSpadeOracleState({


We're fetching the current deal archive from our store. Until it is replaced, it is current. Previous sounds like it's the archive that's 1 before current.

Ideally this would just be const { ok: currentArchive } = dealArchiveStore.get().

Yes, I will rename to current! Thanks

we have spade now, but we could have other stores, but changing to dealArchiveStore we should keep it with explicit key for spade.

alanshaw · 2023-10-26T15:29:43Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+  }
+
+  // Get diff of contracts
+  const diffOracleContracts = computeDiffOracleState({


Suggested change

const diffOracleContracts = computeDiffOracleState({

const diff = computeDiff(currentArchive, nextArchive)

alanshaw · 2023-10-26T15:32:52Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+          // @ts-expect-error not PieceCIDv2
+          piece: legacyPieceCid,
+          provider: `${contract.provider}`,
+          insertedAt: (new Date()).toISOString()


Can we track insertedAt and updatedAt?

alanshaw · 2023-10-26T15:42:54Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+      .pipe(ZSTDDecompress())
+  )
+  /** @type {SpadeOracle} */
+  const SpadeOracle = JSON.parse(toString(resDecompressed))


Suggested change

const SpadeOracle = JSON.parse(toString(resDecompressed))

const rawData = JSON.parse(toString(resDecompressed))

Gozala · 2023-10-26T16:59:12Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+  const piece = Piece.fromInfo({
+    link,
+    size: Piece.Size.fromHeight(height)
+  })


You already have a height and root is link.multihash.digest so you could alse use Piece.view and skip whole info creation piece, that said you would have to do something like

Suggested change

const piece = Piece.fromInfo({

link,

size: Piece.Size.fromHeight(height)

})

const piece = Piece.toView({

root: link.multihash.digest,

height,

padding: 0n

})

If we do know original CAR size however it would be great to compute padding also instead of just putting 0n however.

I think we need the size here and not height: https://github.com/web3-storage/data-segment/blob/main/src/piece.js#L104

This is the aggregate piece CID that Spade Oracle has, while tracking Deals (uses the old Piece Cid v1 + height)

I think we need the size here and not height: https://github.com/web3-storage/data-segment/blob/main/src/piece.js#L104

Sorry I had typo I meant Piece.toView function in the suggestion, which takes {height, root, padding} triple.

This is the aggregate piece CID that Spade Oracle has, while tracking Deals (uses the old Piece Cid v1 + height)

Ah ok, aggregates don't really have padding so that makes sense. I actually wanted to use different CIDs for aggregates, but also don't really want to argue for them in the spec.

cool thanks, changed to suggestion

seed-deploy · 2023-10-26T17:28:42Z

Stack outputs updated

pr57-w3filecoin-ApiStack

Name	Value
AggregateApiCustomDomain	https://pr57.filecoin.web3.storage
AggregateApiEndpoint	https://1xudr299pg.execute-api.us-east-2.amazonaws.com
DealTrackerApiCustomDomain	https://pr57.tracker.web3.storage
DealTrackerApiEndpoint	https://xpz8f0z5sk.execute-api.us-east-2.amazonaws.com

pr57-w3filecoin-DataStack

Name	Value
AggregateTableName	aggregate-store
BufferBucketName	buffer-store-pr57-0
DealTrackerDealArchiveBucketName	deal-tracker-deal-archive-store-pr57-0
DealTrackerDealStoreTableName	deal-tracker-deal-store
InclusionTableName	inclusion-store
PieceTableName	piece-store

pr57-w3filecoin-DealTrackerStack
pr57-w3filecoin-ProcessorStack

seed-deploy · 2023-10-26T17:34:41Z

Stack outputs updated

pr57-w3filecoin-ApiStack

Name	Value
AggregateApiCustomDomain	https://pr57.filecoin.web3.storage
AggregateApiEndpoint	https://1xudr299pg.execute-api.us-east-2.amazonaws.com
DealerApiCustomDomain	Set HOSTED_ZONE in env to deploy to a custom domain
DealerApiEndpoint	https://lcv5jtulq0.execute-api.us-east-2.amazonaws.com
DealTrackerApiCustomDomain	https://pr57.tracker.web3.storage
DealTrackerApiEndpoint	https://xpz8f0z5sk.execute-api.us-east-2.amazonaws.com

pr57-w3filecoin-DataStack

Name	Value
AggregateTableName	aggregate-store
BufferBucketName	buffer-store-pr57-0
DealTrackerDealArchiveBucketName	deal-tracker-deal-archive-store-pr57-0
DealTrackerDealStoreTableName	deal-tracker-deal-store
InclusionTableName	inclusion-store
PieceTableName	piece-store

pr57-w3filecoin-DealTrackerStack
pr57-w3filecoin-ProcessorStack

Gozala · 2023-10-26T19:38:42Z

Looks like @alanshaw has already reviewed this, I'll let him do and focus on other things unless you ask me otherise.

vasco-santos · 2023-10-26T20:28:22Z

@alanshaw skipped old tests for now given they were timing out sometimes. We will integrate as follow up the aggregator-api new tests, and I will get rid of this anyway then

alanshaw · 2023-10-30T10:17:50Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+    diffPieceContracts = fetchLatestDealArchiveRes.ok
+  } else {
+    diffPieceContracts = computeDiff({
+      // falls back to empty map if not found


Suggested change

// falls back to empty map if not found

alanshaw · 2023-10-30T10:37:28Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+        return dealStore.put({
+          ...contract,
+          // @ts-expect-error not PieceCIDv2
+          piece: legacyPieceCid,
+          provider: `${contract.provider}`,
+          insertedAt: (new Date()).toISOString(),
+          updatedAt: (new Date()).toISOString()
+        })


Just so we don't get ever so slightly different dates...and can tell if something was ever updated.

Suggested change

return dealStore.put({

...contract,

// @ts-expect-error not PieceCIDv2

piece: legacyPieceCid,

provider: `${contract.provider}`,

insertedAt: (new Date()).toISOString(),

updatedAt: (new Date()).toISOString()

})

const insertedAt = new Date().toISOString()

return dealStore.put({

...contract,

// @ts-expect-error not PieceCIDv2

piece: legacyPieceCid,

provider: `${contract.provider}`,

insertedAt,

updatedAt: insertedAt

})

alanshaw · 2023-10-30T10:41:54Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+ */
+export async function putDiffToDealStore ({ dealStore, diffPieceContracts }) {
+  const res = await Promise.all(
+    Array.from(diffPieceContracts, ([key, value]) => {


It would be easier to understand if these were named after what they represent:

Suggested change

Array.from(diffPieceContracts, ([key, value]) => {

Array.from(diffPieceContracts, ([pieceCidStr, contracts]) => {

alanshaw · 2023-10-30T10:45:41Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+  for (const [pieceCid, contracts] of updatedPieceContracts.entries() ) {
+    const currentContracts = currentPieceContracts.get(pieceCid) || []
+    // Find diff when different length
+    if (contracts.length !== currentContracts.length) {


Could one contract have expired and another added? i.e. the number of contracts did not change but the contracts did. I would imagine you need to do the diff regardless.

not sure, but should not hurt removing this validation

alanshaw · 2023-10-30T10:48:13Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+    const piecCid = convertPieceCidV1toPieceCidV2(
+      parseLink(replica.piece_cid),
+      replica.piece_log2_size
+    )
+    dealMap.set(piecCid.toString(), replica.contracts.map(c => ({


Suggested change

const piecCid = convertPieceCidV1toPieceCidV2(

parseLink(replica.piece_cid),

replica.piece_log2_size

)

dealMap.set(piecCid.toString(), replica.contracts.map(c => ({

const pieceCid = convertPieceCidV1toPieceCidV2(

parseLink(replica.piece_cid),

replica.piece_log2_size

)

dealMap.set(pieceCid.toString(), replica.contracts.map(c => ({

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

alanshaw · 2023-10-30T11:07:23Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+  spadeOracleUrl
+}) {
+  // Get latest deal archive
+  const fetchLatestDealArchiveRes = await fetchLatestDealArchive(spadeOracleUrl)


Not blocking - I imagine there will be times where the archive has not changed - would it be possible to do a head request to get an etag and avoid pulling a dump with no differences?

Then the deal archive store is more like hash => data and we store a pointer to the latest and delete (or just expire) archives we no longer need.

I will create an issue to improve this and add a comment

alanshaw · 2023-10-30T11:09:14Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+async function putLatestDealArchive ({ dealArchiveStore, spadeOracleId, oracleContracts }) {
+  const putRes = await dealArchiveStore.put({
+    key: spadeOracleId,
+    value: encode(Object.fromEntries(oracleContracts))


Can we just store the data we retrieved and avoid re-encoding?

then we would need to encode/decode all CIDs given stores functions use CID. This was the reason to do the dag json.

alanshaw · 2023-10-30T11:10:43Z

packages/core/src/deal-tracker/spade-oracle-sync-tick.js

+  if (!res.ok) {
+    return {
+      // TODO: Error
+      error: new Error('could not read')


I typically just report the status code:

Suggested change

error: new Error('could not read')

error: new Error(`unexpected response status fetching deal archive: ${res.status}`)

seed-deploy bot temporarily deployed to pr57 October 25, 2023 14:13 Inactive

vasco-santos force-pushed the feat/deal-tracker-cron branch from 9c84fa3 to 1925c34 Compare October 25, 2023 14:28

seed-deploy bot temporarily deployed to pr57 October 25, 2023 14:28 Inactive

vasco-santos force-pushed the feat/deal-tracker-cron branch from 1925c34 to 2b70946 Compare October 26, 2023 08:22

seed-deploy bot had a problem deploying to pr57 October 26, 2023 08:23 Failure

vasco-santos force-pushed the feat/deal-tracker-cron branch from 2b70946 to e3b1164 Compare October 26, 2023 12:27

seed-deploy bot temporarily deployed to pr57 October 26, 2023 12:27 Inactive

vasco-santos force-pushed the feat/deal-tracker-cron branch from e3b1164 to 04b854a Compare October 26, 2023 13:21

seed-deploy bot had a problem deploying to pr57 October 26, 2023 13:21 Failure

vasco-santos marked this pull request as ready for review October 26, 2023 13:25

vasco-santos requested a review from alanshaw October 26, 2023 13:29

seed-deploy bot had a problem deploying to pr57 October 26, 2023 13:29 Failure

seed-deploy bot temporarily deployed to pr57 October 26, 2023 14:28 Inactive

alanshaw requested changes Oct 26, 2023

View reviewed changes

seed-deploy bot temporarily deployed to pr57 October 26, 2023 16:32 Inactive

Gozala reviewed Oct 26, 2023

View reviewed changes

seed-deploy bot temporarily deployed to pr57 October 26, 2023 17:22 Inactive

vasco-santos added 3 commits October 26, 2023 19:29

feat: deal tracker spade sync cron

d0bc60f

fix: convert piece cid v1 to piece cid v2

fad2241

chore: address review comments

e247249

vasco-santos force-pushed the feat/deal-tracker-cron branch from 5b74177 to e247249 Compare October 26, 2023 17:30

seed-deploy bot temporarily deployed to pr57 October 26, 2023 17:30 Inactive

seed-deploy bot temporarily deployed to pr57 October 26, 2023 18:03 Inactive

vasco-santos force-pushed the feat/deal-tracker-cron branch from d5e7a66 to 1d549ac Compare October 26, 2023 19:09

seed-deploy bot temporarily deployed to pr57 October 26, 2023 19:10 Inactive

chore: refactor piece v2 compute to use height

a97ef26

vasco-santos force-pushed the feat/deal-tracker-cron branch from 1d549ac to a97ef26 Compare October 26, 2023 19:25

seed-deploy bot temporarily deployed to pr57 October 26, 2023 19:25 Inactive

Gozala requested review from Gozala and removed request for Gozala October 26, 2023 19:37

chore: improve tests

399eac0

seed-deploy bot temporarily deployed to pr57 October 26, 2023 19:37 Inactive

Gozala approved these changes Oct 26, 2023

View reviewed changes

chore: improve tests

323e561

seed-deploy bot temporarily deployed to pr57 October 26, 2023 19:54 Inactive

chore: drop old tests

a85c68e

seed-deploy bot temporarily deployed to pr57 October 26, 2023 20:06 Inactive

vasco-santos requested a review from alanshaw October 26, 2023 20:28

alanshaw requested changes Oct 30, 2023

View reviewed changes

vasco-santos mentioned this pull request Oct 30, 2023

Spade oracle sync tick should head request to see if there is a difference #62

Open

seed-deploy bot temporarily deployed to pr57 October 30, 2023 14:32 Inactive

alanshaw approved these changes Oct 30, 2023

View reviewed changes

fix: address review

a0c30c9

vasco-santos force-pushed the feat/deal-tracker-cron branch from 5be1e32 to a0c30c9 Compare October 30, 2023 14:45

seed-deploy bot temporarily deployed to pr57 October 30, 2023 14:45 Inactive

vasco-santos merged commit fee55b1 into main Oct 30, 2023
3 checks passed

vasco-santos deleted the feat/deal-tracker-cron branch October 30, 2023 14:51

	const putCmd = new GetObjectCommand({
	const headCmd = new HeadObjectCommand({

	const getUpdatedSpadeOracle = await getSpadeOracleCurrentState(spadeOracleUrl)
	const { ok: nextArchive, error } = await fetchDealArchive(spadeOracleUrl)

	const diffOracleContracts = computeDiffOracleState({
	const diff = computeDiff(currentArchive, nextArchive)

	const SpadeOracle = JSON.parse(toString(resDecompressed))
	const rawData = JSON.parse(toString(resDecompressed))

-  const piece = Piece.fromInfo({
-    link,
-    size: Piece.Size.fromHeight(height)
-  })
+  const piece = Piece.toView({
+    root: link.multihash.digest,
+    height,
+    padding: 0n
+  })

	Array.from(diffPieceContracts, ([key, value]) => {
	Array.from(diffPieceContracts, ([pieceCidStr, contracts]) => {

	error: new Error('could not read')
	error: new Error(`unexpected response status fetching deal archive: ${res.status}`)

feat: deal tracker spade sync cron #57

feat: deal tracker spade sync cron #57

Conversation

vasco-santos commented Oct 25, 2023 • edited Loading

seed-deploy bot commented Oct 25, 2023

seed-deploy bot commented Oct 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alanshaw Oct 26, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasco-santos Oct 26, 2023 • edited Loading

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

Gozala Oct 26, 2023 • edited Loading

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

Choose a reason for hiding this comment

seed-deploy bot commented Oct 26, 2023

seed-deploy bot commented Oct 26, 2023

Gozala commented Oct 26, 2023

vasco-santos commented Oct 26, 2023

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

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

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

vasco-santos commented Oct 25, 2023 •

edited

Loading

alanshaw Oct 26, 2023 •

edited

Loading

vasco-santos Oct 26, 2023 •

edited

Loading

Gozala Oct 26, 2023 •

edited

Loading