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

feat: assign a UUID to all BaseProvider requests #399

Closed
wants to merge 3 commits into from

Conversation

cryptodev-2s
Copy link

@cryptodev-2s cryptodev-2s commented Jan 22, 2025

This PR assigns a UUID to all BaseProvider requests.

@cryptodev-2s cryptodev-2s requested a review from a team as a code owner January 22, 2025 00:01
@cryptodev-2s cryptodev-2s self-assigned this Jan 22, 2025
@cryptodev-2s cryptodev-2s changed the title feat: add an uuid to all BaseProvider requests feat: add a uuid to all BaseProvider requests Jan 22, 2025
@cryptodev-2s cryptodev-2s changed the title feat: add a uuid to all BaseProvider requests feat: define a uuid to all BaseProvider requests Jan 22, 2025
@cryptodev-2s cryptodev-2s changed the title feat: define a uuid to all BaseProvider requests feat: assign a UUID to all BaseProvider requests Jan 22, 2025
@cryptodev-2s cryptodev-2s changed the title feat: assign a UUID to all BaseProvider requests feat: assign a UUID to all BaseProvider requests Jan 22, 2025
@cryptodev-2s cryptodev-2s requested a review from Gudahtt January 22, 2025 00:15
Copy link

socket-security bot commented Jan 22, 2025

Removed dependencies detected. Learn more about Socket for GitHub ↗︎

🚮 Removed packages: npm/@types/[email protected]

View full report↗︎

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

I'm surprised that this hasn't caused a problem so far, but seems like a good idea? Curious to know what others think.

@@ -208,9 +209,11 @@ export abstract class BaseProvider extends SafeEventEmitter {
const payload =
params === undefined || params === null
? {
id: uuidV4(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Do we want to do this in _rpcRequest? I see that we already provide a default value for jsonrpc if it's not given so it seems like this check for id may belong there too.

Copy link
Contributor

Choose a reason for hiding this comment

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

More importantly, can we add a test for this?

@cryptodev-2s
Copy link
Author

The PR was closed because this functionality should be managed by using an rpc middleware in the rpcMiddleware param available in providers, such as createIdRemapMiddleware, here: https://github.com/MetaMask/providers/blob/main/src/StreamProvider.ts#L58.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants