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

Named Accounts #1699

Merged
merged 15 commits into from
Nov 14, 2023
Merged

Named Accounts #1699

merged 15 commits into from
Nov 14, 2023

Conversation

buberdds
Copy link
Contributor

@buberdds buberdds commented Oct 5, 2023

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Oct 5, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 7bc1ef6
Status: ✅  Deploy successful!
Preview URL: https://8f33baab.oasis-wallet.pages.dev
Branch Preview URL: https://mz-namedacc.oasis-wallet.pages.dev

View logs

@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

Merging #1699 (dbf3e8c) into master (baf8166) will decrease coverage by 0.31%.
The diff coverage is 65.21%.

❗ Current head dbf3e8c differs from pull request most recent head 8a7dcb7. Consider uploading reports for the commit 8a7dcb7 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1699      +/-   ##
==========================================
- Coverage   82.52%   82.21%   -0.31%     
==========================================
  Files         185      185              
  Lines        4720     4796      +76     
  Branches      846      880      +34     
==========================================
+ Hits         3895     3943      +48     
- Misses        825      853      +28     
Flag Coverage Δ
cypress 48.19% <45.65%> (-0.17%) ⬇️
jest 77.91% <64.34%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/app/components/AddressBox/index.tsx 100.00% <100.00%> (ø)
src/app/components/DeleteInputForm/index.tsx 58.33% <ø> (ø)
src/styles/theme/ThemeProvider.tsx 96.55% <ø> (ø)
...ponents/Toolbar/Features/AccountSelector/index.tsx 90.32% <83.33%> (-1.68%) ⬇️
...nts/Toolbar/Features/Account/ManageableAccount.tsx 68.75% <33.33%> (-9.03%) ⬇️
src/app/pages/AccountPage/index.tsx 94.87% <77.77%> (-2.32%) ⬇️
src/app/state/wallet/index.ts 72.00% <0.00%> (-6.27%) ⬇️
...ages/AccountPage/Features/AccountSummary/index.tsx 85.29% <70.58%> (-14.71%) ⬇️
...lbar/Features/Account/ManageableAccountDetails.tsx 32.05% <20.68%> (-5.45%) ⬇️

... and 2 files with indirect coverage changes

@buberdds buberdds force-pushed the mz/namedAcc branch 8 times, most recently from 35e825e to 34bfa75 Compare October 5, 2023 16:22
@buberdds buberdds marked this pull request as ready for review October 5, 2023 16:24
@buberdds buberdds requested a review from lukaw3d October 5, 2023 16:30
@buberdds buberdds force-pushed the mz/namedAcc branch 4 times, most recently from be3ba44 to 7bc1ef6 Compare October 13, 2023 11:36
@buberdds buberdds requested a review from lukaw3d October 16, 2023 08:24
const ticker = useSelector(selectTicker)

const unlockedStatus = useSelector(selectUnlockedStatus)
const canEditName = unlockedStatus === 'unlockedProfile' && address === wallet?.address
Copy link
Member

Choose a reason for hiding this comment

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

Alright this isn't wrong, but I don't think it's the best:

  • create a profile with named accounts name1 (address1) and name2 (address2)
  • select name1:
    • see name1 (address1)
    • url is /account/address1
  • change url to /account/address2
  • unlock profile and get redirected to address1
  • click back
    • see address2 without name
    • url is /account/address2

Copy link
Contributor Author

@buberdds buberdds Oct 23, 2023

Choose a reason for hiding this comment

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

I don't think this issue is related to the PR. User will end up in "This is not your account." state, right? not able to do transfers etc.

Copy link
Contributor Author

@buberdds buberdds Nov 6, 2023

Choose a reason for hiding this comment

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

bump, can we revive this discussion?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, user gets into "not your account" state.
Part related to this PR is: it still could show name2 (address2), and canEditName could be more accurate as unlocked && address is in wallet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will skip it in this PR. Wallet state is broken in this case. We show address2 when persisted state has wrong selectedWallet value at this point.

Copy link

github-actions bot commented Nov 14, 2023

Deployed to Cloudflare Pages

Latest commit: 8a7dcb79d654c7486377b1ef6b525358ce789fc4
Status:✅ Deploy successful!
Preview URL: https://14d217fc.oasis-wallet.pages.dev

@buberdds buberdds force-pushed the mz/namedAcc branch 2 times, most recently from e2fd723 to dbf3e8c Compare November 14, 2023 13:23
@buberdds buberdds merged commit 9c2f73c into master Nov 14, 2023
10 checks passed
@buberdds buberdds deleted the mz/namedAcc branch November 14, 2023 17:04
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