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

SEP-24: Added refunded transaction status #1195

Merged
merged 3 commits into from
May 6, 2022
Merged

Conversation

lijamie98
Copy link
Contributor

@lijamie98 lijamie98 commented May 6, 2022

Since the refunded flag is being deprecated, MGI requested the refunded status.

@lijamie98 lijamie98 changed the title Added refunded transaction status SEP-24: Added refunded transaction status May 6, 2022
Copy link
Member

@leighmcculloch leighmcculloch left a comment

Choose a reason for hiding this comment

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

One change request, otherwise looks good to me. cc @JakeUrban could you also 👀?

Updated: 2022-05-05
Version 2.3.0
Updated: 2022-05-06
Version 2.3.1
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a new feature, the addition of a new status, this should be a minor version bump. Reserve patch version bumps for bug fixes.

Suggested change
Version 2.3.1
Version 2.4.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated and pushed.

@lijamie98 lijamie98 force-pushed the 2022-05-06-add-refunded branch from 883a5d2 to db51194 Compare May 6, 2022 00:32
CassioMG
CassioMG previously approved these changes May 6, 2022
Copy link
Contributor

@CassioMG CassioMG left a comment

Choose a reason for hiding this comment

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

LGTM

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.

🤔 what is the relationship between refunded and completed_at? Can we also add a note to the deprecated refunded attribute that there is a transaction status for this now?

@lijamie98
Copy link
Contributor Author

🤔 what is the relationship between refunded and completed_at? Can we also add a note to the deprecated refunded attribute that there is a transaction status for this now?

Updated the refunded: boolean field.

I think there are a couple of options:

  1. completed_at sticks with the status completed.
  2. List the statuses that are considered 'terminal' state, such as: completed, error, refunded, no_market, too_small and too_large
  3. List the statuses that are considered terminal and non-errored states, such as completed, refunded.

Please feel free to comment if there are more sensible options.

@JakeUrban
Copy link
Contributor

I think the third option makes the most sense. Can we update the description of completed_at? Then I'll ✅

@lijamie98
Copy link
Contributor Author

lijamie98 commented May 6, 2022

I think the third option makes the most sense. Can we update the description of completed_at? Then I'll ✅

updated.

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.

4 participants