alex-grover / neynar-next

Create Farcaster apps with Next.js and Neynar
MIT License
13 stars 2 forks source link

πŸ› Bug Report: getCastsInThread does not return type Cast[] #33

Closed connormcmk closed 1 year ago

connormcmk commented 1 year ago

Describe the bug The return type of getCastsInThread is:

Promise<{
    result: {
        casts: Cast[];
    };
}>

However, the actual internal values of result.casts is

{
  "hash": "0x90a9b8eb759fa1bcab4479455e5bb46e8f9fac10",
  "parentHash": "0x7865cc2aa89a96557363c5d6fa95a16282ac5251",
  "parentUrl": null,
  "parentAuthor": {
    "fid": "2588"
  },
  "author": {
    "fid": 2588,
    "custodyAddress": "0x00275fdd516e5a5b9a017e5181f375aaf2751302",
    "username": "nor",
    "displayName": "Connor McCormick πŸ₯/🫦",
    "pfp": {
      "url": "https://i.imgur.com/1JjZFIB.png"
    },
    "profile": {
      "bio": {
        "text": "The only problem is externality pricing. Follower",
        "mentions": []
      }
    },
    "followerCount": 6459,
    "followingCount": 1590,
    "verifications": [
      "0x4f8c531df3d97c6cd437ac8dfe756975445d1161"
    ],
    "activeStatus": "active"
  },
  "text": "nah\nhttps://warpcast.com/nor/0xb0f47f",
  "timestamp": "2023-10-08T12:30:35.000Z",
  "embeds": [
    {
      "url": "https://warpcast.com/nor/0xb0f47f"
    }
  ],
  "reactions": {
    "count": 0,
    "fids": []
  },
  "recasts": {
    "count": 0,
    "fids": []
  },
  "recasters": [],
  "replies": {
    "count": 2
  },
  "threadHash": null
}

What's expected instead is a structure matching the Cast type, like this:

{
  "hash": "0x6f7792e8e224b6707f2fa16dc71cc618eb98f995",
  "thread_hash": null,
  "parent_hash": null,
  "parent_url": "https://option.app",
  "parent_author": {
    "fid": null
  },
  "author": {
    "fid": 2588,
    "custody_address": "0x00275fdd516e5a5b9a017e5181f375aaf2751302",
    "username": "nor",
    "display_name": "Connor McCormick πŸ₯/🫦",
    "pfp_url": "https://i.imgur.com/1JjZFIB.png",
    "profile": {
      "bio": {
        "text": "The only problem is externality pricing. Follower",
        "mentions": []
      }
    },
    "follower_count": 6459,
    "following_count": 1590,
    "verifications": [
      "0x4f8c531df3d97c6cd437ac8dfe756975445d1161"
    ],
    "active_status": "active"
  },
  "text": "It's impossible to build AGI.",
  "timestamp": "2023-10-12T13:45:19.000Z",
  "embeds": [],
  "reactions": {
    "likes": [],
    "recasts": []
  },
  "replies": {
    "count": 0
  }
}
connormcmk commented 1 year ago

After further inspection, it seems the issue might be coming from the data structures being different in the responses from v1 and v2 farcaster

With an `all-casts-in-thread` query: ```javascript import axios from 'axios'; const options = { method: 'GET', url: 'https://api.neynar.com/v1/farcaster/all-casts-in-thread', params: { api_key: 'NEYNAR_API_DOCS', threadHash: '0xfe90f9de682273e05b201629ad2338bdcd89b6be', viewerFid: '194' }, headers: {accept: 'text/plain'} }; ``` It returns: ```json { "result": { "casts": [ ... { "hash": "0xfe90f9de682273e05b201629ad2338bdcd89b6be", "parentHash": null, "parentUrl": null, "parentAuthor": { "fid": null }, "author": { "fid": 358, "custodyAddress": "0x3bcadeaef7f8e3bf5ec9ca5212cc9c72556c43ed", "username": "corbin.eth", "displayName": "Corbin Page", "pfp": { "url": "https://res.cloudinary.com/merkle-manufactory/image/fetch/c_fill,f_png,w_256/https://lh3.googleusercontent.com/szIk2U62Zfaux7eK8tinvy9vCUz2EPDUYet8WDKN9_dCJmm2-JM8Fux7_Cy2ZWzE9h2g3dIL9j_ywn8iK_UZYB0sToZ1dcP0QBsmh2w" }, "profile": { "bio": { "text": "πŸ—οΈ web3 builder\n 🩹 patchwallet.com\n 🧹 dustsweeper.xyz\n 🌐 nf.td/corbin", "mentions": [] } }, "followerCount": 5952, "followingCount": 318, "verifications": [ "0x869ec00fa1dc112917c781942cc01c68521c415e" ], "activeStatus": "active" }, "text": "Banger of a podcast with @pmarca on Rogan!\n\n* Best framing of the dangers of an AI regulatory moat, comparing to Dodd Frank\n* Examples of AI for kids/future gens \n* Cults aren’t all bad \n\nhttps://open.spotify.com/episode/3EWIFUOpaM9s7Pmei0r2C3?si=0xedN1QfTiuMZLAY0cBxlA&context=spotify%3Ashow%3A4rOoJ6Egrf8K2IrywzwOMk", "timestamp": "2023-07-20T00:03:23.000Z", "embeds": [ { "url": "https://open.spotify.com/episode/3EWIFUOpaM9s7Pmei0r2C3?si=0xedN1QfTiuMZLAY0cBxlA&context=spotify%3Ashow%3A4rOoJ6Egrf8K2IrywzwOMk" } ], "reactions": { "count": 5, "fids": [ 124, 8470, 1600, 616, 3 ] }, "recasts": { "count": 1, "fids": [ 714 ] }, "recasters": [ "ryanrodenbaugh" ], "viewerContext": { "liked": false, "recasted": false }, "replies": { "count": 2 }, "threadHash": null }, ... ] } } ```
Whereas with v2 return cast: ```javascript const options = { method: 'GET', headers: {accept: 'application/json', api_key: 'NEYNAR_API_DOCS'} }; fetch('https://api.neynar.com/v2/farcaster/cast?type=url', options) .then(response => response.json()) .then(response => console.log(response)) .catch(err => console.error(err)); ``` it returns: ```json { "cast": { "hash": "0x4425780bf8bc68a6422d4cb440b9ca4ba5617c3f", "thread_hash": null, "parent_hash": "0x6663bea5d2b5fdb94682c47b2a5d4f496d22e3cc", "parent_url": null, "parent_author": { "fid": "2588" }, "author": { "fid": 302, "custody_address": "0xa4b7c9173ff5be5c116d2ff846d23dd12a3cc5ec", "username": "gt", "display_name": "Goksu Toprak", "pfp_url": "https://lh3.googleusercontent.com/gq7_KZeGTW4TRQvQx1QhYJvCX0A7AjW8i7VBc8d9vhtaB3xZRS51AEK5pIlb_dL4jpXQ75h_LwMLtf4Jrzuret7DegPfLedEh8mr", "profile": { "bio": { "text": "Working on Farcaster.", "mentions": [] } }, "follower_count": 6065, "following_count": 391, "verifications": [ "0x8ed9eee4fce8cbc92dc4aafbc36b5721eb66b15d" ], "active_status": "active" }, "text": "Go style spec has this right imo: https://google.github.io/styleguide/go/decisions#variable-names", "timestamp": "2023-10-21T18:57:06.000Z", "embeds": [], "reactions": { "likes": [ { "fid": 474 }, { "fid": 834 }, { "fid": 2588 } ], "recasts": [] }, "replies": { "count": 1 } } } ```
And with v1 return cast: ```javascript const options = { method: 'GET', headers: {accept: 'application/json', api_key: 'NEYNAR_API_DOCS'} }; fetch('https://api.neynar.com/v1/farcaster/cast?hash=0xfe90f9de682273e05b201629ad2338bdcd89b6be', options) .then(response => response.json()) .then(response => console.log(response)) .catch(err => console.error(err)); ``` It returns: ```json { "result": { "cast": { "hash": "0xfe90f9de682273e05b201629ad2338bdcd89b6be", "parentHash": null, "parentUrl": null, "parentAuthor": { "fid": null }, "author": { "fid": 358, "custodyAddress": "0x3bcadeaef7f8e3bf5ec9ca5212cc9c72556c43ed", "username": "corbin.eth", "displayName": "Corbin Page", "pfp": { "url": "https://res.cloudinary.com/merkle-manufactory/image/fetch/c_fill,f_png,w_256/https://lh3.googleusercontent.com/szIk2U62Zfaux7eK8tinvy9vCUz2EPDUYet8WDKN9_dCJmm2-JM8Fux7_Cy2ZWzE9h2g3dIL9j_ywn8iK_UZYB0sToZ1dcP0QBsmh2w" }, "profile": { "bio": { "text": "πŸ—οΈ web3 builder\n 🩹 patchwallet.com\n 🧹 dustsweeper.xyz\n 🌐 nf.td/corbin", "mentions": [] } }, "followerCount": 5952, "followingCount": 318, "verifications": [ "0x869ec00fa1dc112917c781942cc01c68521c415e" ], "activeStatus": "active" }, "text": "Banger of a podcast with @pmarca on Rogan!\n\n* Best framing of the dangers of an AI regulatory moat, comparing to Dodd Frank\n* Examples of AI for kids/future gens \n* Cults aren’t all bad \n\nhttps://open.spotify.com/episode/3EWIFUOpaM9s7Pmei0r2C3?si=0xedN1QfTiuMZLAY0cBxlA&context=spotify%3Ashow%3A4rOoJ6Egrf8K2IrywzwOMk", "timestamp": "2023-07-20T00:03:23.000Z", "embeds": [ { "url": "https://open.spotify.com/episode/3EWIFUOpaM9s7Pmei0r2C3?si=0xedN1QfTiuMZLAY0cBxlA&context=spotify%3Ashow%3A4rOoJ6Egrf8K2IrywzwOMk" } ], "reactions": { "count": 5, "fids": [ 124, 8470, 1600, 616, 3 ] }, "recasts": { "count": 1, "fids": [ 714 ] }, "recasters": [ "ryanrodenbaugh" ], "replies": { "count": 2 }, "threadHash": null } } } ```

You can see that for the v1 farcaster endpoint (of which all-casts-in-thread is a member) it returns:

    "reactions": {
      "likes": [
        { "fid": 474 },
        ...
      ],
      "recasts": [ 
        {"fid": 454}
      ]
    },

Whereas for v2 endpoint it returns:

"reactions": {
  "count": 5,
  "fids": [
    124,
    ...
  ]
},

It would probably be ideal to map these two return types to the same datastructure. I recommend using the v2 data structure for now:

type Cast = {
    hash: Hash;
    thread_hash: Hash | null;
    parent_hash: Hash | null;
    parent_url: string | null;
    parent_author: {
        fid: number | null;
    };
    author: {
        fid: number;
        username: string;
        display_name: string;
        pfp_url: string;
        profile: {
            bio: {
                text: string;
            };
        };
        follower_count: number;
        following_count: number;
        verifications: Hash[];
        active_status: 'active';
    };
    text: string;
    timestamp: string;
    embeds: Embed[];
    reactions: {
        likes: Like[];
        recasts: Recast[];
    };
    replies: {
        count: number;
    };
}
alex-grover commented 1 year ago

this is my oversight - forgot to implement a separate v1 Cast type. will get a fix out later today.

connormcmk commented 1 year ago

I think we should map it all to the v2 Cast type, handling multiple different types is annoying

Or have a v1 Cast type and a utility function that maps it to v2 / generic, but I think it's best to just have 1, what do you think?

alex-grover commented 1 year ago

yep that makes sense, it looks like the types are the same besides the key casing. will use the existing type!

alex-grover commented 1 year ago

the types actually don't match perfectly, the V1 casts have a different format for recasters (fname in a separate array vs with the FID). current thinking is to just omit the names from the returned type, but want to think it through a bit more and test as well so will not be able to get a release out today.

if you want to try to use the version I have going that's not fully finished, you can use the casts-in-thread-type branch!

connormcmk commented 1 year ago

@alex-grover Have you had a chance to look at this since then? Picking this back up now.

connormcmk commented 1 year ago

I think we should just map it from:

{
  "recasts": {
    "count": 5,
    "fids": [
      129,
      13978,
      14392,
      10311,
      880
    ]
  },
  "recasters": [
    "phil",
    "forogh",
    "shahroozraeisi",
    "aeon",
    "accountless.eth"
  ]
}

to:

{
  "recasts": [
    {
      "fid": 129,
      "fname": "phil"
    },
    {
      "fid": 13978,
      "fname": "forogh"
    },
    {
      "fid": 14392,
      "fname": "shahroozraeisi"
    },
    {
      "fid": 10311,
      "fname": "aeon"
    },
    {
      "fid": 880,
      "fname": "accountless.eth"
    }
  ]
}

Right?

Should be as simple as:

recasts.fids.forEach((fid, index) => {
    const fname = recasters[index];
    obj.recasts.push({ fid, fname });
});

The only perhaps dangerous assumption we really have to make is that the fnamse of recasters is always in the same order as the fids in recasts.fids.

Asked Manan in Neynar channel: https://warpcast.com/nor/0xa53f6d10

connormcmk commented 1 year ago

P.S. it also looks like parentHash in v1 should be mapped to parent_hash in the v2 Cast object, guessing there are a handful of more mappings to make so would be worth checking

connormcmk commented 1 year ago

Confirmed that they will be in the correct order:

image

https://warpcast.com/shreyas-chorge/0xcee92868

alex-grover commented 1 year ago

great, thanks for looking into that. have not picked back up yet but can have this released in a couple hours

alex-grover commented 1 year ago

P.S. it also looks like parentHash in v1 should be mapped to parent_hash in the v2 Cast object, guessing there are a handful of more mappings to make so would be worth checking

Can you elaborate what you mean here? Is this sufficient or is there something else I've missed? https://github.com/alex-grover/neynar-next/commit/ee8bdcfd28822eee4dde1d9a4b149c845dcd9903#diff-1113f47917f0bc0f7f54e7f179e6120af0ce81ecfb6e3d1d0942c65f69628ff3R120

alex-grover commented 1 year ago

can you try v0.2.8?

thanks again for the details here!

connormcmk commented 12 months ago

Can you elaborate what you mean here? Is this sufficient or is there something else I've missed? https://github.com/alex-grover/neynar-next/commit/ee8bdcfd28822eee4dde1d9a4b149c845dcd9903#diff-1113f47917f0bc0f7f54e7f179e6120af0ce81ecfb6e3d1d0942c65f69628ff3R120

Yeah, that looks perfect!

can you try v0.2.8?

thanks again for the details here!

Thank you! Will test and follow up this afternoon

connormcmk commented 12 months ago

Yep, it's working. The only issue is that it seems like Cast.author should be of type User, right now it has a bespoke definition, so the response from the getUser endpoint returns a different User than the author field in the getCast endpoint

I.e. switch from:

export type Cast = {
  hash: Hash
  thread_hash: Hash | null
  parent_hash: Hash | null
  parent_url: string | null
  parent_author: {
    fid: number | null
  }
  author: {
    fid: number
    username: string
    display_name: string
    pfp_url: string
    profile: {
      bio: {
        text: string
      }
    }
    follower_count: number
    following_count: number
    verifications: Address[]
    active_status: 'active' | 'inactive'
  }
  text: string
  timestamp: string
  embeds: Embed[]
  reactions: {
    likes: Like[]
    recasts: Recast[]
  }
  replies: {
    count: number
  }
}

To

export type Cast = {
  hash: Hash
  thread_hash: Hash | null
  parent_hash: Hash | null
  parent_url: string | null
  parent_author: {
    fid: number | null
  }
  author: User
  text: string
  timestamp: string
  embeds: Embed[]
  reactions: {
    likes: Like[]
    recasts: Recast[]
  }
  replies: {
    count: number
  }
}
connormcmk commented 12 months ago

If you'd like, let's open it as a new issue. Is this something that changed with this commit or did it already exist?

alex-grover commented 12 months ago

That already existed - same distinction of v1 vs v2 endpoints, the user endpoints are from v1. A guiding principle I had when writing this originally was to stick to exactly what the Neynar docs + APIs were returning to make it easy to match this library with what you see when running a test query.

I agree that having 2 separate types is annoying to deal with. Now that we've stepped away from that, I'm wondering if the best approach here is to just map the UserV1 types to the V2 types, or to return the V1 type to match the APIs and export a utility function for ease of use.

My worry with converting everything is that we'll run into more places where the data doesn't match - the APIs seem to have some bugs and design quirks so this seems pretty likely to me if we cover every single endpoint.

Thinking for now we can convert and just keep an eye out for mismatches down the line, what do you think? Hopefully this is somewhat solved with the upcoming OpenAPI spec. Either way, if you could open another issue that would be great πŸ™πŸ½

connormcmk commented 12 months ago

Cool, created a new one. https://github.com/alex-grover/neynar-next/issues/36

I agree that having 2 separate types is annoying to deal with. Now that we've stepped away from that, I'm wondering if the best approach here is to just map the UserV1 types to the V2 types, or to return the V1 type to match the APIs and export a utility function for ease of use.

Yeah I think this is exactly the right approach. The whole reason I picked up the package in the first place was because I assumed we'd all swarm on a single place to handle the subtleties of neynar's somewhat wonky api spec, and push updates in a single place as the API evolves.

Obviously, any api user can read the docs themselves if they want the direct api experience, so the real value here is mapping the complexities of the neynar api to a consistent set of types and having knowledge of edge cases so the user doesn't have to worry about them.

just my 2c wdyt?

alex-grover commented 12 months ago

yep that makes sense to me. will work on it today :)