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

feat: Update SetFee with new syntax pending XRPFee amendment #634

Merged
merged 8 commits into from
Sep 6, 2023

Conversation

JST5000
Copy link
Contributor

@JST5000 JST5000 commented Aug 22, 2023

High Level Overview of Change

With the XRPFee amendment, the SetFee Psuedotransaction will have new fields.

Context of Change

Ideally, the old fields should still be usable up until the amendment is enabled, and for historical analysis of the ledger it should still be able to interpret older transactions as SetFee transactions. But we want to enforce that only one of the new syntaxes is allowed, not any combination of fields. (Meaning it should not be possible to instantiate a SetFee which has BOTH a base_fee AND a base_fee_drops)

Useful to see how this was done in xrpl.js: XRPLF/xrpl.js#2357

Type of Change

  • New feature (non-breaking change which adds functionality)
  • Documentation Updates

Test Plan

CI Passes

@JST5000 JST5000 marked this pull request as ready for review August 22, 2023 21:33
@JST5000
Copy link
Contributor Author

JST5000 commented Aug 22, 2023

Open to other suggestions about how to handle the typing.

My other ideas didn't pan out (Overloading constructors isn't done in Python, having a type alias named SetFee wouldn't be backwards compatible as it doesn't work as a constructor, and doing something fancy with multiple inheritance also didn't seem to work from my attempts because overloading constructors didn't work)

So, this implementation has the fields set as optional, but has explicit runtime checks and documentation to hopefully guide the user to picking the right fields if they're doing it by hand, and enforce the same "Required" rules the type checker normally would.

@JST5000 JST5000 requested a review from ckniffen August 28, 2023 23:41
@JST5000 JST5000 requested review from mvadari and pdp2121 September 5, 2023 21:04
@JST5000 JST5000 merged commit a1e7ec5 into master Sep 6, 2023
@JST5000 JST5000 deleted the xrp-fee-amendment branch September 6, 2023 19:03
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.

4 participants