-
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-809][SEP-31] Add funding_method to SEP-31 #1581
Conversation
@@ -23,6 +24,8 @@ public static class ReceiveOperation { | |||
|
|||
@SerializedName("max_amount") | |||
Long maxAmount; | |||
|
|||
List<String> methods; |
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.
Do we call this methods
here so that it's consistent with the SEP-6 asset configuration?
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.
Actually this is to match the naming in asset config, for data binding
sep31:
enabled: false
receive:
min_amount: 0
max_amount: 1000000
methods:
- SEPA
- SWIFT
@@ -34,9 +34,13 @@ public class Sep31PostTransactionRequest { | |||
@SerializedName("receiver_id") | |||
String receiverId; | |||
|
|||
Sep31TxnFields fields; | |||
@Deprecated Sep31TxnFields fields; |
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.
This is going into 3.0, right? Can we remove the field instead?
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.
Yes this is going to 3.0 but no we cannot remove it. I tried to do so in the Protocol but Jake insist that we should keep it and continue support, and so far there isn't a plan to get rid of it :/
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 believe there are a number of changes going into 3.0 that are deprecated in the protocol but we decided to remove in 3.0. wdyt @lijamie98
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.
While I’d love to remove all those long-deprecated fields, I don’t think we can get everyone on the same page right now with the 3.0 release so close.
A more practical approach might be to create a plan after the 3.0 release, outlining the removal timeline, the specific fields to be deprecated, and a guide for identifying their replacements or alternatives.
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.
Sounds good. Let's merge this change as is and discuss whether we want to remove these fields in 3.x or 4.0.
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.
* methods. | ||
*/ | ||
public static void validateFundingMethod( | ||
String assetID, String method, List<String> supportedMethods) throws AnchorException { |
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 throw BadRequestException
instead of AnchorException
?
By the way, |
Description
funding_methods
to/info
response. This field is mandatory for the anchor.funding_method
toPOST /transaction
param. This field is mandatory for wallet.Context
SEP-31 changes scoped in stellar/stellar-protocol#1567
Testing
./gradlew test
Next
The SEP-6 change will be handled in a separate PR.