Add ud resolution #15817
Add ud resolution #15817
Conversation
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
I have read the CLA Document and I hereby sign the CLA |
1 similar comment
|
I have read the CLA Document and I hereby sign the CLA |
|
Hey @bbehrman10 and @V-Shadbolt -- thanks for the submission. Looks like a lot of test failures are valid. I'm going to change this to draft until you have tests passing and ready for review. Feel free to pull it out of draft when ready |
There was a problem hiding this comment.
Before any further review will happen:
- Please go through and update variable names like 'object' to something of meaning for reviewers and future maintainers of this code. Naming things is hard, but it is absolutely vital for legibility and understanding code and it is impossible to review when combined with the second bullet point:
- Add jsdocs to every method you are introducing that describe what it does, and also any inline docs that help to explain your intent with the changes being made.
- Analyze my request to remove web requests for viable TLDs for unstoppable domains, as well as keys.
Thank you for the contribution and I hope we can work through these changes and find a way to get this landed.
ui/ducks/send/send.js
Outdated
| if (state.UNS) { | ||
| if ( | ||
| state.UNS.resolution === | ||
| state.send.draftTransactions[state.send.currentTransactionUUID] | ||
| .recipient.address && | ||
| state.UNS.domainName | ||
| ) { | ||
| const object = await swapToken(state.UNS.domainName, asset); | ||
| if (object.error) { | ||
| if ( | ||
| object.error === 'UnspecifiedCurrency' || | ||
| object.error === 'RecordNotFound' | ||
| ) { | ||
| asset.error = UNS_CURRENCY_SPEC_ERROR; | ||
| } else if (object.error === 'UnsupportedCurrency') { | ||
| asset.error = UNS_CURRENCY_ERROR; | ||
| } else { | ||
| asset.error = UNS_UNKNOWN_ERROR; | ||
| } | ||
| await dispatch(actions.updateAsset({ asset, initialAssetSet })); | ||
| throw new Error('No address associated with this token'); | ||
| } else { | ||
| dispatch(unsLookup(object)); | ||
| await dispatch( | ||
| updateRecipient({ | ||
| address: object.address, | ||
| nickname: object.unsName, | ||
| }), | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This code is duplicated can you help me understand what this portion of the code is doing?
There was a problem hiding this comment.
we had this twice because when you swap a token it can either be a native token or a multi chain token
V-Shadbolt
requested review from
brad-decker
and removed request for
georgewrmarshall
|
Possible duplicate of #7258 |
This is an updated implementation reflecting support for our most recent registry inclusive of Polygon based NFT Domains. This version uses our most recent and established libraries, so I think the old one is no longer applicable |
|
@brad-decker - We can submit a change and move the data from the endpoint to a constants file. If that's okay with you we can do that, and in the coming weeks / months if there needs to be an update to those constants (such as if UD releases more token support) just submit a PR with only changes to that constants file. In this way the integration can stay up to date without the need of that REST endpoint. How does that sound? Alternatively we're also comfortable if you are to keep that open source endpoint to leave it all server side. |
|
Open source does not mean decentralized, and does not mitigate the risk of exposing our user's ip addresses to UD. Our preference would be for your team to create a npm module that contains the constants necessary, linked to an open source repository that can be validated by our team to contain neutral code/constants/etc. Then your team may update those constants and publish new versions which can then be upgraded in our codebase and a reviewer can validate the changes. Just so you're aware, once the requested changes have been made i'll be doing further review of your integration into the send slice. I would leave additional inline comments everywhere where they may be some context needed for proper understanding of what the code is doing. We audited the ENS integration and no centralized services are hit to perform the reverse lookup or resolutions. We would expect UD to meet the same standards to be included in the code. |
|
@brad-decker quick question. Our unit tests we wrote are passing locally but when running as part of the suite on CircleCI we are getting an 'ENETUNREACH' - have you experienced this before? How could we alter our tests to prevent this from occuring |
|
@bbehrman10 unit tests should not hit the network, you can't test network integrations with unit tests. You'll need to mock out those network requests. The easiest way to do that, if the network request happens in the background, is to mock the background action call such that the code that makes the network request is never reached. If the network requests happen in the UI you'll need to use nock or similar to replace those network calls. You can do nock in the background as well if you'd prefer the background call to occur naturally in tests... ultimately though if you replace those network calls with flat files from a npm module as discussed above you should be good. Other than retrieving TLDS and keys what other network calls are made that aren't to the ethereum client? |
|
@brad-decker gotcha that makes sense. Okay we will remove those network calls altogether. Thanks! |
|
@brad-decker hey! We are ready for you to take another look. We have moved the TLDs and Resolver Keys to this NPM package: https://www.npmjs.com/package/@unstoppabledomains/tldsresolverkeys - Docs here: https://github.com/unstoppabledomains/tlds-and-resolver-keys-package. We have removed all network calls aside from the same calls to infura that metamask is making. |
|
@bbehrman10 I think it is best for us to split up some of this work. I think a good first PR would be renaming all of the ENS specific selectors, state keys, components to use the generic term domain like you have done in this PR. We can get that landed and then layer on some of this work. I'm going to still do a full review of this pr but I know I'm going to ask for that, so if you wanted to get a jump start I think it would be worthwhile. |
|
@brad-decker sounds good! If you can get us those other comments ASAP that would be great. |
|
@brad-decker - So we combined the resolution modules for ENS and UNS, and put them into a single state slice called domainState. Something interesting is going on with the testing. So when we run the unit tests they all do pass, but the actual testing process timeouts before it prints out the summary. We are also passing both chrome and firefox e2e tests. You'll see these behaviors on the circleCI tests as well. Should we disregard this timeout issue? We've pushed these changes for when you're ready to take a look. Thanks so much! |
|
@bbehrman10 what was the status of splitting off some of this work to make the review less complex? |
|
@brad-decker - we made a new PR for the name changes requested: #16166 |
* Updating controller dependency * fix * fix * fix * fix * fixes * Lavamoat auto * Update URLs for phishing detection testcase * update lavamoat files * call phishingController.test synchronously again * bump @metamask/controllers to v32.0.1 * lint * update policy files * bump controllers version again * modify update phishing list strategy * revert back to use isOutOfDate, but without blocking substream * possible way to fix e2e tests? * enable testing * Remove promise return from setupController in background.js, as it is no longer used * Ensure updatePhishingLists is called in MM contrller constructer, so that phishing lists are updated right away Co-authored-by: seaona <[email protected]> Co-authored-by: Alex <[email protected]> Co-authored-by: Dan Miller <[email protected]>
MetaMask#16102) * Remove ETH badge from NetworkAccountBalanceHeader when on non-ETH networks * Network specific icons, or fallback, in the network-account-balance-header * Update snapshots * Code cleanup Co-authored-by: Dan Miller <[email protected]>
|
@brad-decker Just rebased it. I did my best to sort through what ended up being hundreds of merge conflicts. I think our plan is to start a different PR off of the current develop branch in case the merging had any issues. I'll try to break down what pieces that will be added so we can collaborate on splitting this PR up into more manageable chunks. So firstly we have to install our UD resolution library as well as the TLD/Resolver Key library From our end it looks like we could split this into 3 chunks:
How does that sound? |
|
Lets save anything to do with token resolution for the end, we don't currently do that at all and those changes to the send slice were the most alarming to me when reviewing this PR. The easiest to land would be the IPFS domain resolution, followed shortly thereafter by the recipient resolution that we already do. Once those two land you are at feature parity with what we currently support for ENS. |
|
sounds good, we will link to the new PR when it's ready for you to take a look at. Thanks Brad! |
|
@brad-decker we're trying to add the UD resolution package via yarn, but when running yarn dist we get a lavamoat error. We went through the process outlined in the github (setup, lavamoat:auto, etc), but still get the same package not in allowlist error. Are there new steps in order to add dependencies? |
|
Typically this means you need to add something to policy-overrides files, what is the exact output you get |
|
@brad-decker these are the errors that are occurring. |
|
That typically means that in the policy-override.json file we need to add a key to the "builtins" option for "gulp-livereload>event-stream" |
|
Isn't that what the lavamoat commands are for? Rewriting those policy files so one doesn't have to manually put those keys in? |
|
There are two files "policy.json" and "policy-overrides.json" lavamoat:auto will build a policy file, so don't modify the 'policy.json' file. Not all dependency usages can be statically analyzed, and require a manual process. See https://github.com/LavaMoat/LavaMoat/blob/main/docs/policy.md#using-policy-overridejson-to-expand-or-limit-access |
|
hmm okay will take a look. I think I'm just a bit confused because we didn't have to do this before. Would you happen to have a bit of time to go over this. I know the holiday makes this week a bit complicated, but would early next week be okay to chat about this for 15/20 minutes? |
|
@brad-decker is there a day or time that works well for you? |
|
IPFS Pull Request Ready for Review: #16664 |
|
@bbehrman10 could you push up your changes that are not passing lavamoat:auto ? its pretty hard to be helpful when looking at this PR because its so out of date. |
|
This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions. |
|
This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions. |
|
This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions. |
|
This PR was closed because there has been no follow up activity in the last 14 days. Thank you for your contributions. |
Explanation
Implements Unstoppable Domains Resolution Library
More Information
Responds to issue #12990
Manual Testing Steps
External MetaMask + UD Resolution Testing.pdf
Testing Address Resolution
It should resolve for different currencies and tokens like MATIC & USDT
It should throw an error if
It should throw a warning if
Testing IPFS Website Resolution
Pre-Merge Checklist
+ If there are functional changes: