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

Changes baseFee, reserveBase, reserveIncrement to XRPAmount #3546

Closed
wants to merge 1 commit into from
Closed

Changes baseFee, reserveBase, reserveIncrement to XRPAmount #3546

wants to merge 1 commit into from

Conversation

movitto
Copy link
Contributor

@movitto movitto commented Jul 17, 2020

Addresses weird custom serialization of these fields, see
issues #3165 and #3502 on github.

Work in progress, submitting for initial feedback, will require
an amendment to transaction from the old representation to the
new.

Tests updated to pass.

Addresses weird custom serialization of these fields, see
issues #3165 and #3502 on github.

Work in progress, submitting for initial feedback, will require
an amendment to transaction from the old representation to the
new.

Tests updated to pass.
@nbougalis
Copy link
Contributor

nbougalis commented Aug 1, 2020

This change, as is, would cause massive breakage. Implementing this properly would, at the very least, require an amendment (which you've pointed out), as well as support for "dual-type" fields (U64 prior to the amendment, Amount afterwards).

I don't mean to dissuade you from pursuing this, but I think that the cost-benefit ratio is simply not favorable.

@carlhua carlhua marked this pull request as draft August 24, 2020 13:39
@carlhua
Copy link
Contributor

carlhua commented Aug 24, 2020

@movitto Thank you for your contribution! rippled can always use more contributions! However, like @nbougalis pointed out, the changes would be a breaking change. I am going to convert this PR to draft as we iterate on this.

@nbougalis
Copy link
Contributor

@movitto, I'm going to close this PR. Please feel free to reopen it at a later time if you decide to pursue this further. 🖖

@nbougalis nbougalis closed this Nov 25, 2020
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