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: new transfer flow #3334

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

feat: new transfer flow #3334

wants to merge 6 commits into from

Conversation

o-az
Copy link
Member

@o-az o-az commented Dec 4, 2024

the work is in app/src/routes/transfer-new until the transfer form is fully refactored then just app/src/routes/transfer

purpose of this pr is to complete the first stage of the intent process; namely: validation step. Once the form is filled and the transfer is submitted, the validation results are logged

@o-az o-az requested review from cor and benluelo as code owners December 4, 2024 03:31
Copy link

vercel bot commented Dec 4, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
site ⬜️ Ignored (Inspect) Visit Preview Dec 11, 2024 7:02pm

Copy link

github-actions bot commented Dec 4, 2024

App 🤌

✨ Deployment complete! Take a peek over at https://ee763592.app-1b1.pages.dev
✨ Deployment alias URL: https://head.app-1b1.pages.dev


Copy link

pkg-pr-new bot commented Dec 5, 2024

npm i https://pkg.pr.new/unionlabs/union/@unionlabs/client@3334

commit: 163a48f

@o-az o-az force-pushed the refactor-transfer-form branch from ed248d8 to fd874ec Compare December 5, 2024 00:38
@o-az o-az force-pushed the refactor-transfer-form branch from fd874ec to 4099052 Compare December 6, 2024 16:42
Copy link
Contributor

@cor cor left a comment

Choose a reason for hiding this comment

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

I like the general structure of encoding pre-validated user input in the URL, but I feel that the data structures need some work.

  1. For all state, let's ensure that it is defined at most once, in strongly-typed structures
  2. let's refrain from $: as it is fully dropped in svelte 5,
  3. derived state should never be writable

Also, I noticed that whenever I click a form element which opens a dialog, then exit the dialog, then click one again, nothing happens. I need to click everything twice

Excited about the direction where this is heading!

import { type Writable, writable, derived, get, type Readable } from "svelte/store"
import { custom, switchChain, getConnectorClient, waitForTransactionReceipt } from "@wagmi/core"

type SearchParams = { [key: string]: string }
Copy link
Contributor

Choose a reason for hiding this comment

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

This should have a stronger type, rather than just key:value

queryFn: ({ queryKey, signal, meta }) => Object.fromEntries($page.url.searchParams)
})

let state = createQuery(transferQueryOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

state is not really descriptive

Comment on lines 88 to 92
$: asset = $page.url.searchParams.get("asset") || $state.data?.asset
$: amount = $page.url.searchParams.get("amount") || $state.data?.amount
$: receiver = $page.url.searchParams.get("receiver") || $state.data?.receiver
$: source = $page.url.searchParams.get("source") || $state.data?.source
$: destination = $page.url.searchParams.get("destination") || $state.data?.destination
Copy link
Contributor

Choose a reason for hiding this comment

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

What do tehse lines do? it seems that currently asset is defined in three places:

  1. searchParams.get("asset")
  2. searchParams.data?.asset
  3. $: asset = ...

Let's make sure we have only 1, strongly-typed, place where de fine the user input that is to be validated.

If we need to initially set this manually based on some initial data from the URL, I suggest handling this in an onMount call rather than introducing reactive state

Comment on lines 94 to 95
$: sourceChain = writable(chains.find(chain => chain.chain_id === source))
$: destinationChain = writable(chains.find(chain => chain.chain_id === destination))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is derived info that should never be written to. Let's use a let destinationChain = derived(...) which is of type Readable<T> rather than Writable<T>

Comment on lines 103 to 106
$: balances = derived([rawBalances, sourceChain], ([$rawBalances, $sourceChain]) => {
if (!($sourceChain && $rawBalances)) return []
return $rawBalances.data[source] ?? []
})
Copy link
Contributor

Choose a reason for hiding this comment

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

balances is derived so it should not have $:

Comment on lines 109 to 112
$: assetInfo = $balances.find(x => x?.address === asset)
$: {
console.log(asset, assetInfo)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this is derive data

@o-az o-az force-pushed the refactor-transfer-form branch from 4099052 to 923ff74 Compare December 10, 2024 21:26
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