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

disperse v2 [WIP] #52

Open
wants to merge 63 commits into
base: develop
Choose a base branch
from
Open

disperse v2 [WIP] #52

wants to merge 63 commits into from

Conversation

w84april
Copy link
Collaborator

@w84april w84april commented Feb 6, 2024

No description provided.

Copy link

vercel bot commented Feb 6, 2024

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

Name Status Preview Comments Updated (UTC)
smoldapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 2, 2024 0:36am

@w84april w84april changed the title feat: refactor input components disperse v2 Feb 11, 2024
@w84april w84april changed the title disperse v2 disperse v2 [WIP] Feb 11, 2024
components/designSystem/NetworkSelector/Dropdown.tsx Outdated Show resolved Hide resolved
components/designSystem/Curtains/InfoCurtain.tsx Outdated Show resolved Hide resolved
next.config.js Outdated Show resolved Hide resolved
contexts/useBalancesCurtain.tsx Show resolved Hide resolved
@@ -164,7 +182,7 @@ export const WithAddressBook = ({children}: {children: React.ReactElement}): Rea
try {
const existingEntry = await getEntry({address: entry.address});
if (existingEntry) {
const mergedChains = [...(existingEntry.chains || []), ...(entry.chains || [])];
const mergedChains = [...(entry.chains || [])];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
const mergedChains = [...(entry.chains || [])];
const mergedChains = [...(existingEntry.chains || []), ...(entry.chains || [])];

Once you update an entry for a given chain, you want to use the most up-to-date version of it, aka the one in the storage (from existingEntry), not the one in the cache/state (from entry) as base.
So [...storage, ...cache]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This way it's not possible to update the chains completely. It was only possible to add new chains to the entry, but not removing some of them. The reason for that is we were always spreading existingEntry.chains into merged chains which seemed wrong. I think that chains can be the only field to be overwritten. Can you pls provide some example when we actually need to spread the chains like this? It will help to come up with new logic for this

components/sections/Send/Wizard.tsx Outdated Show resolved Hide resolved
components/sections/Send/Wizard.tsx Outdated Show resolved Hide resolved
Comment on lines 180 to 182
allSelected.forEach(input => onUpdateStatus(input.UUID, 'pending'));
const {safeTxHash} = await sdk.txs.send({txs: transactions});
allSelected.forEach(input => onUpdateStatus(input.UUID, 'success'));
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this loops should be merged into one single forof

components/sections/Send/SendStatus.tsx Outdated Show resolved Hide resolved
components/designSystem/SmolAddressInput.tsx Outdated Show resolved Hide resolved
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