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

Adds an address changer tool #935

Merged
merged 7 commits into from
Sep 16, 2020
Merged

Adds an address changer tool #935

merged 7 commits into from
Sep 16, 2020

Conversation

lsaether
Copy link
Contributor

closes #836

image

@laboon
Copy link
Contributor

laboon commented Sep 11, 2020

This looks great! Is it ready to merge?

@@ -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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@krichard410 krichard410 left a 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... ?

@Swader
Copy link
Contributor

Swader commented Sep 11, 2020

This seems to add quite a big file into the wiki's commit history, are we sure we want this approach?

@lsaether
Copy link
Contributor Author

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.

@Swader
Copy link
Contributor

Swader commented Sep 14, 2020

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?

@laboon
Copy link
Contributor

laboon commented Sep 14, 2020

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...

@Swader
Copy link
Contributor

Swader commented Sep 14, 2020

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.

@lsaether
Copy link
Contributor Author

lsaether commented Sep 14, 2020

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.

@laboon
Copy link
Contributor

laboon commented Sep 15, 2020

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. =).

@Swader
Copy link
Contributor

Swader commented Sep 15, 2020

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

@laboon
Copy link
Contributor

laboon commented Sep 15, 2020

Not sure how I misread MB as KB. I must need a vacation.

@laboon
Copy link
Contributor

laboon commented Sep 16, 2020

@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.

@lsaether lsaether changed the title Adds an adress changer tool Adds an address changer tool Sep 16, 2020
Copy link
Contributor

@Swader Swader left a comment

Choose a reason for hiding this comment

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

Merge when ready

@lsaether lsaether merged commit 6909f9a into master Sep 16, 2020
@lsaether lsaether deleted the ls-address-changer branch September 16, 2020 19:01
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.

Add tool for showing addresses on different networks
5 participants