-
-
Notifications
You must be signed in to change notification settings - Fork 203
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
Release 287.0.0 #5182
Release 287.0.0 #5182
Conversation
- Relax NFTs metadata RPC calls ([#5134](https://github.com/MetaMask/core/pull/5134)) | ||
- We now check the number of NFTs to update against a threshold value (500) to avoid sending an excessive amount of RPC calls to fetch NFTs metadata. |
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.
No sure if this is one is truly a "Fixed" (the PR was marked as such)
Yes this PR fixes an issue we had with excessive network calls, but it also now changes the behavior when fetching NFT metadata.
Should this be a "Changed" instead, also should we make this a minor update in that case?
cc: @sahar-fehri
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 think its fine keeping it as "Fixed". There were multiple updates to the pref controller that ended up triggering that function which shouldn't have been the case. I think the main issue was that we were not checking which params got updated and we were triggering the updateNftMetadata function regardless.
The 500 threshold we added was another "safe net" to avoid making the excessive amount of RPC calls.
### Changed | ||
|
||
- Bump `@metamask/keyring-api` from `^13.0.0` to `^14.0.0` ([#5177](https://github.com/MetaMask/core/pull/5177)) | ||
- Bump `@metamask/keyring-internal-api` from `^2.0.0` to `^2.0.1` ([#5177](https://github.com/MetaMask/core/pull/5177)) | ||
- Bump `@metamask/keyring-snap-client` from `^2.0.0` to `^3.0.0` ([#5177](https://github.com/MetaMask/core/pull/5177)) |
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.
As this is the initial release what do you think about removing the Changed section
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 thought about it too, we can go without !
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.
@@ -1,6 +1,6 @@ | |||
{ | |||
"name": "@metamask/multichain-transactions-controller", | |||
"version": "0.0.0", | |||
"version": "0.0.1", |
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.
Nit: do we need to release this controller at all ?
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.
Yes, it's required for the non-EVM/multichain initiative
Co-authored-by: cryptodev-2s <[email protected]>
c84dc46
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!
Just a bump dependencies release to align the `keyring-api` recent changes everywhere.
Just a bump dependencies release to align the
keyring-api
recent changes everywhere.