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

PE-4727: meta mask login #1398

Closed
wants to merge 51 commits into from
Closed

Conversation

thiagocarvalhodev
Copy link
Collaborator

@thiagocarvalhodev thiagocarvalhodev commented Oct 3, 2023

@thiagocarvalhodev thiagocarvalhodev self-assigned this Oct 3, 2023
@github-actions
Copy link

github-actions bot commented Oct 3, 2023

Visit the preview URL for this PR (updated for commit 6f1f3ac):

https://ardrive-web--pr1398-pe-4727-meta-mask-lo-g6k4f9xl.web.app

(expires Tue, 24 Oct 2023 14:49:22 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: a224ebaee2f0939e7665e7630e7d3d6cd7d0f8b0

@matibat matibat self-requested a review October 10, 2023 19:19
This improves compatibility with some other wallets (e.g. Phantom)
@elliotsayes
Copy link
Contributor

Small tweak to support some other MetaMask-compatible wallets (e.g. Phantom)
#1418

Copy link
Contributor

@matibat matibat left a comment

Choose a reason for hiding this comment

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

Looking good! Left some comments.

lib/authentication/login/blocs/login_bloc.dart Outdated Show resolved Hide resolved
lib/authentication/login/blocs/login_bloc.dart Outdated Show resolved Hide resolved
lib/authentication/login/views/login_page.dart Outdated Show resolved Hide resolved
lib/blocs/profile/profile_cubit.dart Outdated Show resolved Hide resolved
lib/components/profile_card.dart Show resolved Hide resolved
lib/models/database/database.dart Outdated Show resolved Hide resolved
profileType INTEGER NOT NULL
profileType INTEGER NOT NULL,

-- Added in schema v18
Copy link
Contributor

Choose a reason for hiding this comment

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

I loved this comment. We shall start doing this for future changes. 🧠

@matibat
Copy link
Contributor

matibat commented Oct 11, 2023

The build GH Action is failing. Something related to the Stripe package.

@elliotsayes
Copy link
Contributor

Updates based on comments (WIP) #1421

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's see... Some rubber-duck debugging here 🦆

  1. We don't have a migration strategy until the 17th one. So we do:
    a. from >= 1 && from < 16
  2. When we're moving from migration v16 to v17, we do:
    a. from < 17
  3. And when we're moving from the migration v17 to v18:
    a. from < 18
  • ✅ If we go from v1 to v18, then we fall in the 1st case.
  • ❌ If we go from v16 to v18, then we fall in the 2nd case. - Here we'd be migrating to v17, and after the migration is done we'd still have an old migration.
  • ✅ If we go from v17 to v18, then we fall in the 3rd case.

Please take a look to my thinking process, perhaps I got it wrong. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

If we go from v16 to v18, we would fall into 2nd AND 3rd cases (it is a sequence of ifs, not an if/else)
Maybe the nesting here is confusing? Instead of the main if/else should I do an if with early return?
Screenshot 2023-10-13 at 8 37 11 AM
Here's the example from the drift docs I used as a reference
Screenshot 2023-10-13 at 8 36 34 AM

@thiagocarvalhodev
Copy link
Collaborator Author

Closing this as the ETH login will be addressed in this PR: #1614

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