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

Fee error types #110

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Conversation

Shadouts
Copy link
Contributor

@Shadouts Shadouts commented Jan 4, 2024

In order to provide more flexible uses for fee validation, this change introduces fee error types.

Previously, fee validation only returned strings. With validation functions which return an error type, applications using this library will be able to provide their own user feedback based on error type.

The new type FeeValidationError is a typescript enum collecting fee validation error categories currently being returned by validateFee and validateFeeRate functions.

To preserve backward compatibility, validateFee and validateFeeRate functions are now merely wrappers for more granular functions which return pieces of an error. These new functions are: checkFeeError, checkFeeRateError, and getFeeErrorMessage.

I'm not entirely happy with the naming. I think the ideal names for the more "raw" validation feedback that checkFeeError and checkFeeRateError perform would have used "validate", but validate was already used for the existing functions which return the error message. I'm open to suggestions.

Copy link
Contributor

@bucko13 bucko13 left a comment

Choose a reason for hiding this comment

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

Couple small nits. Overall I like the change. I see what you mean about the names, but I'm fine with what you've got for now until we're ready to make breaking changes.


switch (error) {
case FeeValidationError.FEE_CANNOT_BE_NEGATIVE:
errorMessage = "Fee cannot be negative.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't these just return the string at the end of each case?

Copy link
Contributor Author

@Shadouts Shadouts Jan 4, 2024

Choose a reason for hiding this comment

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

They could be, but I don't like using returns in switch statements. There is already a conventional command to interrupt switch statement fall-through. Introducing a different one isn't needed and may result in a bigger change in the future if modifying the switch statement or function. If this single exit-point pattern isn't acceptable, I'd probably change the function to use if statements, but I think the switch is more performant when checking this type.

Choose a reason for hiding this comment

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

could I convince you to use an object mapping here then?

const feeErrorMessages = {
  [FeeValidationError.FEE_CANNOT_BE_NEGATIVE]: "Fee cannot be negative.",
  [FeeValidationError.FEE_RATE_CANNOT_BE_NEGATIVE]: "Fee rate cannot be negative.",
  // ... other mappings
};

export function getFeeErrorMessage(error: FeeValidationError | null): string {
  return error !== null ? feeErrorMessages[error] ?? "" : "";
}

Choose a reason for hiding this comment

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

oh nevermind lol

Copy link
Contributor

Choose a reason for hiding this comment

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

whoops. sorry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing released yet so we can revert

Copy link
Contributor

Choose a reason for hiding this comment

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

The mapping is nice.

Choose a reason for hiding this comment

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

Six of one, a half dozen of the other. It's all the same to me, really.

src/fees.ts Outdated Show resolved Hide resolved
Co-authored-by: buck <[email protected]>
@Shadouts Shadouts requested a review from bucko13 January 4, 2024 18:27
@bucko13 bucko13 merged commit 52c173d into unchained-capital:master Jan 4, 2024
3 checks passed
@Shadouts Shadouts deleted the fee-error-types branch January 4, 2024 18:37
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