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

Use evm inputs for issuers #2226

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

kattylucy
Copy link
Contributor

Use the address input that we have for CFG transfers, that supports both EVM addresses and Centrifuge addresses

#2221

Approvals

  • Dev
  • [x ] Dev
  • Designer
  • [ x] Product

Impact

Pool managers on Create pool page
Investor input for Centrifuge Chain on Investors tab
AO wallets on Access tab
Receiving address on Fees tab

Copy link

github-actions bot commented Jun 19, 2024

PR deployed in Google Cloud
URL: https://pr2226-app-ff-production.k-f.dev
Commit #: 44e0b8e
To access the functions directly check the corresponding deploy Action

Copy link

github-actions bot commented Jun 19, 2024

PR deployed in Google Cloud
URL: https://app-pr2226.k-f.dev
Commit #: 44e0b8e
To access the functions directly check the corresponding deploy Action

@kattylucy kattylucy marked this pull request as ready for review June 19, 2024 13:37
function handleBlur(e: React.FocusEvent<HTMLInputElement>) {
const address = e.target.value
if (address && !exists) {
onAdd(addressToHex(address))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the desired behaviour? If so, it should check that it's a valid address (check if truncated is truthy)

Comment on lines 256 to 258
value?: string
onChange: (e: React.ChangeEvent<HTMLInputElement>) => void
onBlur?: (e: React.FocusEvent<HTMLInputElement>) => void
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you have to explicitly declare these three types, they should already be included in the type React.InputHTMLAttributes<HTMLInputElement

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right ! - thanks

Copy link
Collaborator

@sophialittlejohn sophialittlejohn left a comment

Choose a reason for hiding this comment

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

Great job!

Can you please make sure the add address button is vertically centered? And the same for the pool managers?
Screenshot 2024-06-21 at 12 34 06

In the fees tab I noticed that when I use an EVM address it's saying "Invalid address", I think you might need to adjust the validation to make it work for EVM
Screenshot 2024-06-21 at 12 38 45

@sophialittlejohn
Copy link
Collaborator

Nice work Katty! Just a few more things to clarify and then I think it's good to go!

I think we can now remove the dropdown next to the input since the network is now implied by the address, buuuut @hieronx will we still need it to differentiate between the different EVM chains for liquidity pools?
Screenshot 2024-06-25 at 13 48 46

I think we also need some kind of icon for the evm wallets here, maybe just the ethereum logo for now? I also tried submitting the transaction but it failed: {"err":{"cannotLookup":null}}. Maybe you're still missing the address conversion here?
Screenshot 2024-06-25 at 13 51 11

@hieronx
Copy link
Contributor

hieronx commented Jun 25, 2024

I think we can now remove the dropdown next to the input since the network is now implied by the address, buuuut @hieronx will we still need it to differentiate between the different EVM chains for liquidity pools?

Yeah we still need to differentiate between EVM chains. For the Centrifuge network, the input could be either an EVM address or a Substrate/Centrifuge address. For all other networks, it's always an EVM address.

@kattylucy
Copy link
Contributor Author

Yeah we still need to differentiate between EVM chains.

How do we currently differentiate the two?

@hieronx
Copy link
Contributor

hieronx commented Jun 28, 2024

@kattylucy that was in response to Sophia's question. The dropdown is for selecting the network

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.

None yet

4 participants