-
Notifications
You must be signed in to change notification settings - Fork 7
Make CreateSessionKey parameters optional with defaults #161
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
Conversation
Updated the CreateSessionKey method to provide default values for its parameters, allowing them to be optional. This improves usability by enabling callers to omit parameters, which will then be set to sensible defaults within the method.
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Thirdweb/Thirdweb.Wallets/SmartWallet/SmartWallet.cs (1)
393-402: Update XML documentation to reflect optional parameters and their defaults.The documentation should indicate that parameters beyond
signerAddressare optional and describe the default values applied when omitted. This helps developers discover the simplified API.Consider updating the XML comments like this:
/// <summary> /// Creates a new session key for a signer to use with the smart account. /// </summary> /// <param name="signerAddress">The address of the signer to create a session key for.</param> - /// <param name="approvedTargets">The list of approved targets for the signer. Use a list of a single Constants.ADDRESS_ZERO to enable all contracts.</param> - /// <param name="nativeTokenLimitPerTransactionInWei">The maximum amount of native tokens the signer can send in a single transaction.</param> - /// <param name="permissionStartTimestamp">The timestamp when the permission starts. Can be set to zero.</param> - /// <param name="permissionEndTimestamp">The timestamp when the permission ends. Make use of our Utils to get UNIX timestamps.</param> - /// <param name="reqValidityStartTimestamp">The timestamp when the request validity starts. Can be set to zero.</param> - /// <param name="reqValidityEndTimestamp">The timestamp when the request validity ends. Make use of our Utils to get UNIX timestamps.</param> + /// <param name="approvedTargets">Optional. The list of approved targets for the signer. Defaults to Constants.ADDRESS_ZERO (enables all contracts) if null.</param> + /// <param name="nativeTokenLimitPerTransactionInWei">Optional. The maximum amount of native tokens the signer can send in a single transaction. Defaults to "0" if null.</param> + /// <param name="permissionStartTimestamp">Optional. The timestamp when the permission starts. Defaults to "0" (immediate) if null.</param> + /// <param name="permissionEndTimestamp">Optional. The timestamp when the permission ends. Defaults to 10 years in the future if null.</param> + /// <param name="reqValidityStartTimestamp">Optional. The timestamp when the request validity starts. Defaults to "0" (immediate) if null.</param> + /// <param name="reqValidityEndTimestamp">Optional. The timestamp when the request validity ends. Defaults to 10 years in the future if null.</param>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Thirdweb/Thirdweb.Wallets/SmartWallet/SmartWallet.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
Thirdweb/Thirdweb.Wallets/SmartWallet/SmartWallet.cs (2)
Thirdweb/Thirdweb.Utils/Utils.cs (2)
Utils(24-1207)GetUnixTimeStampIn10Years(213-216)Thirdweb/Thirdweb.Utils/Constants.cs (1)
Constants(3-55)
🔇 Additional comments (1)
Thirdweb/Thirdweb.Wallets/SmartWallet/SmartWallet.cs (1)
405-410: LGTM! Well-designed optional parameters.The parameter defaults are backward-compatible and improve usability by allowing callers to omit commonly-defaulted values.
Updated the CreateSessionKey method to provide default values for its parameters, allowing them to be optional. This improves usability by enabling callers to omit parameters, which will then be set to sensible defaults within the method.
PR-Codex overview
This PR focuses on updating the
CreateSessionKeymethod in theSmartWalletclass to allow optional parameters with default values, enhancing flexibility when creating session keys.Detailed summary
CreateSessionKeyto have default values (set tonull).ApprovedTargetsto use a default list ifapprovedTargetsisnull.nullvalues, providing default fallbacks.Summary by CodeRabbit