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

Smart Transactions List #13

Merged
merged 17 commits into from
Nov 3, 2021
Merged

Smart Transactions List #13

merged 17 commits into from
Nov 3, 2021

Conversation

meppsilon
Copy link
Contributor

No description provided.

}

if (smartTransaction.status === SmartTransactionStatuses.RESOLVED) {
// remove smart transaction
Copy link
Collaborator

Choose a reason for hiding this comment

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

What if we just hide it instead of removing it after it was resolved? That way if we want to have all STX in this controller in the future (instead of pushing into a list of regular transactions), we would already have all historical records for a user.

return;
}

// if (smartTransaction.status === SmartTransactionStatuses.RESOLVED) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this commented code?

@dan437
Copy link
Collaborator

dan437 commented Nov 3, 2021

Thanks a lot for keeping unit tests coverage high! Once the build is green, let's merge.

@meppsilon meppsilon marked this pull request as ready for review November 3, 2021 16:48
@meppsilon meppsilon requested a review from a team as a code owner November 3, 2021 16:48
@meppsilon meppsilon merged commit 2b2a19e into main Nov 3, 2021
@meppsilon meppsilon deleted the stx-list branch November 3, 2021 16:48
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.

2 participants