-
Notifications
You must be signed in to change notification settings - Fork 8
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
(BEDS-875) Add rewards chart history based on premium perks #1220
base: staging
Are you sure you want to change the base?
Conversation
517ce8d
to
c6d5fa4
Compare
Deploying beaconchain with Cloudflare Pages
|
ChartRewards | ||
) | ||
|
||
func (d ChartType) Int() int { |
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.
What is the int value that we pass is invalid?
Should we have a function that checks the valid number
func (d ChartType) IsValid() bool {
return d >= ChartDefault && d <= ChartRewards
}
and maybe a function that stringify for debugging and logging purposes?
func (d ChartType) String() string {
switch d {
case ChartDefault:
return "Default"
case ChartRewards:
return "Rewards"
default:
return "Unknown"
}
}
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.
What is the int value that we pass is invalid?
This should be covered here already:
beaconchain/backend/pkg/api/enums/enums.go
Lines 15 to 17 in 2ca36e5
func IsInvalidEnum(e Enum) bool { | |
return e.Int() == -1 | |
} |
and maybe a function that stringify for debugging and logging purposes?
It's meant as an internal type only, thought it's cleaner than passing strings or numbers. It doesn't depend on user params and isn't logged anywhere, so I think that's not needed.
@remoterami Ideally to cover the changes with tests. Unit tests are nice, but api tests can be fine. |
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.
lgtm
8e79801
to
362046d
Compare
I've added some tests and did some refactoring. Could probably further solidify and generalize the testing setup, but that depends on our coverage goals and I didn't want to over-engineer for now. Open to suggestions though |
No description provided.