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/wallet integration #49

Merged
merged 18 commits into from
Apr 5, 2024
Merged

Feat/wallet integration #49

merged 18 commits into from
Apr 5, 2024

Conversation

truemiller
Copy link
Collaborator

Integrated new Wallet/Account management endpoints

@truemiller truemiller changed the base branch from main to feat/wallet April 4, 2024 14:24
@truemiller truemiller requested a review from mohandast52 April 4, 2024 14:33
Copy link
Collaborator

@mohandast52 mohandast52 left a comment

Choose a reason for hiding this comment

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

LGTM, but added some tiny comments.

* Returns a list of available wallets
*/
const getWallets = async () =>
fetch(`${BACKEND_URL}/wallet`).then((res) => res.json());
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you want to catch any errors?

Copy link
Collaborator Author

@truemiller truemiller Apr 4, 2024

Choose a reason for hiding this comment

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

no, this is intended.

if we catch it here it will alter the promises used later on, and potentially return a "fulfilled" status.

if for example the promise uses a chained .then() after this it will execute as the catch statement will fulfill

for example:
fetch(NaN).then(()=> 1).catch(()=>2).then(()=>3)
returns 3

frontend/service/Wallet.ts Outdated Show resolved Hide resolved
frontend/service/Wallet.ts Show resolved Hide resolved
frontend/service/Wallet.ts Show resolved Hide resolved
frontend/service/Services.ts Outdated Show resolved Hide resolved
frontend/components/Setup.tsx Outdated Show resolved Hide resolved
frontend/components/Setup.tsx Outdated Show resolved Hide resolved
frontend/components/Setup.tsx Outdated Show resolved Hide resolved
frontend/components/Setup.tsx Outdated Show resolved Hide resolved
frontend/components/Setup.tsx Outdated Show resolved Hide resolved
frontend/components/Setup.tsx Outdated Show resolved Hide resolved
frontend/components/Settings.tsx Show resolved Hide resolved
frontend/components/Setup.tsx Outdated Show resolved Hide resolved
frontend/components/Setup.tsx Show resolved Hide resolved
frontend/components/Setup.tsx Outdated Show resolved Hide resolved
frontend/components/Setup.tsx Show resolved Hide resolved
frontend/context/ServicesProvider.tsx Outdated Show resolved Hide resolved
const updateBalance = useCallback(async () => {
const walletsToCheck: Address[] = [];
for (const wallet of wallets) {
if (!getAddress || !wallet.address) continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's harder to understand negative conditions, can we change it? (and for 47 line as well)

Suggested change
if (!getAddress || !wallet.address) continue;
if (getAddress && wallet.address) {
walletsToCheck.push(wallet.address);
}

Copy link
Collaborator Author

@truemiller truemiller Apr 4, 2024

Choose a reason for hiding this comment

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

i tend to use guard clauses, to exit quickly, avoid future nesting, brackets .etc

will do my best to avoid negative conditions in future

frontend/enums/SetupScreen.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@mohandast52 mohandast52 left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 thanks @truemiller for addressing all the review requests 🙌

@truemiller truemiller merged commit ed44c4f into feat/wallet Apr 5, 2024
3 checks passed
@truemiller truemiller deleted the feat/wallet-integration branch April 5, 2024 19:43
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.

3 participants