Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add SubmittableDeal type #95

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

NikolasHaimerl
Copy link
Contributor

We are currently not using a specific type when submitting deals through the deal observer backend.
In two recent discussions (Here and here), the need for a specific type for submittable deals has come up.

This PR suggests a new type for deals that are found to be submittable and used throughout the submitting process of the deal observer.
This solves the issue of the use of the any type when activating type checking and also delivers clarity on the type returned by various functions during the submitting process.

@NikolasHaimerl NikolasHaimerl marked this pull request as ready for review February 11, 2025 09:39
Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposal in this pull request is consistent with what we are doing elsewhere in this project, I don't see any obvious issues besides one minor comment below.

@@ -1,13 +1,17 @@
/** @import {PgPool, Queryable} from '@filecoin-station/deal-observer-db' */
/** @import { Static } from '@sinclair/typebox' */
/** @import { ActiveDealDbEntry, PayloadRetrievabilityStateType } from '@filecoin-station/deal-observer-db/lib/types.js' */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two imports seem unused; can we remove this line?

Suggested change
/** @import { ActiveDealDbEntry, PayloadRetrievabilityStateType } from '@filecoin-station/deal-observer-db/lib/types.js' */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would have expected for the linter to pick thisup.

@@ -45,7 +49,7 @@ export const findAndSubmitUnsubmittedDeals = async (pgPool, batchSize, submitDea
*
* @param {PgPool} pgPool
* @param {number} batchSize
* @returns {AsyncGenerator<Array>}
* @returns {AsyncGenerator<Array<Static<typeof SubmittableDeal>>, void, unknown>}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my understanding, why does this type need void, unknown?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great question, I missed this detail myself.

FWIW, here is how TypeScript defines the AsyncGenerator type:

interface AsyncGenerator<T = unknown, TReturn = any, TNext = any> extends AsyncIteratorObject<T, TReturn, TNext> {
    // NOTE: 'next' is defined using a tuple to ensure we report the correct assignability errors in all places.
    next(...[value]: [] | [TNext]): Promise<IteratorResult<T, TReturn>>;
    return(value: TReturn | PromiseLike<TReturn>): Promise<IteratorResult<T, TReturn>>;
    throw(e: any): Promise<IteratorResult<T, TReturn>>;
    [Symbol.asyncIterator](): AsyncGenerator<T, TReturn, TNext>;
}

@@ -82,7 +86,7 @@ const findUnsubmittedDeals = async function * (pgPool, batchSize) {
* Mark deals as submitted.
*
* @param {Queryable} pgPool
* @param {Array} eligibleDeals
* @param {Array<Static<typeof SubmittableDeal>>} eligibleDeals
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param {Array<Static<typeof SubmittableDeal>>} eligibleDeals
* @param {SubmittableDeal[]} eligibleDeals

I think we could simplify this type signature and make it more correct. I have tried to break down the previous type signature using Le Chat:

  1. typeof SubmittableDeal: This refers to the type of the SubmittableDeal class or object itself. If SubmittableDeal is a class, typeof SubmittableDeal would be the type of the class constructor.

  2. Static: The Static utility type is not a built-in TypeScript type, but it is often used in libraries to refer to the static side of a class. This means it refers to the properties and methods that are available on the class itself, rather than on instances of the class.

  3. Array<...>: This indicates that the type is an array. The type inside the angle brackets (<...>) specifies the type of the elements in the array.

I think we can safely remove Static<...> as we're not referring to a static side of this class. Did you have for goal to make this array immutable or read only? If that's the case it would be more adequate to use ReadonlyArray type.

As for difference between Array<SubmittableDeal> and Array<typeof SubmittableDeal> Le Chat says the following:

SubmittableDeal[] or Array<SubmittableDeal> refers to an array of instances of the SubmittableDeal class.
Array<typeof SubmittableDeal> refers to an array of the SubmittableDeal class constructors or static members.

Given this breakdown I think it would be more correct if we change type to SubmittableDeal[] or Array<SubmittableDeal>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want the use of the fields for a given class to be checked at compile time, which is why we are using the Static type. This was suggested by @bajtos in a PR a while back. I think it also applies here.

@@ -112,7 +116,7 @@ const markDealsAsSubmitted = async (pgPool, eligibleDeals) => {
*
* @param {string} sparkApiBaseURL
* @param {string} sparkApiToken
* @param {Array} deals
* @param {Array<Static<typeof SubmittableDeal>>} deals
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @param {Array<Static<typeof SubmittableDeal>>} deals
* @param {SubmittableDeal[]} deals

Same comment is applicable here.

@@ -45,7 +49,7 @@ export const findAndSubmitUnsubmittedDeals = async (pgPool, batchSize, submitDea
*
* @param {PgPool} pgPool
* @param {number} batchSize
* @returns {AsyncGenerator<Array>}
* @returns {AsyncGenerator<Array<Static<typeof SubmittableDeal>>, void, unknown>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @returns {AsyncGenerator<Array<Static<typeof SubmittableDeal>>, void, unknown>}
* @returns {AsyncGenerator<SubmittableDeal[], void, unknown>}

I think same comment is applicable here.

Copy link
Contributor

@pyropy pyropy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can improve type signature to make it simple and more correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants