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

Blair/sync urlscan worigin #73

Open
wants to merge 3 commits into
base: dan/remove-seed-phrase
Choose a base branch
from

Conversation

blairmunroakusa
Copy link
Contributor

Simplified contribution to head dev branch to,

  • simplify URL scanning flow (only one message pass, instead of two as before)
  • clean up misc icons in extension and bundle in assets directory
  • update manifest file to include module type and simplified access declaration
  • create service and content directories, for worker.js and script.js, resrpectively
  • clean up misc magic strings by declaring new constants
    -! temporarily commented out the galactus call (cut down on error noise)

@jmercouris
Copy link
Contributor

Have you confirmed that all operations are still functional :-D?

@blairmunroakusa
Copy link
Contributor Author

blairmunroakusa commented Sep 9, 2023

@jmercouris @DecentralizedDan I have officially covered my tracks and found all the broken things I thought I found but didn't. ! I fixed them. I have list of bugs I collected to diff against my branch. That is as issue here:
PR https://github.com/interlock-network/threatslayer/issues/75

@DecentralizedDan The issues you were having with the scripting API breaking things if not in manifest has something to do with how you didn't complete the merge somehow...not sure what happened, but after inspecting remove-seed-phrase, I couldn't find any changes relevant to this PR. That is good though, because I made a final update to this PR to make the improved TS version non-crap and fully functional.

This branch is ready-ready for merge.

@jmercouris
Copy link
Contributor

Let us be careful to make so many changes right before a big release! It could be catastrophic :-O

@blairmunroakusa
Copy link
Contributor Author

I mean, we don't need to merge now...makes no difference to me ultimately in the near term. In any case however, our merge into main will introduce massive changes regardless of this particular PR being included or not. I am only proposing a merge into that branch itself that we will soon merge to main, this PR actually a merge to main. The merge 1) works 2) simplifies everything 3) cleans up the mess that is the extension src dir and 4) uses fewer permissions than the previous remove-seed-phrase branch.

Just saying this to emphasize that the large changes in PR are comparatively speaking, a non issue. We could compromise, and create a branch out of current remove-seed-phrase to merge this into, so we can see how it handles. My only gripe is that the extension as it stands is 1) a mess 2) uses more permissions than it needs and 3) is garbage in the way the url scanning flow is implemented. Like, it's embarrassing (and I feel somewhat responsible, because it was I who implemented that particular flow for triggering the warning banner).

But yeah, whatever, not really important to me at the moment. If however, I find myself needing to build dApp stuff into our service-worker or our banner content script, then for the record I need this branch integrated with main.

@blairmunroakusa
Copy link
Contributor Author

And then there is the messed up styling problem:
image
image

Above are a couple examples of how current remove-seed-phrase branch messes up styling after closing warning banner.

This branch fixes that.

@jmercouris
Copy link
Contributor

I guess I'm confused. How is this related to a dApp?

@jmercouris
Copy link
Contributor

Are you saying that there exists some sort of integration with Polkadot that is misbehaving given the current environment?

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