Conversation
* Fix apple auth scope * Fix Apple auth scope test
…ession config helper
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
|
Reviewer's GuideAdds explicit session configuration utilities to the dapp client to support EOA login, including typed session parameter helpers and re‑exports, and bumps packages via a changeset. Sequence diagram for creating explicit session config during EOA loginsequenceDiagram
actor User
participant Dapp as DappClient
participant Utils as DappClientUtils
participant WalletCore
User->>Dapp: Initiate_EOA_login
Dapp->>Dapp: Collect_ExplicitSessionParams
Dapp->>Utils: createExplicitSessionConfig(params)
Utils->>Utils: Compute_deadline_and_valueLimit
Utils-->>Dapp: ExplicitSessionConfig
Dapp->>WalletCore: Start_session_with(ExplicitSessionConfig)
WalletCore-->>Dapp: Session_established
Dapp-->>User: Login_successful
Class diagram for explicit session configuration utilitiesclassDiagram
class SessionDuration {
+number days
+number hours
+number minutes
}
class NativeTokenSpending {
+bigint valueLimit
+Address_Address[] allowedRecipients
}
class ExplicitSessionParams {
+number chainId
+SessionDuration expiresIn
+Permission_Permission[] permissions
+NativeTokenSpending nativeTokenSpending
}
class ExplicitSessionConfig {
}
class Permission_Permission {
}
class Address_Address {
}
class DappClientUtils {
+createExplicitSessionConfig(params: ExplicitSessionParams): ExplicitSessionConfig
}
ExplicitSessionParams --> SessionDuration : has
ExplicitSessionParams --> NativeTokenSpending : optional
ExplicitSessionParams --> Permission_Permission : uses
NativeTokenSpending --> Address_Address : allowedRecipients
DappClientUtils --> ExplicitSessionParams : input
DappClientUtils --> ExplicitSessionConfig : output
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Consider validating
SessionDuration(e.g., disallow negative or non-integer values and guard against the all-zero case) sosessionLifetimeSecondsanddeadlinedon’t end up as unexpected values like immediate expiry or huge numbers. - In
createExplicitSessionConfig, it might be worth explicitly handling the case wherenativeTokenSpending.valueLimitis set butallowedRecipientsis omitted/empty, to clarify whether a global value limit without per-recipient rules is intentional or should be constrained.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider validating `SessionDuration` (e.g., disallow negative or non-integer values and guard against the all-zero case) so `sessionLifetimeSeconds` and `deadline` don’t end up as unexpected values like immediate expiry or huge numbers.
- In `createExplicitSessionConfig`, it might be worth explicitly handling the case where `nativeTokenSpending.valueLimit` is set but `allowedRecipients` is omitted/empty, to clarify whether a global value limit without per-recipient rules is intentional or should be constrained.
## Individual Comments
### Comment 1
<location> `packages/wallet/dapp-client/src/utils/index.ts:149-150` </location>
<code_context>
+
+export const createExplicitSessionConfig = (params: ExplicitSessionParams): ExplicitSessionConfig => {
+ const nowInSeconds = BigInt(Math.floor(Date.now() / 1000))
+ const { days = 0, hours = 0, minutes = 0 } = params.expiresIn
+ const sessionLifetimeSeconds = days * 24 * 60 * 60 + hours * 60 * 60 + minutes * 60
+ const deadline = nowInSeconds + BigInt(sessionLifetimeSeconds)
+
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle non-integer and large duration values more defensively before converting to BigInt.
`sessionLifetimeSeconds` is computed as a `number` and only converted at the end. If `expiresIn` contains non-integer values (e.g. `0.5` hours), `BigInt(sessionLifetimeSeconds)` will throw. Large durations might also exceed `Number.MAX_SAFE_INTEGER`, causing precision loss before conversion. Consider normalizing input (e.g. `Math.floor`) and/or doing the arithmetic in `BigInt` from the start, or validating that the seconds value is a safe integer before converting.
</issue_to_address>
### Comment 2
<location> `packages/wallet/dapp-client/src/utils/index.ts:148-151` </location>
<code_context>
+ const nowInSeconds = BigInt(Math.floor(Date.now() / 1000))
+ const { days = 0, hours = 0, minutes = 0 } = params.expiresIn
+ const sessionLifetimeSeconds = days * 24 * 60 * 60 + hours * 60 * 60 + minutes * 60
+ const deadline = nowInSeconds + BigInt(sessionLifetimeSeconds)
+
+ if (params.permissions.length === 0) {
</code_context>
<issue_to_address>
**suggestion:** Consider validating that the computed session lifetime is positive and non-zero.
If `days`, `hours`, and `minutes` all use their defaults, `sessionLifetimeSeconds` is `0`, so `deadline` == `nowInSeconds` and the session expires immediately. If that’s not desired, consider validating that the lifetime is > 0 and either throwing or enforcing a minimum duration.
```suggestion
const nowInSeconds = BigInt(Math.floor(Date.now() / 1000))
const { days = 0, hours = 0, minutes = 0 } = params.expiresIn
const sessionLifetimeSeconds = days * 24 * 60 * 60 + hours * 60 * 60 + minutes * 60
if (sessionLifetimeSeconds <= 0) {
throw new Error(
'createExplicitSessionConfig: Session lifetime must be greater than 0 seconds. ' +
'Please provide a non-zero duration in expiresIn.',
)
}
const deadline = nowInSeconds + BigInt(sessionLifetimeSeconds)
```
</issue_to_address>
### Comment 3
<location> `packages/wallet/dapp-client/src/utils/index.ts:160-162` </location>
<code_context>
+ const nativeTokenSpending = params.nativeTokenSpending
+ const valueLimit = nativeTokenSpending?.valueLimit ?? 0n
+ const nativeTokenReceivers = [...(nativeTokenSpending?.allowedRecipients || [])]
+ const nativeTokenSpendingPermissions = nativeTokenReceivers.map((receiver) => ({
+ target: receiver,
+ rules: [],
+ }))
+
</code_context>
<issue_to_address>
**question (bug_risk):** Clarify whether an empty `rules` array matches the intended semantics for native token spending permissions.
`nativeTokenSpending` currently maps each allowed recipient to `{ target: receiver, rules: [] }`. Depending on how `Permission` is evaluated, `rules: []` may either mean “unrestricted spend” or “no effective permission”. Consider either adding an explicit default rule or confirming that the downstream permission logic interprets an empty `rules` array exactly as intended for this use case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add helper utilities for constructing explicit session configurations for the dapp client and record patch releases for multiple packages related to EOA login support.
New Features:
Chores: