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

chore(contracts): make _processWithdrawal() be virtual for overriding #234

Merged
merged 1 commit into from
Dec 11, 2023
Merged

chore(contracts): make _processWithdrawal() be virtual for overriding #234

merged 1 commit into from
Dec 11, 2023

Conversation

0xHansLee
Copy link
Contributor

Fixed error in build contracts: https://github.com/kroma-network/kroma/actions/runs/7162496625/job/19499546642.
I made the _processWithdrawal be virtural in FeeVault.sol, and overrode this in ValidatorRewardVault.sol.

@0xHansLee 0xHansLee requested a review from a team December 11, 2023 04:57
@0xHansLee 0xHansLee self-assigned this Dec 11, 2023
@seolaoh
Copy link
Contributor

seolaoh commented Dec 11, 2023

Do you know why the test did not run in this PR? If the test had been run, this bug would have been found in that PR.

@0xHansLee
Copy link
Contributor Author

Do you know why the test did not run in #232? If the test had been run, this bug would have been found in that PR.

Yes it should be detected in the PR. However, the ci is run only when merged to dev or main, but the PR is not merged to nither dev nor main, release/1.0.3-base. We may need to change the condition of ci running.

@seolaoh
Copy link
Contributor

seolaoh commented Dec 11, 2023

Do you know why the test did not run in #232? If the test had been run, this bug would have been found in that PR.

Yes it should be detected in the PR. However, the ci is run only when merged to dev or main, but the PR is not merged to nither dev nor main, release/1.0.3-base. We may need to change the condition of ci running.

Oh I see. We should definitely test it locally when not merging into dev or main.

seolaoh
seolaoh approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@seolaoh seolaoh left a comment

Choose a reason for hiding this comment

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

LGTM

kangsorang
kangsorang approved these changes Dec 11, 2023
View reviewed changes
Copy link
Contributor

@kangsorang kangsorang left a comment

Choose a reason for hiding this comment

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

LGTM

@0xHansLee 0xHansLee merged commit 541990e into release/v1.0.3-base Dec 11, 2023
1 check passed
@0xHansLee 0xHansLee deleted the chore/fix-contracts-build branch December 11, 2023 07:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seolaoh seolaoh seolaoh approved these changes

@kangsorang kangsorang kangsorang approved these changes

@Pangssu Pangssu Pangssu approved these changes

Assignees

@0xHansLee 0xHansLee

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@0xHansLee @seolaoh @Pangssu @kangsorang