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

sturdier check #5866

Merged
merged 12 commits into from
May 5, 2023
Merged

sturdier check #5866

merged 12 commits into from
May 5, 2023

Conversation

witmicko
Copy link
Contributor

@witmicko witmicko commented Mar 1, 2023

enhance #678

Development & PR Process

  1. Follow MetaMask Mobile Coding Standards
  2. Add release-xx label to identify the PR slated for a upcoming release (will be used in release discussion)
  3. Add needs-dev-review label when work is completed
  4. Add needs-qa label when dev review is completed
  5. Add QA Passed label when QA has signed off

Description

Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions,
1. What is the reason for the change?
2. What is the improvement/solution?

Screenshots/Recordings

If applicable, add screenshots and/or recordings to visualize the before and after of your change

Issue

Progresses #???

Checklist

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

@witmicko witmicko requested a review from a team as a code owner March 1, 2023 15:57
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2023

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.

Copy link
Contributor

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

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

A couple nitpicky comments, but otherwise the solution looks good. Running the branch now to experiment with locally. Can we add a unit test as well that this works as expected for a couple payloads?

app/components/UI/CollectibleOverview/index.js Outdated Show resolved Hide resolved
app/components/UI/CollectibleOverview/index.js Outdated Show resolved Hide resolved
@witmicko witmicko requested review from Cal-L, NicholasEllul and 409H March 8, 2023 16:11
app/util/linkCheck.ts Outdated Show resolved Hide resolved
app/util/linkCheck.ts Outdated Show resolved Hide resolved
app/util/linkCheck.ts Outdated Show resolved Hide resolved
app/util/linkCheck.test.ts Show resolved Hide resolved
app/util/linkCheck.ts Show resolved Hide resolved
@witmicko witmicko force-pushed the nft-view-protect-2 branch from d5c84ff to bef2567 Compare March 29, 2023 15:32
@witmicko witmicko requested a review from NicholasEllul March 29, 2023 16:38
Copy link
Contributor

@NicholasEllul NicholasEllul left a comment

Choose a reason for hiding this comment

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

Tested locally and changes look good to me

@witmicko witmicko added the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Apr 5, 2023
@witmicko witmicko force-pushed the nft-view-protect-2 branch from bef2567 to 1807171 Compare April 12, 2023 11:32
@socket-security
Copy link

socket-security bot commented Apr 12, 2023

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Ignoring: @types/[email protected], @types/[email protected]

Pull request alert summary
Issue Status
Critical CVE ✅ 0 issues
CVE ✅ 0 issues
Mild CVE ✅ 0 issues
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script confusion ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
GitHub dependency ✅ 0 issues
No bug tracker ✅ 0 issues
No contributors or author data ✅ 0 issues
No README ✅ 0 issues
Deprecated ✅ 0 issues
Unmaintained ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues
AI detected security risk ✅ 0 issues
AI warning ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
@types/[email protected] None +0 types
@types/[email protected] None +0 types

@witmicko witmicko requested a review from Cal-L April 12, 2023 11:47
app/util/linkCheck.test.ts Outdated Show resolved Hide resolved
@witmicko witmicko requested a review from Cal-L April 12, 2023 16:42
Copy link
Contributor

@Cal-L Cal-L left a comment

Choose a reason for hiding this comment

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

Could you pull from main to resolve the checks - Aside from that, LGTM
Please check in with QA @cortisiko or @chrisleewilcox for QA hand off before merging

@Cal-L Cal-L added needs-qa Any New Features that needs a full manual QA prior to being added to a release. team-mobile-client and removed needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Apr 13, 2023
@witmicko witmicko force-pushed the nft-view-protect-2 branch 2 times, most recently from ef24197 to 444205c Compare April 17, 2023 08:46
@witmicko witmicko force-pushed the nft-view-protect-2 branch from 444205c to 5187097 Compare April 20, 2023 15:57
@cortisiko cortisiko added QA in Progress QA has started on the feature. and removed needs-qa Any New Features that needs a full manual QA prior to being added to a release. labels May 4, 2023
@Cal-L
Copy link
Contributor

Cal-L commented May 4, 2023

@SocketSecurity ignore @types/[email protected]

@Cal-L
Copy link
Contributor

Cal-L commented May 4, 2023

@SocketSecurity ignore @types/[email protected]

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.

Looks good!

@cortisiko cortisiko merged commit 6541a90 into main May 5, 2023
@cortisiko cortisiko deleted the nft-view-protect-2 branch May 5, 2023 00:16
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2023
@NicholasEllul NicholasEllul removed the QA in Progress QA has started on the feature. label May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants