-
Notifications
You must be signed in to change notification settings - Fork 7
feevault config getter #83
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
Summary of ChangesHello @tac0turtle, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new public getter function to the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
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.
Code Review
This pull request introduces a getConfig function to the FeeVault contract, allowing external callers to retrieve all configuration parameters in a single call. A corresponding test case has been added to verify the functionality. The implementation is sound, but I've suggested a refactoring to use a struct for the return value of getConfig. This will improve code clarity and make future extensions to the configuration easier to manage without breaking client integrations.
| /// @notice Return the full configuration currently stored in the contract. | ||
| function getConfig() | ||
| external | ||
| view | ||
| returns ( | ||
| address _owner, | ||
| uint32 _destinationDomain, | ||
| bytes32 _recipientAddress, | ||
| uint256 _minimumAmount, | ||
| uint256 _callFee, | ||
| uint256 _bridgeShareBps, | ||
| address _otherRecipient, | ||
| address _hypNativeMinter | ||
| ) | ||
| { | ||
| return ( | ||
| owner, | ||
| destinationDomain, | ||
| recipientAddress, | ||
| minimumAmount, | ||
| callFee, | ||
| bridgeShareBps, | ||
| otherRecipient, | ||
| address(hypNativeMinter) | ||
| ); | ||
| } |
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.
For better readability and future extensibility, consider returning a struct instead of a large tuple. This makes it easier for clients to consume the data and allows for non-breaking changes if more configuration variables are added later.
You would need to define a Config struct at the contract level like this:
struct Config {
address owner;
uint32 destinationDomain;
bytes32 recipientAddress;
uint256 minimumAmount;
uint256 callFee;
uint256 bridgeShareBps;
address otherRecipient;
address hypNativeMinter;
}Then, the function can be updated as follows. Note that this will require updating test_GetConfig in FeeVault.t.sol to handle the struct return type.
/// @notice Return the full configuration currently stored in the contract.
function getConfig() external view returns (Config memory) {
return Config({
owner: owner,
destinationDomain: destinationDomain,
recipientAddress: recipientAddress,
minimumAmount: minimumAmount,
callFee: callFee,
bridgeShareBps: bridgeShareBps,
otherRecipient: otherRecipient,
hypNativeMinter: address(hypNativeMinter)
});
}
damiannolan
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.
Works for me. Thank you!
Description
Type of Change
Related Issues
Fixes #(issue)
Checklist
Testing
Additional Notes