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-525] Introduce NotifyAmountsAssetsUpdated RPC #1188

Conversation

philipliu
Copy link
Contributor

@philipliu philipliu commented Nov 3, 2023

Description

This introduces the NotifyAmountsAssetsUpdated RPC which allows SEP-6 implementations to update all amounts/assets when it calculates fees.

Context

The current NotifyAmountsUpdated RPC was previously created for the SEP-24 withdrawal flow where only the amount values for amount_out and amount_fee need to be updated. In SEP-6, the asset values need to be updated as well since in the case of a deposit, the amount_in asset and amount are not known at transaction creation time. In the SEP-6 withdrawal flow, the amount_out asset and amount are unknown.

The current reference implementation works around this by updating the amount_in value and asset when it requests off-chain funds which really could be down when it updates the fees initially (Sep6EventProcessor).

Additionally, by not using this RPC, the SEP-6 implementation would not need to purposely set wrong amount values when creating the transaction (Sep6Service).

Testing

  • ./gradlew test

Documentation

TODO: make the stellar-docs change if this is ok.

Known limitations

N/A

@JakeUrban
Copy link
Contributor

I'm not sure I fully understand the problem this is addressing and what the changes are.

The explanation of the notify_amounts_updated is as follows:

If onchain funds were received, but for some reason the amount_in differs from specified in the interactive flow (amount_expected), you can update amount_out and amount_fee to make them correspond to the actual amount_in. The status of the transaction in this case won't be changed and will be equal to pending_anchor.

This method is really only for an edge case, which is when the amount the client said they would send is not what was actually sent.

For 99% of cases, amounts and assets should be set when requesting and receiving funds using the following methods:

  • request_offchain_funds
  • notify_offchain_funds_received
  • request_onchain_funds
  • notify_onchain_funds_received

Why can't SEP-6 also use these methods, or why aren't these methods sufficient?

@philipliu
Copy link
Contributor Author

philipliu commented Nov 3, 2023

@JakeUrban Ah, this was a misunderstanding on my part. This all stems from the fact that I was using notify_amounts_updated to update the fees before requesting KYC. This RPC requires amount_fee and amount_out to be previously set, which is not the case for SEP-6 when transactions are created. The SEP-6 reference implementation does use those methods you listed so there's no need to introduce this new PR.

I'll close this PR.

@philipliu philipliu closed this Nov 3, 2023
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