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.

Sign up for GitHub

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

Jump to bottom

Use evm inputs for issuers #2226

Merged
merged 2 commits into from
Jul 23, 2024
Merged

Use evm inputs for issuers #2226

merged 2 commits into from
Jul 23, 2024

Conversation

kattylucy
Copy link
Collaborator

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 19, 2024 •
edited
Loading

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

@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 19, 2024 •
edited
Loading

PR deployed in Google Cloud
URL: https://app-pr2226.k-f.dev
Commit #: 00cf81b
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
onnovisser
onnovisser reviewed Jun 19, 2024
View reviewed changes
centrifuge-app/src/pages/IssuerPool/Configuration/AddAddressInput.tsx
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)

sophialittlejohn
sophialittlejohn reviewed Jun 21, 2024
View reviewed changes
fabric/src/components/TextInput/index.tsx Outdated
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
Collaborator 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

sophialittlejohn
sophialittlejohn reviewed Jun 21, 2024
View reviewed changes
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
Collaborator 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

@kattylucy kattylucy force-pushed the use_evm_inputs_for_issuers branch 3 times, most recently from 33df005 to 89930f2 Compare July 16, 2024 15:57
@sophialittlejohn
Copy link
Collaborator

Almost there, typing doesn't work anymore on this input

Screenshot 2024-07-18 at 17 24 54

sophialittlejohn
sophialittlejohn approved these changes Jul 22, 2024
View reviewed changes
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.

Left one more comment to clean up a bit but already looks good to me 🔥 nice work pushing this one through!

centrifuge-app/src/pages/IssuerPool/Access/AssetOriginators.tsx Outdated
Comment on lines 161 to 168
React.useEffect(() => {
const fetchChainId = async () => {
const id = await cent.getChainId()
setChainId(id)
}

fetchChainId()
}, [cent])
Copy link
Collaborator

Choose a reason for hiding this comment

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

this would make a good hook

onnovisser
onnovisser reviewed Jul 23, 2024
View reviewed changes
centrifuge-app/src/pages/IssuerPool/Access/AssetOriginators.tsx Outdated
@@ -156,6 +158,15 @@ function AOForm({
transferAllowlist: { receiverDeposit },
} = useCentrifugeConsts()

React.useEffect(() => {
const fetchChainId = async () => {
const id = await cent.getChainId()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is the wrong chain id. you'll need to use useCentEvmChainId()

onnovisser
onnovisser reviewed Jul 23, 2024
View reviewed changes
fabric/src/components/TextInput/index.tsx Outdated
...inputProps
}: AddressInputProps) {
const defaultId = React.useId()
id ??= defaultId

const [inputVal, setInputVal] = React.useState<string>(value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

the state doesn't seem necessary. the effect can use the value prop. using state here can lead to a bug when the value is changed outside the component, causing the effect not to run

@kattylucy kattylucy force-pushed the use_evm_inputs_for_issuers branch 2 times, most recently from 9a29ce0 to ab6c3c3 Compare July 23, 2024 13:27
onnovisser
onnovisser reviewed Jul 23, 2024
View reviewed changes
centrifuge-js/src/utils/index.ts Outdated
@@ -137,6 +137,8 @@ export function computeMultisig(multisig: Multisig): ComputedMultisig {
}

export function evmToSubstrateAddress(address: string, chainId: number) {
if (!chainId) return
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to throw in that case

@kattylucy kattylucy merged commit cede9eb into main Jul 23, 2024
14 checks passed
@kattylucy kattylucy deleted the use_evm_inputs_for_issuers branch July 23, 2024 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onnovisser onnovisser onnovisser approved these changes

@sophialittlejohn sophialittlejohn Awaiting requested review from sophialittlejohn

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kattylucy @sophialittlejohn @hieronx @onnovisser