-
-
Notifications
You must be signed in to change notification settings - Fork 183
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
Add optional networkClientId
and userAddress
args to remaining NftController
public methods
#2006
Add optional networkClientId
and userAddress
args to remaining NftController
public methods
#2006
Conversation
networkClientId
and userAddress
args to remaining NftController
public methods
b1d2243
to
6a6b6cc
Compare
…oller public methods
522997c
to
4a9ea83
Compare
df2879d
to
49563a6
Compare
ff487e7
to
d59dc18
Compare
d59dc18
to
c235299
Compare
}; | ||
|
||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore-next-line |
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.
what are we ignoring about the next line?
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.
a type error, watchNft
expects the tokenId in the first arg object to be a string. We're ignoring it precisely to test that the error is thrown for this case, since we currently don't enforce types in the extension where this is called.
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.
you could cast the fn to any
or equivalent type instead:
const erc721Result = (nftController.watchNft as any)(ERC721_NFT, type);
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 that would also work. Happy to do that if you think its better. Seems kindof the same to me
Most of these changes should be marked as BREAKING |
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.
LGTM
@@ -17,10 +17,10 @@ module.exports = merge(baseConfig, { | |||
// An object that configures minimum threshold enforcement for coverage results | |||
coverageThreshold: { | |||
global: { | |||
branches: 87.77, | |||
branches: 88.2, |
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.
💪
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.
lookin good
@metamask/assets-controllers
Changed:
watchNft
,removeNft
,removeAndIgnoreNft
,removeNftContract
,updateNftFavoriteStatus
, andcheckAndUpdateAllNftsOwnershipStatus
methods onNftController
all now accept an optional options object argument containingnetworkClientId
anduserAddress
to identify where in state to mutate.addNft
no longer accepts achainId
property in its options argument since this value can be retrieved by thenetworkClientId
property and is therefore redundant.addNftVerifyOwnership
method, have been replaced with an options object containing optional propertiesnetworkClientId
,userAddress
andsource
. This method signature is more aligned with the options pattern for passingnetworkClientId
anduserAddress
on this controller and elsewhere.checkAndUpdateSingleNftOwnershipStatus
on NftController no longer accepts achainId
in its options argument. This is replaced with an optionalnetworkClientId
property which can be used to fetch chainId.*BREAKING: The fourth argument of the
isNftOwner
method onNftController
is now an options object with an optionalnetworkClientId
property. This method signature is more aligned with the options pattern for passingnetworkClientId
on this controller and elsewhere.validateWatchNft
method onNftController
is now private.detectNfts
onNftDetectionController
now accepts a single object argument with optional propertiesnetworkClientId
anduserAddress
, rather than taking these as two sequential arguments.