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.

Sign up for GitHub

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

Jump to bottom

Defaults to current network if chain id not specified in QR codes #3929

Merged
merged 8 commits into from
Apr 28, 2022
Merged

Defaults to current network if chain id not specified in QR codes #3929

merged 8 commits into from
Apr 28, 2022

Conversation

Fatxx
Copy link
Contributor

@Fatxx Fatxx commented Mar 21, 2022 •
edited
Loading

Description

Use current network as default if chain id is not specified in QR codes or deeplinks.

Checklist

  • There is a related GitHub issue
  • Tests are included if applicable
  • Any added code is fully documented

Issue

Resolves #3847 and https://github.com/metamask/mobile-planning/issues/178

@Fatxx Fatxx requested a review from a team as a code owner March 21, 2022 22:23
@github-actions
Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@Fatxx Fatxx changed the title Defaults to mainnet if chain id not specified on QR codes Defaults to mainnet if chain id not specified in QR codes Mar 21, 2022
@Fatxx Fatxx changed the title Defaults to mainnet if chain id not specified in QR codes Defaults to current network if chain id not specified in QR codes Mar 22, 2022
@Fatxx Fatxx added type-bug Something isn't working deeplinks Deeplinks related issue or bug needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 23, 2022
@cortisiko cortisiko added the QA Passed A successful QA run through has been done label Mar 23, 2022
cortisiko
cortisiko approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

🌮 🌮 🌮 🌮

gantunesr
gantunesr approved these changes Mar 23, 2022
View reviewed changes
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

lgtm

@cortisiko cortisiko removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 24, 2022
@mobularay
Copy link
Contributor

Take-away from Mar 24 sprint review: @Fatxx @omnat @andrepimenta to discuss the logic on how chainID and QR code should interact with each other

gantunesr
gantunesr requested changes Mar 25, 2022
View reviewed changes
Copy link
Member

@gantunesr gantunesr left a comment

Choose a reason for hiding this comment

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

Waiting on resolution for this PR

@Fatxx
Copy link
Contributor Author

Fatxx commented Mar 25, 2022

Still discussing with @omnat possible changes.

@mobularay mobularay added the DO-NOT-MERGE Pull requests that should not be merged label Mar 27, 2022
@kevinghim kevinghim mentioned this pull request Mar 29, 2022
Closed
@gantunesr gantunesr removed the QA Passed A successful QA run through has been done label Apr 5, 2022
@Fatxx Fatxx removed the DO-NOT-MERGE Pull requests that should not be merged label Apr 21, 2022
@Fatxx Fatxx added needs-qa Any New Features that needs a full manual QA prior to being added to a release. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels Apr 21, 2022
@Fatxx Fatxx mentioned this pull request Apr 21, 2022
Merged
3 tasks
@Fatxx Fatxx added the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 26, 2022
@Fatxx
Copy link
Contributor Author

Fatxx commented Apr 28, 2022

Let's assume this as QA passed since there where no changes after @cortisiko passed it before and our conversations didn't reflect in any further changes.

@cortisiko cortisiko removed the needs-qa Any New Features that needs a full manual QA prior to being added to a release. label Apr 28, 2022
cortisiko
cortisiko approved these changes Apr 28, 2022
View reviewed changes
Copy link
Member

@cortisiko cortisiko left a comment

Choose a reason for hiding this comment

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

🌮 🌮 🌮 🌮 🌮 🌮 🌮

@Fatxx Fatxx merged commit 73ede4f into main Apr 28, 2022
@Fatxx Fatxx deleted the bugfix/default-to-mainnet-on-deeplinks-qrcodes branch April 28, 2022 15:48
@github-actions github-actions bot locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@cortisiko cortisiko cortisiko approved these changes

@gantunesr gantunesr gantunesr approved these changes

Assignees
No one assigned
Labels
deeplinks Deeplinks related issue or bug type-bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Network Not Found - Missing chain id.
4 participants
@Fatxx @mobularay @cortisiko @gantunesr