Skip to content
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.

Sign up for GitHub

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

Jump to bottom

SOV-2197 Recover incorrect fee allocation on feeSharingCollector #499

Merged
merged 8 commits into from
Jul 3, 2023
Merged

SOV-2197 Recover incorrect fee allocation on feeSharingCollector #499

merged 8 commits into from
Jul 3, 2023

Conversation

cwsnt
Copy link
Contributor

@cwsnt cwsnt commented May 24, 2023

No description provided.

@cwsnt cwsnt requested review from ororopickpocket and tjcloa May 24, 2023 00:54
@cwsnt cwsnt changed the title Recover incorrect fee allocation on feeSharingCollector SOV-2197 Recover incorrect fee allocation on feeSharingCollector May 24, 2023
tjcloa
tjcloa reviewed May 24, 2023
View reviewed changes
Copy link
Contributor

@tjcloa tjcloa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing a test for revert sending RBTC which could be the case if the receiver is a contract: "FeeSharingCollector::recoverIncorrectAllocatedFees: Withdrawal rbtc failed"

ororopickpocket
ororopickpocket approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@ororopickpocket ororopickpocket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general it would be better to read the owner only once and store it locally to save gas, but because this is a one-time function it doesn't really matter

@cwsnt cwsnt requested a review from tjcloa May 29, 2023 06:28
@cwsnt cwsnt changed the base branch from SOV-2181-fix-fee-sharing-withdraw to development May 29, 2023 08:02
@cwsnt cwsnt merged commit 3121de4 into development Jul 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tjcloa tjcloa tjcloa approved these changes

@ororopickpocket ororopickpocket ororopickpocket approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cwsnt @ororopickpocket @tjcloa