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

Adds two UTxO properties #642

Merged
merged 1 commit into from
Jul 17, 2019
Merged

Adds two UTxO properties #642

merged 1 commit into from
Jul 17, 2019

Conversation

mdimjasevic
Copy link
Contributor

This pull request implements two properties for UTxO:

  • no double spending
  • UTxO is outputs minuts inputs

It closes #263.

@dnadales , @uroboros: is there anything to add or change to improve this PR?

@mdimjasevic mdimjasevic requested review from uroboros and dnadales July 16, 2019 11:34
@mdimjasevic mdimjasevic requested a review from nc6 as a code owner July 16, 2019 11:34
@mdimjasevic mdimjasevic self-assigned this Jul 16, 2019
dnadales
dnadales reviewed Jul 17, 2019
View reviewed changes
Copy link
Member

@dnadales dnadales left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM.

My main remarks:

  • I'd remove the HasTrace UTXO instance.
  • I'd replace all for traverse_ for better error messages, and do some style changes.

@dnadales
Copy link
Member

Thanks for addressing the comments Marko. Ping me when you want me to take a second look 🙏

@mdimjasevic
Copy link
Contributor Author

Thanks for addressing the comments Marko. Ping me when you want me to take a second look pray

Thank you for the feedback! I'll think of coverage before pining you for another review.

uroboros
uroboros reviewed Jul 17, 2019
View reviewed changes
byron/ledger/executable-spec/test/Ledger/UTxO/Properties.hs
(utxo0, utxoSt) = (utxo *** utxo) . firstAndLastState $ t
txs = body <$> traceSignals OldestFirst t
when (all (\ti -> dom (txouts ti) ∩ dom utxo0 == empty) txs) $
foldl' union' empty txs ⋪ (utxo0 ∪ allTxOuts txs) === utxoSt
Copy link
Contributor

Choose a reason for hiding this comment

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

you could us Set.unions (foldl union empty) as in

Set.unions ((fromList . txins) <$> txs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair point. I guess it boils down to what's more readable to someone and/or what is one's preference. I don't have a strong opinion here. Should I change this?

uroboros
uroboros approved these changes Jul 17, 2019
View reviewed changes
Copy link
Contributor

@uroboros uroboros left a comment

Choose a reason for hiding this comment

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

LGTM2

@dnadales
Copy link
Member

Thank you for the feedback! I'll think of coverage before pining you for another review.

No problem. I would address the coverage in a separate issue. Let's just close this one if possible.

@mdimjasevic
Copy link
Contributor Author

No problem. I would address the coverage in a separate issue. Let's just close this one if possible.

@dnadales, I just added something so please take a look: ed339b6

dnadales
dnadales approved these changes Jul 17, 2019
View reviewed changes
Copy link
Member

@dnadales dnadales left a comment

Choose a reason for hiding this comment

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

:shipit:

@mdimjasevic mdimjasevic mentioned this pull request Jul 17, 2019
Closed
@mdimjasevic mdimjasevic force-pushed the mdimjasevic/263-utxo-properties branch 2 times, most recently from 4e6d436 to 2e22a5d Compare July 17, 2019 13:40
Closes #263: implements two properties for UTxO:
779be01 
* no double spending
* UTxO is outputs minus inputs

An issue with UTxO generators has appeared while working on this, which
is tracked in a new issue #646.
@mdimjasevic mdimjasevic force-pushed the mdimjasevic/263-utxo-properties branch from 2e22a5d to 779be01 Compare July 17, 2019 13:42
@mdimjasevic mdimjasevic merged commit 44dea79 into master Jul 17, 2019
@mdimjasevic mdimjasevic deleted the mdimjasevic/263-utxo-properties branch July 17, 2019 14:09
kevinhammond pushed a commit that referenced this pull request Oct 28, 2019
@mdimjasevic
Merge pull request #642 from input-output-hk/mdimjasevic/263-utxo-pro…
ac0806c 
…perties

Adds two UTxO properties per #263
@nfrisby nfrisby mentioned this pull request Mar 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnadales dnadales dnadales approved these changes

@uroboros uroboros uroboros approved these changes

@nc6 nc6 Awaiting requested review from nc6

Assignees

@mdimjasevic mdimjasevic

Labels
byron ⚙️ executable-model
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add property tests for UTxO: no double spending and utxo is inputs minus outputs
3 participants
@mdimjasevic @dnadales @uroboros