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-704] Clean up amount_fee and document user_action_required_by #910

Closed
wants to merge 4 commits into from

Conversation

JiahuiWho
Copy link
Contributor

This is a resubmit of #857 due to rebase

  1. Clean up deprecated amount_fee and update platform transaction schema
  2. Add user_action_required_by to related RPC methods
    NotifyInteractiveFlowCompletedRequest
    NotifyTransactionOnHoldRequest
    NotifyOffchainFundsPendingRequest
    NotifyOffchainFundsAvailableRequest
    RequestCustomerInfoUpdateRequest
    RequestOffchainFundsRequest
    RequestOnchainFundsRequest
    RequestTrustRequest
  3. Add use instruction in SEP-6 and SEP-24
    platforms/anchor-platform/admin-guide/sep6/configuration.mdx
    platforms/anchor-platform/admin-guide/sep24/configuration.mdx

@stellar-jenkins
Copy link

@ElliotFriend
Copy link
Contributor

ElliotFriend commented Aug 14, 2024

Looks good from what I can tell after a quick look. Just want to make sure, though, this is still needed for v2.x anchor platform, right? (i'm working on the versioning setup in #909, so it's front of mind for me)

Comment on lines -1067 to +1094
amount_fee:
$ref: '#/components/schemas/Amount'
fee_details:
$ref: '#/components/schemas/FeeDetails'
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we remove amount_fee from the response we return? I think it should still be there to avoid breaking wallets that still use it. It should just always equal fee.total

Copy link
Contributor

Choose a reason for hiding this comment

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

This is platform response, not sep response. We don't have openapi spec for SEP responses (though IMO we should)
For wallets yes amount_fee is still there.
Even in platform amount_fee is still there, just deprecated. We always remove deprecated fields from docs

Copy link
Contributor Author

@JiahuiWho JiahuiWho Aug 14, 2024

Choose a reason for hiding this comment

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

amount_fee is removed in 3.0 https://stellarorg.atlassian.net/browse/ANCHOR-636 as breaking change
edit from Gleb: removed from platform transaction, not from sep transaction

Comment on lines 116 to 117
`SEP24_INITIAL_USER_DEADLINE_SECONDS` is the time in seconds a user has to act before the transaction automatically moves to the next status. It determines the `user_action_required_by` field, which indicates the deadline for statuses with automatic movement. Check [JSON-RPC Methods][json-rpc-methods] for usage examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

I must have missed the PR implementing user_action_required_by so I'm missing context, but why do we have a configurable default value for this? Wouldn't the time the user has to act be highly dependent on the specific situation? I would've thought we would always require the business to set the time in each RPC request.

Also when you say "automatically moves to the next status", the business still has to make an RPC request to move the transaction to the next status right? The anchor platform doesn't monitor the time and move the transaction status on its own.

Copy link
Contributor

Choose a reason for hiding this comment

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

but why do we have a configurable default value for this

When we create transaction and it's in incomplete status, we might want to set an expiration time right from the start. This is the "default" value (probably better to name it "starting" value)

The anchor platform doesn't monitor the time and move the transaction status on its own.

This is correct @JiahuiWho business needs to monitor expiration time for transaction on their own. They should use GET /transactions and can order_by user_action_required_by field

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you min adding an example of GET /transactions usage?

@Ifropc
Copy link
Contributor

Ifropc commented Aug 14, 2024

Just want to make sure, though, this is still needed for v2.x anchor platform, right?

Yes, I asked Jiahui to wait a bit with this PR to not mess up your reorganizing PR 🙏🏼

@stellar-jenkins
Copy link

Something went wrong with PR preview build please check

@stellar-jenkins
Copy link

Copy link
Contributor

@Ifropc Ifropc left a comment

Choose a reason for hiding this comment

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

Let's wait until versioning pr is merged

@ElliotFriend
Copy link
Contributor

Hi, @JiahuiWho! #909 is now merged, so you should be ready to get this ready now. Two things you'll probably want to note:

  1. I made a (longer-than-expected) CONTRIBUTING.md doc for Anchor Platform docs. It's got some pretty helpful information as far as links, versioned docs, plugin instances, etc.
  2. Now that the "latest" version v2.8.4 has been "released," any changes for the v2.x docs will need to take place in a different set of files/directories. That's all outlined in the contributing doc linked above. If you have any questions, or need any help, I'm happy to assist.

@Ifropc
Copy link
Contributor

Ifropc commented Aug 20, 2024

@JiahuiWho we probably want to start this PR fresh off the current main branch

@stellar-jenkins
Copy link

@JiahuiWho JiahuiWho closed this Aug 21, 2024
@JiahuiWho JiahuiWho deleted the anchor-704-new branch August 21, 2024 18:26
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.

5 participants