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.

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

[FEA-792] added multiplier #127

Merged
merged 4 commits into from
Dec 19, 2024
Merged

[FEA-792] added multiplier #127

merged 4 commits into from
Dec 19, 2024

Conversation

Purva-Chaudhari
Copy link
Member

What's new in this PR?

Add multiplier for flight class

@Purva-Chaudhari
Copy link
Member Author

ran forge fmt

@Purva-Chaudhari Purva-Chaudhari requested a review from eyqs December 17, 2024 19:37
@Purva-Chaudhari Purva-Chaudhari changed the title added multiplier [FEA-792] added multiplier Dec 17, 2024
} else if (flightClass == 5) {
multiplier = 300; // 3x (scaled by 100)
} else if (flightClass == 6) {
multiplier = 500; // 5x (scaled by 100)
Copy link
Member

Choose a reason for hiding this comment

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

Add comments on what each flight class is (Economy, Plus, etc.)

Copy link
Member

Choose a reason for hiding this comment

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

There must be a more efficient way of doing this? e.g. uint256[] constant multipliers = [0, 1, 11, 125, 200, 300, 500]; - check if this actually saves gas

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe something like this.

uint256[6] memory multipliers = [100, 110, 125, 200, 300, 500];
    
if (flightClass == 0 || flightClass > 6) {
    revert InvalidFlightClass(flightClass);
}
    
return (baseAmount * multipliers[flightClass - 1]) / 100;```

Copy link
Member

@eyqs eyqs left a comment

Choose a reason for hiding this comment

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

Can you install the latest version of Foundry and run forge fmt again?

} else if (flightClass == 5) {
multiplier = 300; // 3x (scaled by 100)
} else if (flightClass == 6) {
multiplier = 500; // 5x (scaled by 100)
Copy link
Member

Choose a reason for hiding this comment

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

There must be a more efficient way of doing this? e.g. uint256[] constant multipliers = [0, 1, 11, 125, 200, 300, 500]; - check if this actually saves gas

@Purva-Chaudhari
Copy link
Member Author

made it to array, for 6 classes it's better

Upgraded forge

Copy link
Member

@eyqs eyqs left a comment

Choose a reason for hiding this comment

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

LGTM

@Purva-Chaudhari
Copy link
Member Author

Purva-Chaudhari commented Dec 18, 2024

Class 6 if else is better, Class 9 and above array works better
image

@Purva-Chaudhari Purva-Chaudhari merged commit 18e9346 into main Dec 19, 2024
1 check passed
@Purva-Chaudhari Purva-Chaudhari deleted the multiplier branch December 19, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants