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

feat(unlock-app): Support ENS in checkout builder for Referrer field #14834

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

sudheerDev
Copy link
Contributor

@sudheerDev sudheerDev commented Oct 10, 2024

Description

  • Add changes to use AddressField component to resolve ens in checkout builder form
  • Changed getReferrer to getReferrers to support an array of addresses
  • Added a new hook useGetReferrer
  • Added new util func shouldReferrerResolveForENS to check if a lock config has ENS
  • use shouldReferrerResolveForENS in getReferrers to resolve for ENS
  • Add changes to instances of getReferrers to be async

Issues

Fixes # #14102
Refs #

Checklist:

  • 1 PR, 1 purpose: my Pull Request applies to a single purpose
  • I have commented my code, particularly in hard-to-understand areas
  • I have updated the docs to reflect my changes if applicable
  • I have added tests (and stories for frontend components) that prove my fix is effective or that my feature works
  • I have performed a self-review of my own code
  • If my code involves visual changes, I am adding applicable screenshots to this thread

348459375-d371d3eb-b37e-4fca-a7a9-dddb78e046bb

Release Note Draft Snippet

* Added a new hook useGetReferrer
* Added new util func shouldReferrerResolveForENS to check if a
  lock config has ENS
* use shouldReferrerResolveForENS in getReferrers to resolve for ENS
@cla-bot cla-bot bot added the cla-signed label Oct 10, 2024
lockAddress
)
if (isReferrerAddressEns) {
if (isReferrerAddressEns && paywallConfig && paywallConfig.referrer) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason to change this to be here is that if the referrer is an ENS it will be the same for all. There is no reason to call this for all recipients since that will unnecessarily make quite a few ENS calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This codepath will be executed for any existing ENS addresses on config before this change goes live. since it will be formatted to address from here on out.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure the referrer should ever be an ENS. You are resolving the ENS in the settings UI and the config only receives the actual address, which, IMO is good!

Copy link
Contributor Author

@sudheerDev sudheerDev Nov 5, 2024

Choose a reason for hiding this comment

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

Additionally, if the config does use an ENS, instead of having an address, the paywall UI should be able to "resolve" that ENS and use the resolved address when sending transactions.

The issue states to handle this scenario, hence I added it. Let me know if my assumption here is wrong. If we don't have to resolve an ENS from config all we need is small change of using <AddressInput>

@sudheerDev
Copy link
Contributor Author

sudheerDev commented Oct 10, 2024

@julien51 Up for review

@@ -113,6 +113,7 @@
"deploy-fleek": "./scripts/deploy-fleek.sh",
"start": "yarn build && NODE_ENV=production next start",
"test": "UNLOCK_ENV=test vitest run --coverage --environment=jsdom",
"test:watch": "UNLOCK_ENV=test vitest --coverage --environment=jsdom -w",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added for convenience, Let me know if this has to be reverted

@julien51 julien51 self-requested a review November 1, 2024 14:24

import { getReferrers } from '~/utils/checkoutLockUtils'

export const useGetReferrers = (
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why we need a hook for this and we could not call getReferrers when we need ?

Copy link
Member

Choose a reason for hiding this comment

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

(you actually do that in several places and that's correct!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since getReferrers is not sync and it does make rpc calls, I am using hook. react query caches it based on the inputs to avoid making extra calls on re-renders causing issues.

error={errors.referrer?.message}
value={referrer}
onChange={(value: any) => {
setValue('referrer', value)
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@julien51 julien51 left a comment

Choose a reason for hiding this comment

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

Please, remove the hook and let's assume the paywall config's referrer value is always an address. If we ever want to add support for ENS, then we should do it upstream.

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

Successfully merging this pull request may close these issues.

2 participants