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.

Sign up for GitHub

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

Jump to bottom

fix: exclude active exiting utxos from calls to /account.get_exitable_utxos #1505

Merged
merged 6 commits into from
May 19, 2020
Merged

fix: exclude active exiting utxos from calls to /account.get_exitable_utxos #1505

merged 6 commits into from
May 19, 2020

Conversation

pgebal
Copy link
Contributor

@pgebal pgebal commented May 12, 2020

Closes https://github.com/omisego/security-issues/issues/30

Overview

Active exiting utxos are excluded from /account.get_exitable_utxos response.

@pgebal pgebal changed the title Pgebal/fix standard exited utxos not recognized as unspendable fix: exclude active exiting utxos from calls to /account.get_exitable_utxos May 12, 2020
@pgebal pgebal changed the title fix: exclude active exiting utxos from calls to /account.get_exitable_utxos fix: exclude active exiting utxos from calls to /account.get_exitable_utxos May 12, 2020
@pgebal pgebal force-pushed the pgebal/fix_standard_exited_utxos__not_recognized_as_unspendable branch from 968649f to 24163df Compare May 12, 2020 15:24
@coveralls
Copy link

coveralls commented May 12, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.04%) to 78.384% when pulling 1dda013 on pgebal/fix_standard_exited_utxos__not_recognized_as_unspendable into 4ab2549 on master.

@pgebal pgebal force-pushed the pgebal/fix_standard_exited_utxos__not_recognized_as_unspendable branch 2 times, most recently from 21028db to f39a246 Compare May 12, 2020 16:08
@pgebal pgebal requested a review from pnowosie May 12, 2020 17:01
pgebal
pgebal commented May 12, 2020
View reviewed changes
apps/omg_watcher/test/omg_watcher/api/account_test.exs Outdated
@@ -63,5 +66,44 @@ defmodule OMG.Watcher.API.AccountTest do

assert %{blknum: ^blknum, txindex: ^txindex, oindex: ^oindex} = utxo
end

@tag fixtures: [:db_initialized]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will get rid of this fixture before making it ready to review, but review comments are welcomed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we're heading to ditch all fixtures, once they already there. Such fixtures are very useful and also keeps the codebase clean. That's my opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I like them to. The problem we have is that if test uses a fixture defined in other app it can be run only in bulk, not as a single test. There might be a problem in how we designed tests. In this case removing fixture was easy, so I did it.

@pgebal pgebal force-pushed the pgebal/fix_standard_exited_utxos__not_recognized_as_unspendable branch from f39a246 to 329e05a Compare May 14, 2020 10:47
@pgebal pgebal force-pushed the pgebal/fix_standard_exited_utxos__not_recognized_as_unspendable branch from 329e05a to cf760f3 Compare May 14, 2020 13:17
pnowosie
pnowosie approved these changes May 18, 2020
View reviewed changes
Copy link
Contributor

@pnowosie pnowosie left a comment

Choose a reason for hiding this comment

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

Nice!

apps/omg_watcher/lib/omg_watcher/api/status.ex Outdated Show resolved Hide resolved
apps/omg_watcher/test/omg_watcher/api/account_test.exs Outdated
@@ -63,5 +66,44 @@ defmodule OMG.Watcher.API.AccountTest do

assert %{blknum: ^blknum, txindex: ^txindex, oindex: ^oindex} = utxo
end

@tag fixtures: [:db_initialized]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we're heading to ditch all fixtures, once they already there. Such fixtures are very useful and also keeps the codebase clean. That's my opinion.

@pgebal pgebal marked this pull request as ready for review May 18, 2020 11:01
@pgebal pgebal force-pushed the pgebal/fix_standard_exited_utxos__not_recognized_as_unspendable branch from f92cfc3 to 9708bb6 Compare May 18, 2020 16:55
unnawut
unnawut approved these changes May 19, 2020
View reviewed changes
apps/omg_watcher/test/omg_watcher/api/account_test.exs
@eth OMG.Eth.zero_address()
@payment_output_type OMG.WireFormatTypes.output_type_for(:output_payment_v1)

setup do
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on moving fixture to setup, this makes it more explicit, plus avoids deep fixture inheritance that fixtures allow

@pgebal pgebal merged commit 7e4a231 into master May 19, 2020
@pgebal pgebal deleted the pgebal/fix_standard_exited_utxos__not_recognized_as_unspendable branch May 19, 2020 11:56
@unnawut unnawut added the bug Something isn't working label Aug 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@unnawut unnawut unnawut approved these changes

@pnowosie pnowosie pnowosie approved these changes

@InoMurko InoMurko Awaiting requested review from InoMurko

@achiurizo achiurizo Awaiting requested review from achiurizo

Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pgebal @coveralls @unnawut @pnowosie