Skip to content

Blair/sync urlscan worigin#73

Open
blairmunroakusa wants to merge 3 commits into
dan/remove-seed-phrasefrom
blair/sync-urlscan-worigin
Open

Blair/sync urlscan worigin#73
blairmunroakusa wants to merge 3 commits into
dan/remove-seed-phrasefrom
blair/sync-urlscan-worigin

Conversation

@blairmunroakusa

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

@blairmunroakusa

blairmunroakusa commented Sep 9, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Contributor

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

@blairmunroakusa

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@jmercouris

Copy link
Copy Markdown
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