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

multicallClaim, not assigning eventPassNft #290

Open
sebpalluel opened this issue Apr 1, 2024 · 0 comments
Open

multicallClaim, not assigning eventPassNft #290

sebpalluel opened this issue Apr 1, 2024 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@sebpalluel
Copy link
Contributor

sebpalluel commented Apr 1, 2024

  • The multicallClaim function is not assigning the eventPassNft to new owners. Use registerOwnership to assign them and update the existing test to check that.
  • It's taking the first order to get the contractAddress. What happen if the following orders doesn't have the same contractAddress ? Same for eventPassId ? This function should be renamed to claimOrdersForEventPassId, it should check that all the orders are with the same contractAddress and eventPassId since otherwise it's breaking. It should preferably take also as arguments directly the contractAddress and eventPassId.
  • The integration test is far too thin !! Ideally we would need unit + integration test. I'm expecting many edge cases will break this function that is kind of long and weirdly organised. Look at:
try {
      await adminSdk.UpdateOrdersStatus({
        updates: orders.map((order) => ({
          _set: {
            status: OrderStatus_Enum.Completed,
          },
          where: {
            id: {
              _eq: order.id,
            },
          },
        })),
      });
    } catch (e) {
      console.error(e);
      await adminSdk.UpdateOrdersStatus({
        updates: orders.map((order) => ({
          _set: {
            status: OrderStatus_Enum.Error,
          },
          where: {
            id: {
              _eq: order.id,
            },
          },
        })),
      });
    }

We should have a try catch that take the whole function (just after the initial checks) By the way I don't see check like checkOrder. It should check the total of orders quantity to be sure there is enough.

  • Deprecate checkOrder , claimOrder. We shouldn't instantiate the sdk like this anymore and use a master wallet. Using the gasless should be the norm.
@sebpalluel sebpalluel added the bug Something isn't working label Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants