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

[ANCHOR-603] Add fee_details to SEP-6/24/31 Transaction object and appropriate RPC endpoints #1283

Merged
merged 8 commits into from
Mar 15, 2024

Conversation

Ifropc
Copy link
Contributor

@Ifropc Ifropc commented Mar 5, 2024

Description

Add protocol changes introduced in stellar/stellar-protocol#1441 and stellar/stellar-protocol#1429

Context

Previously, protocol was lacking ability to break down fees being charged by the anchor. This PR adds this functionality

Testing

  • ./gradlew test

Documentation

stellar/stellar-docs#349

Known limitations

N/A

@Ifropc Ifropc changed the title ANCHOR-603: Add fee_details to SEP-6/24/31 Transaction object and appropriate RPC endpoints [ANCHOR-603] Add fee_details to SEP-6/24/31 Transaction object and appropriate RPC endpoints Mar 5, 2024
Copy link

github-actions bot commented Mar 5, 2024

Code Coverage

Overall Project NaN% NaN% 🍏

There is no coverage information present for the Files changed

@Ifropc Ifropc requested a review from philipliu March 6, 2024 00:02
Copy link
Contributor

@philipliu philipliu left a comment

Choose a reason for hiding this comment

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

The change looks good. I just have a few minor comments about error messages and testing.

@@ -52,6 +54,11 @@ public abstract class JdbcSepTransaction {
@Column(name = "amount_fee_asset")
String amountFeeAsset;

@SerializedName("fee_details")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove the amountFee and implement setAmountFee and getAmountFee to get and set the fee_details?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's trivial. We would need to merge 2 columns into a new JSON column + currently some places in codebase allows to only update amount_fee (without even knowing fee_asset), while when updating fee_details we need to know both amount_fee and fee_asset (which in turn means we need to fetch transaction from DB)
I think we can look into merging columns into a new JSON column in v3 when we drop support for a separate amount_fee and fee_asset fields

@Ifropc
Copy link
Contributor Author

Ifropc commented Mar 13, 2024

Can we add an issue to remove the @deprecated fileds?

I will create an issue once this PR is approved and add comments near deprecation markers pointing to the issue

Copy link
Collaborator

@lijamie98 lijamie98 left a comment

Choose a reason for hiding this comment

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

LGTM

@Ifropc Ifropc merged commit 8e2e1c6 into develop Mar 15, 2024
7 checks passed
@Ifropc Ifropc deleted the ANCHOR-603-fee-details branch March 15, 2024 00: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.

None yet

3 participants