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

Factor out pure balanceTransaction logic #2970

Merged
merged 9 commits into from
Oct 29, 2021
Merged

Factor out pure balanceTransaction logic #2970

merged 9 commits into from
Oct 29, 2021

Conversation

Anviking
Copy link
Member

@Anviking Anviking commented Oct 12, 2021 •
edited by jonathanknowles
Loading

Motivation

Make property testing of the balanceTransaction logic possible.

Overview

  • Add m to HasLogger constraint rather than to assume IO
  • Make selectAssets run in MonadRandom m rather than IO
  • Let selectAssets take PParams directly, rather than fetching from NetworkLayer
  • Remove IO from assignScriptRedeemers
  • Factor out W.balanceTransaction running in MonadRandom m

Issue Number

ADP-1182

@Anviking Anviking requested a review from KtorZ October 12, 2021 13:10
@Anviking Anviking self-assigned this Oct 12, 2021
@Anviking Anviking force-pushed the anviking/ADP-1182/pure-balance-tx branch from 7ab1661 to 60e2fe8 Compare October 12, 2021 13:32
@Anviking
Copy link
Member Author

Thanks @jonathanknowles, grouping arguments in records probably makes sense. My strategy so far was to make minimal changes and not do any premature abstractions, but I'll have a look. But I might try experimenting with writing actual property tests on top of this first, in case I learn something.

@Anviking Anviking marked this pull request as draft October 14, 2021 12:01
@Anviking Anviking force-pushed the anviking/ADP-1182/pure-balance-tx branch 2 times, most recently from 87c3403 to d89025c Compare October 21, 2021 13:25
@Anviking Anviking force-pushed the anviking/ADP-1182/pure-balance-tx branch from b192331 to 73b91f7 Compare October 25, 2021 19:29
@Anviking Anviking marked this pull request as ready for review October 25, 2021 19:38
@Anviking Anviking force-pushed the anviking/ADP-1182/pure-balance-tx branch from 07e0289 to b3aca28 Compare October 25, 2021 19:54
jonathanknowles
jonathanknowles reviewed Oct 27, 2021
View reviewed changes
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

Hi @Anviking

This PR seems like a very positive step towards making balanceTransaction more testable. Thank-you for making it! 👍🏻

I only have a few suggestions and requests: some of these apply to code that was moved (and therefore not "changed" by this PR per se), but I think it would still be good to clean up the highlighted areas before merging.

lib/core/src/Cardano/Wallet.hs Show resolved Hide resolved
lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/Api/Server.hs Outdated Show resolved Hide resolved
lib/core/src/Cardano/Wallet/Primitive/Slotting.hs Show resolved Hide resolved
lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs Outdated Show resolved Hide resolved
Anviking and others added 6 commits October 27, 2021 10:15
@Anviking
Add m to HasLogger constraint rather than to assume IO
4ae0540
@Anviking
Make selectAssets run in MonadRandom m rather than IO
7258380
@Anviking
Let selectAssets take PParams directly, rather than fetching from Net…
32633a7 
…workLayer

Passing the full NetworkLayer seems overkill, and would have required
less elegant mocking to property test balanceTransaction
(calling selectAssets).
@Anviking
Remove IO from assignScriptRedeemers
de32bbe 
Possible by adding the following function:
```
snapshot
    :: TimeInterpreter (ExceptT PastHorizonException IO)
    -> IO (TimeInterpreter (Either PastHorizonException))
```
@Anviking
Factor out W.balanceTransaction running in MonadRandom m
ba3f277 
Which can be more easily property tested.
@Anviking
Regenerate nix
539f120
@Anviking Anviking force-pushed the anviking/ADP-1182/pure-balance-tx branch from 513e722 to c022012 Compare October 27, 2021 10:43
@Anviking Anviking force-pushed the anviking/ADP-1182/pure-balance-tx branch from c022012 to eb00f06 Compare October 27, 2021 11:32
@Anviking
Copy link
Member Author

Thanks @jonathanknowles — I believe I have addressed all suggestions except for #2970 (comment) which I think is invalid and #2970 (comment) which I'd be inclined not to do.

@sevanspowell
Copy link
Contributor

Given it a good read over. This seems sensible 👍 I'm a bit dubious on the padding and estimation, but I guess we'll catch anything off in our property tests.

@Anviking
Copy link
Member Author

I'm a bit dubious on the padding and estimation

I'm very dubious too 😅 I wouldn't want to start changing it before having property tests, however.

jonathanknowles
jonathanknowles approved these changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

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

Hi @Anviking

Many thanks again for making this PR. It looks good to me!

@jonathanknowles
Copy link
Contributor

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Oct 29, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit d527bbb into master Oct 29, 2021
@iohk-bors iohk-bors bot deleted the anviking/ADP-1182/pure-balance-tx branch October 29, 2021 03:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KtorZ KtorZ KtorZ left review comments

@jonathanknowles jonathanknowles jonathanknowles approved these changes

Assignees

@Anviking Anviking

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Anviking @sevanspowell @jonathanknowles @KtorZ