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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added custom ethereum network url generation #31

Merged
merged 14 commits into from
Mar 10, 2021
Merged

Added custom ethereum network url generation #31

merged 14 commits into from
Mar 10, 2021

Conversation

BboyAkers
Copy link
Contributor

@BboyAkers BboyAkers commented Feb 28, 2021 •
edited

This should allow for the ability to generate urls for custom networks based upon EIP 3091.

Still a WIP. Tried several other ways to do this but decided on this. Would love to know ya'lls thoughts 馃檪!! Any better ideas to on building this out? @Gudahtt @rekmarks @danfinlay @darkwing

@BboyAkers BboyAkers requested a review from a team as a code owner February 28, 2021 05:20
@BboyAkers BboyAkers changed the title Added custom network url generation Added custom ethereum network url generation Feb 28, 2021
Gudahtt
Gudahtt requested changes Mar 1, 2021
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment •
edited

Choose a reason for hiding this comment

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

Thanks, this is a good start!

I think the API would be simpler if we separated the functions based upon EIP-3091 from the others. Maybe they could all go in a separate module or directory or something?

Also it would be great to have a couple of usage examples in the README

src/account-link.ts Outdated Show resolved Hide resolved
src/account-link.ts Outdated Show resolved Hide resolved
src/token-tracker-link.ts Outdated Show resolved Hide resolved
@BboyAkers
Copy link
Contributor Author

BboyAkers commented Mar 4, 2021 •
edited

Made some more changes! Let me know if these work 馃檪 . If not, please do yo thang 馃槑 ThePRGuru

@BboyAkers BboyAkers mentioned this pull request Mar 4, 2021
Closed
@brad-decker
Copy link
Contributor

@BboyAkers - could I get you to review #32 and see how you think our work should best merge when it comes time?

Gudahtt
Gudahtt reviewed Mar 4, 2021
View reviewed changes
src/token-tracker-link.ts Outdated Show resolved Hide resolved
src/token-tracker-link.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@Gudahtt
Copy link
Member

Gudahtt commented Mar 4, 2021

If the custom network functionality is isolated to separate functions (like getCustomBlockExplorerAccountLink), I don't think there will be any overlap. Your PR will ensure the etherscan links are chain ID based, and custom networks won't use a chain ID or network ID.

@BboyAkers
Copy link
Contributor Author

@BboyAkers - could I get you to review #32 and see how you think our work should best merge when it comes time?

I'll take a look today 馃檪

@brad-decker
Copy link
Contributor

@BboyAkers - could I get you to review #32 and see how you think our work should best merge when it comes time?

I'll take a look today 馃檪

Still want your review, merged but hasn't been released yet. If you get time to look at it I'll make any suggested changes ahead of the package update. Maybe holding off until this lands too :)

@BboyAkers
Copy link
Contributor Author

@BboyAkers - could I get you to review #32 and see how you think our work should best merge when it comes time?

I'll take a look today 馃檪

Still want your review, merged but hasn't been released yet. If you get time to look at it I'll make any suggested changes ahead of the package update. Maybe holding off until this lands too :)

Gotcha! Just logged off for the day 馃槂. I should be able to give it in the next hour or so!

@BboyAkers
Copy link
Contributor Author

Pulling locally to test some things and fix some wonkyness on my merge conflict. Should hopefully be by morning. 馃槄

@brad-decker brad-decker mentioned this pull request Mar 5, 2021
Closed
@BboyAkers
Copy link
Contributor Author

@Gudahtt @brad-decker let me know what you think! 馃檪

@BboyAkers BboyAkers requested a review from Gudahtt March 5, 2021 23:21
Gudahtt
Gudahtt reviewed Mar 9, 2021
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Lots of indentation issues! I added a bunch of suggestions - I'll commit them all in a batch to move the review forward.

src/account-link.ts Outdated Show resolved Hide resolved
src/helpers.ts Outdated Show resolved Hide resolved
src/token-tracker-link.ts Outdated Show resolved Hide resolved
src/explorer-link.ts Outdated Show resolved Hide resolved
Gudahtt
Gudahtt reviewed Mar 9, 2021
View reviewed changes
src/token-tracker-link.ts Outdated Show resolved Hide resolved
Gudahtt
Gudahtt reviewed Mar 9, 2021
View reviewed changes
tsconfig.json Show resolved Hide resolved
Gudahtt
Gudahtt reviewed Mar 9, 2021
View reviewed changes
src/token-tracker-link.ts Outdated Show resolved Hide resolved
Gudahtt
Gudahtt approved these changes Mar 10, 2021
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@BboyAkers
Copy link
Contributor Author

@brad-decker can you approve as well por favor so I can merge? 馃檪

brad-decker
brad-decker approved these changes Mar 10, 2021
View reviewed changes
Copy link
Contributor

@brad-decker brad-decker left a comment

Choose a reason for hiding this comment

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

LGTM!

@brad-decker brad-decker merged commit eafa614 into MetaMask:master Mar 10, 2021
@BboyAkers BboyAkers deleted the customNetwork branch March 10, 2021 20:09
@Gudahtt
Copy link
Member

Gudahtt commented Mar 11, 2021

This has now been released in v2.0.0 of this package

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@brad-decker brad-decker brad-decker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@BboyAkers @brad-decker @Gudahtt