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

Indexed parameters of events: types are wrong/Do we need it at all? #1045

Closed
rin-st opened this issue Jan 28, 2025 · 2 comments · Fixed by #1048
Closed

Indexed parameters of events: types are wrong/Do we need it at all? #1045

rin-st opened this issue Jan 28, 2025 · 2 comments · Fixed by #1048
Assignees

Comments

@rin-st
Copy link
Member

rin-st commented Jan 28, 2025

In #540 we added indexed args to events. But types for indexed arguments are still not supported (or it broke someday)

How to reproduce:

app/page.tsx code:

Details

"use client";

import Link from "next/link";
import type { NextPage } from "next";
import { useAccount } from "wagmi";
import { BugAntIcon, MagnifyingGlassIcon } from "@heroicons/react/24/outline";
import { Address } from "~~/components/scaffold-eth";
import { useScaffoldEventHistory } from "~~/hooks/scaffold-eth";

const Home: NextPage = () => {
  const { address: connectedAddress } = useAccount();
  const { data: greetingChangeEvents } = useScaffoldEventHistory({
    contractName: "YourContract",
    eventName: "GreetingChange",
    fromBlock: 0n,
  });

  return (
    <>
      <div className="flex items-center flex-col flex-grow pt-10">
        <div className="px-5">
          <h1 className="text-center">
            <span className="block text-2xl mb-2">Welcome to</span>
            <span className="block text-4xl font-bold">Scaffold-ETH 2</span>
          </h1>
          <div className="flex justify-center items-center space-x-2 flex-col sm:flex-row">
            <p className="my-2 font-medium">Connected Address:</p>
            <Address address={connectedAddress} />
          </div>
          <p className="text-center text-lg">
            Get started by editing{" "}
            <code className="italic bg-base-300 text-base font-bold max-w-full break-words break-all inline-block">
              packages/nextjs/app/page.tsx
            </code>
          </p>
          <p className="text-center text-lg">
            Edit your smart contract{" "}
            <code className="italic bg-base-300 text-base font-bold max-w-full break-words break-all inline-block">
              YourContract.sol
            </code>{" "}
            in{" "}
            <code className="italic bg-base-300 text-base font-bold max-w-full break-words break-all inline-block">
              packages/hardhat/contracts
            </code>
          </p>
        </div>

        {greetingChangeEvents?.map((event, index) => {
          return (
            <tr key={index} className="flex">
              <td>
                <Address address={event.args?.[0]} />
              </td>
              <td className="w-12 text-center">{event.args?.[1]}</td>
              <td className="w-12 text-center">{event.args?.[2]?.toString()}</td>
            </tr>
          );
        })}

        <div className="flex-grow bg-base-300 w-full mt-16 px-8 py-12">
          <div className="flex justify-center items-center gap-12 flex-col sm:flex-row">
            <div className="flex flex-col bg-base-100 px-10 py-10 text-center items-center max-w-xs rounded-3xl">
              <BugAntIcon className="h-8 w-8 fill-secondary" />
              <p>
                Tinker with your smart contract using the{" "}
                <Link href="/debug" passHref className="link">
                  Debug Contracts
                </Link>{" "}
                tab.
              </p>
            </div>
            <div className="flex flex-col bg-base-100 px-10 py-10 text-center items-center max-w-xs rounded-3xl">
              <MagnifyingGlassIcon className="h-8 w-8 fill-secondary" />
              <p>
                Explore your local transactions with the{" "}
                <Link href="/blockexplorer" passHref className="link">
                  Block Explorer
                </Link>{" "}
                tab.
              </p>
            </div>
          </div>
        </div>
      </div>
    </>
  );
};

export default Home;

  1. Change greeting from debug page
  2. You can see events listed on the main page as expected, but if you check this block on your IDE you see that types are wrong
Image

I couldn't find a fast solution for this and then I thought:
Why do we even need that workaround to get Ethers.js-like behavior in Wagmi/Viem? In general, it only adds a mess when working with events. For me, if event arguments are named, then get it like event.args?.newGreeting. If your arguments are unnamed, get it like event.args?.[0]. But when you can get the same event arguments both ways, it only adds ambiguity to codebase.

Again, Explicit is better than implicit. So I think it's better just to remove those additional indexed arguments.

What do you think?

@technophile-04
Copy link
Collaborator

Again, Explicit is better than implicit. So I think it's better just to remove those additional indexed arguments.

++ I think this makes a lot of sense and we should remove it! I think the reason we always sugarcoat with indexed params is because of this : scaffold-eth/se-2-challenges#93

^ The above could be solved by updating the challenge readme and asking people to be mindful about args name + also maybe mentioning this behaviour in SE-2 docs?

@rin-st rin-st self-assigned this Feb 6, 2025
@rin-st
Copy link
Member Author

rin-st commented Feb 6, 2025

++ I think this makes a lot of sense and we should remove it! I think the reason we always sugarcoat with indexed params is because of this : scaffold-eth/se-2-challenges#93

Yes, it worked for some time but now people get that type error for example if they use named arguments in challenge 1 (I think cursor add it most of the time).

^ The above could be solved by updating the challenge readme and asking people to be mindful about args name + also maybe mentioning this behaviour in SE-2 docs?

Yes, I'll fix it, thanks!

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 a pull request may close this issue.

2 participants