-
Notifications
You must be signed in to change notification settings - Fork 34
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
Conversation
…ropriate RPC endpoints
Code Coverage
|
There was a problem hiding this 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.
core/src/main/java/org/stellar/anchor/sep24/Sep24Transaction.java
Outdated
Show resolved
Hide resolved
platform/src/main/java/org/stellar/anchor/platform/rpc/NotifyOffchainFundsReceivedHandler.java
Outdated
Show resolved
Hide resolved
platform/src/main/java/org/stellar/anchor/platform/rpc/RequestOffchainFundsHandler.java
Outdated
Show resolved
Hide resolved
platform/src/main/java/org/stellar/anchor/platform/rpc/RequestOnchainFundsHandler.java
Outdated
Show resolved
Hide resolved
platform/src/main/java/org/stellar/anchor/platform/rpc/RequestOnchainFundsHandler.java
Outdated
Show resolved
Hide resolved
platform/src/main/java/org/stellar/anchor/platform/service/TransactionService.java
Show resolved
Hide resolved
platform/src/main/java/org/stellar/anchor/platform/service/TransactionService.java
Outdated
Show resolved
Hide resolved
platform/src/main/java/org/stellar/anchor/platform/service/TransactionService.java
Outdated
Show resolved
Hide resolved
...hema/src/main/java/org/stellar/anchor/api/rpc/method/NotifyOffchainFundsReceivedRequest.java
Outdated
Show resolved
Hide resolved
api-schema/src/main/java/org/stellar/anchor/api/platform/PlatformTransactionData.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/stellar/anchor/sep31/Sep31TransactionBuilder.java
Outdated
Show resolved
Hide resolved
@@ -52,6 +54,11 @@ public abstract class JdbcSepTransaction { | |||
@Column(name = "amount_fee_asset") | |||
String amountFeeAsset; | |||
|
|||
@SerializedName("fee_details") |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
I will create an issue once this PR is approved and add comments near deprecation markers pointing to the issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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