feat(across): Add Setter for LP Fee %/second in BridgePool #3622
Conversation
| * @param _newLpFeeRatePerSecond The new rate to set. | ||
| */ | ||
| function changeLpFeeRatePerSecond(uint64 _newLpFeeRatePerSecond) public override nonReentrant() { | ||
| require(msg.sender == address(bridgeAdmin)); |
There was a problem hiding this comment.
nit: should we make this a modifier?
nit: this should have a require message?
nit: should the natspec say only the admin can call this?
There was a problem hiding this comment.
nit: should we make this a modifier?
yeah, slightly improves readability let's do that.
nit: this should have a require message?
It definitely should have a require message, will add.
nit: should the natspec say only the admin can call this?
Will add
| */ | ||
| function changeLpFeeRatePerSecond(uint64 _newLpFeeRatePerSecond) public override onlyBridgeAdmin() nonReentrant() { | ||
| lpFeeRatePerSecond = _newLpFeeRatePerSecond; | ||
| emit LpFeeRateChanged(lpFeeRatePerSecond); |
There was a problem hiding this comment.
Do you think it'd be useful to include the old LP fee rate in this event?
There was a problem hiding this comment.
Not really because all historical LP fee rates are captured by this event, since its also emitted in the constructor.
There was a problem hiding this comment.
I think it's fine the way it is. When digging through events in the past, I remember wanting this info (for instance, in the store), but I think this is fine, as you can always find the event before if you need to as you said.
| @@ -130,6 +130,11 @@ contract BridgeAdmin is BridgeAdminInterface, Ownable, Lockable { | |||
| emit BridgePoolsAdminTransferred(bridgePools, newAdmin); | |||
| } | |||
|
|
|||
| function changeLpFeeRatePerSecond(address bridgePool, uint64 newLpFeeRate) public onlyOwner nonReentrant() { | |||
…ol#3622) * feat(across): Add Setter for LP Fee %/second in BridgePool * Update BridgePool.sol * Add natspec comments * Change name from Change to Set * Update BridgePool.sol Signed-off-by: Dmitry Vasilev <dmitry.vasil@gmail.com>
Motivation
This rate is not easy to set and forget and should be configurable by Bridge admin.
Testing
Check a box to describe how you tested these changes and list the steps for reviewers to test.