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-505] Add fee details field to transaction object of SEP-6 and SEP-24 #1429

Merged
merged 7 commits into from
Feb 9, 2024

Conversation

Ifropc
Copy link
Contributor

@Ifropc Ifropc commented Jan 25, 2024

Background

Currently, both SEP-24 and SEP-6 offers an ability for anchor to charge fees. However, fees are only present to user as a single value (transaction.fee, without an ability to break it down. This proposal addresses this issue with ability to specify all charges being made (such as fees, taxes), on top of the existing summary value.

Proposed change

Deprecate amount_fee and amount_fee_asset in favor of a new field: fee_details. This field follows the same schema as fee details defined in SEP-38 standard.

@Ifropc Ifropc changed the title Anchor 505 fee details [ANCHOR-505] Add fee details field to transaction object of SEP-6 and SEP-24 Jan 25, 2024
philipliu
philipliu previously approved these changes Jan 29, 2024
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.

LGTM, just a minor comment.

ecosystem/sep-0024.md Outdated Show resolved Hide resolved
Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

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

I don't love that we're duplicating information on GET /quote/:id and GET /transaction?id=, is this avoidable? Am I correct in understanding that we're adding this because having fee info on the quote doesn't work when the quote is indicative or a quote is not used?

ecosystem/sep-0024.md Outdated Show resolved Hide resolved
@Ifropc
Copy link
Contributor Author

Ifropc commented Jan 29, 2024

Am I correct in understanding that we're adding this because having fee info on the quote doesn't work when the quote is indicative or a quote is not used?

Yes, you are right. I think duplicating fee object for when the quote is used is the better option, otherwise we would need to omit it for when the quote is used (to avoid duplication), which can be confusing.

philipliu
philipliu previously approved these changes Feb 8, 2024
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.

LGTM

JakeUrban
JakeUrban previously approved these changes Feb 9, 2024
Copy link
Contributor

@JakeUrban JakeUrban left a comment

Choose a reason for hiding this comment

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

Approved pending the comment, but I don't feel strongly about it.

ecosystem/sep-0006.md Outdated Show resolved Hide resolved
@Ifropc Ifropc dismissed stale reviews from JakeUrban and philipliu via 0533ebb February 9, 2024 20:40
@Ifropc Ifropc merged commit 3e84814 into master Feb 9, 2024
2 checks passed
Ifropc added a commit to stellar/java-stellar-anchor-sdk that referenced this pull request Mar 15, 2024
…propriate RPC endpoints (#1283)

### 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
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