-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Adds an address changer tool #935
Conversation
This looks great! Is it ready to merge? |
docs/learn-accounts.md
Outdated
@@ -353,3 +353,17 @@ need to place the deposit and Alice will receive her deposit back. | |||
- [Understanding Accounts and Keys in Polkadot](https://www.crowdcast.io/e/polkadot-keys) - An | |||
explanation of what the different kinds of accounts and keys are used for in Polkadot, with Bill | |||
Laboon and Chinmay Patel of BlockX Labs. | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would recommend just placing this above Resources - as I think that should be the last section of this page
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep added a few more, we can support all of them once we have an available registry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Wondering if we could also add in choices for Plasm, Acala, Centrifuge, etc... ?
This seems to add quite a big file into the wiki's commit history, are we sure we want this approach? |
The other option is that we have a separate service instead of hosting the static files in the wiki. The advantage of the current method is that it would work offline and without making any external calls and is arguably simpler. The downside is that we have a large JavaScript file which is mostly the Polkadot-JS dependency. |
Do we need the whole of PolkadotJS just for the address switch? It's just a prefix translation, so it shouldn't be required, right? Can we strip it down to just that functionality? |
The js is packaged and minified, so that might be a bit involved. I'm honestly fine with including a ~ 1 MB file, as long as we don't make a habit of it. Yeah git is distributed but it's not like a blockchain where it's going to be shared among thousands of nodes... |
Right but we'll need to bump this version from time to time, which will bloat our history whenever we bump. There are unminified versions of API we could dig the important stuff out of imo. |
Ok, I redirected the dependencies to pull directly from the relevant file which brought the size of the minified file by about ~30% from 1.4 MB to just under 1 MB. I don't think I'm gonna get this reduced any further unless we write our own versions of these functions which would be less than ideal because 1) we diverge from the same implementation used by polkadot-js and 2) we would be spending time on optimizing a specific library for relatively little upside. If we are not happy with a 1 MB minified file I would rather just host the logic as a Lambda function and make an external call from the wiki page. This still has the downside of being less reliable than the current implementation due to network connectivity. It also increases the operational overhead of maintaining this small widget. If it makes it any better, my MB karma will be more than balanced out by #923 which should reduce the overall HEAD of the git tree's size by one or two orders of magnitude more in MB than what I'm adding with this PR. |
Would we actually have had to update the file? I can't imagine something as core as the address translation function would be updated often, if at all. But this is a moot point - a kilobyte is fine, I'm sure. =). |
A kilobyte sure, but this is a megabyte :) Either way, I think we're good, let's see how much of an effect it has long term. If push comes to shove we can always reset history. This ain't blockchain :P |
Not sure how I misread MB as KB. I must need a vacation. |
@lsaether Can you check on why there's the weird CI linking error on this? Other than that I think it's good to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge when ready
closes #836