-
-
Notifications
You must be signed in to change notification settings - Fork 2
Fix ConfigBuilder.build() API inconsistency #85
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
Fix ConfigBuilder.build() API inconsistency #85
Conversation
Co-authored-by: joe10832 <103850533+joe10832@users.noreply.github.com>
joe10832
left a 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.
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.
Pull Request Overview
This PR fixes an API inconsistency in the ConfigBuilder.build() method to ensure it returns the same type as BaseWalletBuilder.build(). The change eliminates the need for users to access a nested .options property when using ConfigBuilder directly.
- Standardizes return type across all builder implementations
- Removes unnecessary object wrapper from ConfigBuilder.build()
- Includes nodeConfig in the returned configuration object
joe10832
left a 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.
joe10832
left a 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.
joe10832
left a 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.
Problem
The
ConfigBuilder.build()method was returning{ options: WalletFixtureOptions }whileBaseWalletBuilder.build()(used by wallet-specific builders likeMetaMaskConfigBuilder) was returningWalletFixtureOptionsdirectly. This created an inconsistent API where users would theoretically need to call:Root Cause
The issue was in the
ConfigBuilder.build()method implementation:Solution
Changed
ConfigBuilder.build()to returnWalletFixtureOptionsdirectly, making it consistent withBaseWalletBuilder.build(). This ensures a uniform API wherecreateOnchainTest(config.build())works regardless of the configuration path taken.Impact
{ options }pattern wasn't used anywhere in the codebaseVerification
Original prompt
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.