Skip to content

Conversation

@kayvee-x
Copy link

  • Added ERROR status to OWNERSHIP_STATUS enum for proper error handling
  • Made verification message customizable via prop with sensible default
  • Removed TODOs with explanations:
    • Global state: Appropriate for this use case, simpler than Redux/localStorage
    • Signature caching: Adds complexity without significant UX benefit

Copy link
Contributor

@johnpmitsch johnpmitsch left a comment

Choose a reason for hiding this comment

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

Seems like this moves the verification message and adds another error type but there's a lot of TODOs removed that don't seem addressed here

'This message is used to verify your wallet ownership.\n\nSigning this message will not make any transactions.';
await signer.signMessage(message);
// TODO: Store the hashed message, and re-verify hash instead of having to ask wallet to sign each time
// https://github.com/ethers-io/ethers.js/issues/447
Copy link
Contributor

Choose a reason for hiding this comment

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

@kayvee-x does this address this TODO?

// Using this global state to persist the verification state between page navigation
// TODO: Can we do this with a localstorage or a cookie in a secure way?
// that can't be faked by non-nft holders with an expiration?
// TODO: Can we use redux here?
Copy link
Contributor

Choose a reason for hiding this comment

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

@kayvee-x are these TODOs addressed by these changes?

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.

2 participants