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: scoped session properties #5687

Merged
merged 7 commits into from
Mar 10, 2025
Merged

feat: scoped session properties #5687

merged 7 commits into from
Mar 10, 2025

Conversation

ganchoradkov
Copy link
Member

@ganchoradkov ganchoradkov commented Mar 10, 2025

Description

Added scopedProperties to sign sessions. They are like sessionProperties but scoped to the namespaces in the session

Type of change

  • Chore (non-breaking change that addresses non-functional tasks, maintenance, or code quality improvements)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Draft PR (breaking/non-breaking change which needs more work for having a proper functionality [Mark this PR as ready to review only when completely ready])
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How has this been tested?

tests, canary 2.19.0-canary-sp-0

Checklist

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

Additional Information (Optional)

Please include any additional information that may be useful for the reviewer.

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@ganchoradkov ganchoradkov requested a review from Copilot March 10, 2025 12:12
@arein arein added the accepted label Mar 10, 2025

Choose a reason for hiding this comment

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

PR Overview

This PR introduces support for scopedProperties in session handling by updating connection and approval flows, extending type definitions, and adding tests to validate correct behavior.

  • Adds scopedProperties to connection and approval parameters and their validations
  • Updates types across modules to include scopedProperties
  • Enhances UniversalProvider and EthereumProvider to propagate scopedProperties in session-related operations

Reviewed Changes

File Description
packages/sign-client/test/sdk/client.spec.ts Added a test to verify that scopedProperties are correctly set and propagated in the session
packages/sign-client/test/sdk/validation.spec.ts Added tests to reject connections/approvals using invalid scopedProperties
packages/types/src/sign-client/engine.ts Extended EngineTypes and updated engine controller to include and validate scopedProperties
providers/universal-provider/src/UniversalProvider.ts Updated connection and namespace handling to support scopedProperties
providers/ethereum-provider/src/EthereumProvider.ts Updated the connection flow to include scopedProperties and adjusted the type for scopedProperties
packages/types/src/sign-client/jsonrpc.ts Added scopedProperties field to JSON RPC types
packages/types/src/sign-client/session.ts Updated session type definitions to include scopedProperties
packages/types/src/sign-client/proposal.ts Added scopedProperties type to proposal types
providers/universal-provider/src/types/misc.ts Extended ConnectParams to include scopedProperties
packages/sign-client/src/controllers/engine.ts Modified engine controller methods to extract, propagate, and validate scopedProperties

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (3)

packages/sign-client/test/sdk/client.spec.ts:136

  • [nitpick] Consider removing the console.log debug output from the test to avoid cluttering test run outputs.
console.log("dappSession", dappSession.scopedProperties);

providers/ethereum-provider/src/EthereumProvider.ts:85

  • [nitpick] Consider updating the type for scopedProperties to SessionTypes.ScopedProperties for consistency with other providers.
scopedProperties?: unknown;

packages/sign-client/src/controllers/engine.ts:2957

  • [nitpick] Consider revising the error message for clarity, e.g., 'Each key in ' + type + ' must have a valid non-empty value; received ' + property + ' for key ' + Object.keys(properties)[index].
`${type} must be contain an existing value for each key. Received: ${property} for key ${Object.keys(properties)[index]}`
@ganchoradkov ganchoradkov requested a review from Copilot March 10, 2025 12:59

Choose a reason for hiding this comment

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

PR Overview

This PR introduces a new feature to support session scopedProperties alongside sessionProperties across the codebase. Key changes include:

  • Updates to test suites to validate the behavior of scopedProperties on connect and approval flows.
  • Modifications in the engine controllers to include and validate scopedProperties in session creation and proposal handling.
  • Corresponding updates in provider and type definitions to support the new scopedProperties field.

Reviewed Changes

File Description
packages/sign-client/test/sdk/client.spec.ts Added integration test for setting and updating scopedProperties in sessions.
packages/sign-client/test/sdk/validation.spec.ts Added negative test cases for invalid scopedProperties handling.
packages/sign-client/src/controllers/engine.ts Updated connect, approve, and validation methods to handle scopedProperties.
providers/ethereum-provider/src/EthereumProvider.ts Integrated scopedProperties in provider connection flows.
providers/universal-provider/src/UniversalProvider.ts Extended UniversalProvider to support scopedProperties in namespaces management.
packages/types/* Updated type definitions to include scopedProperties in session and proposal types.

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

packages/sign-client/src/controllers/engine.ts:2957

  • The error message contains a grammatical error ('must be contain'). Consider updating it to 'must contain a valid value for each key' for clarity.
`${type} must be contain an existing value for each key. Received: ${property} for key ${Object.keys(properties)[index]}`

Choose a reason for hiding this comment

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

PR Overview

This PR adds support for scopedProperties in session management, enabling the use of properties scoped to namespaces in addition to the existing sessionProperties.

  • Added tests to verify proper handling and error rejection for scopedProperties.
  • Updated engine, providers, and type definitions to integrate scopedProperties into connection, approval, and session settlement workflows.

Reviewed Changes

File Description
packages/sign-client/test/sdk/client.spec.ts Added a test validating that scopedProperties are correctly set in sessions
packages/sign-client/test/sdk/validation.spec.ts Added tests to ensure connect/approve reject invalid scopedProperties
packages/sign-client/src/controllers/engine.ts Integrated scopedProperties in connect, session creation and validation logic
providers/ethereum-provider/src/EthereumProvider.ts Propagated scopedProperties from options when connecting
providers/universal-provider/src/UniversalProvider.ts Updated connection and namespace-setting logic to handle scopedProperties
providers/universal-provider/src/types/misc.ts, packages/types/src/sign-client/* Updated types to include scopedProperties in relevant interfaces

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (2)

packages/types/src/sign-client/proposal.ts:17

  • Consider aligning the type definition of ScopedProperties with SessionProperties (Record<string, string>) if only string values are expected, or document the intent to support non-string values to avoid potential inconsistencies.
type ScopedProperties = Record<string, unknown>;

packages/sign-client/src/controllers/engine.ts:2632

  • [nitpick] Ensure the error message for invalid scopedProperties is consistent with similar validations (such as in the approve flow) to provide uniform feedback to developers and maintain clarity in error handling.
throw new Error(`Scoped properties must be a subset of required/optional namespaces, received: ${JSON.stringify(scopedProperties)}, required/optional namespaces: ${JSON.stringify(requestedNamespaces)}`);
@ganchoradkov ganchoradkov changed the title feat: session properties feat: scoped session properties Mar 10, 2025
@ganchoradkov ganchoradkov marked this pull request as ready for review March 10, 2025 13:05
Copy link
Collaborator

@KannuSingh KannuSingh left a comment

Choose a reason for hiding this comment

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

lgtm

ganchoradkov and others added 2 commits March 10, 2025 17:47
@ganchoradkov ganchoradkov merged commit 70a0f6d into v2.0 Mar 10, 2025
10 checks passed
@ganchoradkov ganchoradkov deleted the feat/session-properties branch March 10, 2025 15:58
@ganchoradkov ganchoradkov mentioned this pull request Mar 12, 2025
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants