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

Fix Change Calculation (#286) #292

Merged
merged 3 commits into from
May 21, 2019
Merged

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented May 21, 2019

Issue Number

#286

Overview

  • I have extended our test to capture the issue highlighted in the bug
  • I have looked at the test failing
  • I have fixed the calculation of the change which was... weirdly way off the specification..

Comments

@KtorZ KtorZ requested a review from piotr-iohk May 21, 2019 07:42
@KtorZ KtorZ self-assigned this May 21, 2019
changeUTxO proxy pending = evalState $ do
ourUtxo <- mapM (state . utxoOurs proxy) (Set.toList pending)
let ins = txIns pending
return $ fold ourUtxo `restrictedBy` ins
Copy link
Member Author

Choose a reason for hiding this comment

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

Not even sure why this was even there... The spec says:

image

Which is exactly what the first line already does... The two others are non-sense.

[ expectSuccess
, expectEventually ctx balanceAvailable (oneMillionAda + 1)
]

verify ra
[ expectEventually ctx balanceAvailable 999999831346
Copy link
Member Author

Choose a reason for hiding this comment

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

I really love this expectEventually combinator 😄 ....

[ expectSuccess
, expectEventually ctx balanceAvailable (oneMillionAda + 1)
]

verify ra
[ expectEventually ctx balanceAvailable 999999831346
Copy link
Member

Choose a reason for hiding this comment

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

Hi @KtorZ
Where does this 999999831346 value come from, exactly, and what is its significance? Assuming it's supposed to be the same value as https://git.io/fj4GR, then would it be clearer to use a named constant, possibly with a comment to state its origin?

Copy link
Member Author

Choose a reason for hiding this comment

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

All fixture wallets have exactly 1000M ADA on them (should be 1M though, but will fix in another PR). So, this amount is 1000M - cost of the last payment. Can make them constants :)

@KtorZ KtorZ force-pushed the KtorZ/286/fix-change-calculation branch from d380903 to 252cd12 Compare May 21, 2019 09:12
oneMillionAda :: Natural
oneMillionAda = ada (1_000_000)
where
ada = (*) (1_000_000)
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if it would be possible to use the type system here, to distinguish amounts (in Ada or Lovelace) from other values that happen to be Natural?

In particular, we're trying to encode something that is a number of Lovelace: the number of lovelace in one Ada.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could use Quantity etc ... But we purposely use raw types in the integration DSL to keep the test scenarios readable and simple to write.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's was my question : why not Quantity here?

Copy link
Member Author

Choose a reason for hiding this comment

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

As just explained, to avoid having to perform to many transformations when writing the integration test scenarios. Scenarios are mostly just about submitting things to the API and asserting results; Manipulating raw types directly there makes it very easy to write these scenarios and piggy-back on the API to do the type-checking etc ...

More importantly, it also allows us to provide invalid and out-of-range values and still submit them to test that API fulfill its role! This is also why we don't use a typed client in the integration tests. We do want to be able to submit invalid values :)

Copy link
Member

Choose a reason for hiding this comment

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

Besides, Quantity might not always help. Consider

pay :: Quantity "lovelace" Natural -> result

-- later

do
pay (Quantity 100)
...

The type is inferred at the call-site and can easily be confused. Would be better to enforce it to be explicit.

Like

do
pay (Lovelace 100)
...

(I'm sure there are fancier ways too)


r <- request @ApiTransaction ctx ("POST", postTx wa) Default payload
verify r
[ expectSuccess
, expectResponseCode HTTP.status202
, expectFieldEqual amount 168654
, expectFieldEqual amount (fee + amt)
Copy link
Member

Choose a reason for hiding this comment

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

This is much clearer. I like it 👍

, expectFieldEqual direction Outgoing
, expectFieldEqual status Pending
]

ra <- request @ApiWallet ctx ("GET", getWallet wa) Default payload
verify ra
[ expectSuccess
, expectFieldEqual balanceTotal 999999831346
, expectFieldEqual balanceTotal (oneMillionAda - fee - amt)
Copy link
Member

Choose a reason for hiding this comment

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

👍

]

verify ra
[ expectEventually ctx balanceAvailable 999999831346
[ expectEventually ctx balanceAvailable (oneMillionAda - fee - amt)
Copy link
Member

Choose a reason for hiding this comment

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

Nice :)

@@ -430,7 +432,7 @@ fixtureWallet ctx@(Context _ _ _ faucet) = do
Left _ -> fail "fixtureWallet: waited too long for initial transaction"
Right a -> return a
where
oneSecond = 1*1000*1000
oneSecond = 1_000_000
Copy link
Contributor

@paweljakubas paweljakubas May 21, 2019

Choose a reason for hiding this comment

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

we do not want Quantity here? maybe it is overstreatch here, but we use second through the code (I believe) and maybe it is not silly idea to introduce time units somewhere. and if so rely on Quantity there

Copy link
Contributor

@paweljakubas paweljakubas left a comment

Choose a reason for hiding this comment

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

LGTM! I see that shuffle (one red test detected it) haunts us also here ;-) After #295 we should be confident it is not to happen again

@KtorZ
Copy link
Member Author

KtorZ commented May 21, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request May 21, 2019
283: Sqlite: add checkpoints and transactions to DBLayer r=KtorZ a=rvl

Relates to issue #154.

# Overview

- Implemented saving and loading of transaction history to SQLite
- Implemented saving and loading of wallet checkpoints to SQLite, including the state for sequential scheme address discovery.

# Comments

- The SqliteSpec testing is a bit light. However, there should be enough implemented for all the DBSpec tests to work. Also I plan to finish the QSM tests tomorrow which should provide even more coverage.
- Cascading deletes are not working with persistent-sqlite, which is annoying.
- DB indexes are missing on some fields. (Needs some custom SQL, can be fixed later)


289: show feature availability in API specification r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

N/A

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [ ] I have removed the"priority" from the specification and added a "status" indicating the availability of a particular feature in the API.

# Comments

<!-- Additional comments or screenshots to attach if any -->

As requested by Chris. Makes it easier for externals to know what's available or not. 

A preview:

![image](https://user-images.githubusercontent.com/5680256/58074137-bdc26f00-7ba4-11e9-95b5-33ff723e0f2a.png)

![image](https://user-images.githubusercontent.com/5680256/58074150-c6b34080-7ba4-11e9-83a5-943877e14fd7.png)

![image](https://user-images.githubusercontent.com/5680256/58074161-cfa41200-7ba4-11e9-8d83-0948b129e856.png)

![image](https://user-images.githubusercontent.com/5680256/58074177-dd599780-7ba4-11e9-867e-5047012b1f93.png)

![image](https://user-images.githubusercontent.com/5680256/58074188-e5193c00-7ba4-11e9-845c-cacfd34a9fc9.png)

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


292: Fix Change Calculation (#286) r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#286 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have extended our test to capture the issue highlighted in the bug
- [x] I have looked at the test failing
- [x] I have fixed the calculation of the change which was... weirdly way off the specification..

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Rodney Lorrimar <[email protected]>
Co-authored-by: Pawel Jakubas <[email protected]>
Co-authored-by: KtorZ <[email protected]>
@KtorZ
Copy link
Member Author

KtorZ commented May 21, 2019

bors r+

iohk-bors bot added a commit that referenced this pull request May 21, 2019
283: Sqlite: add checkpoints and transactions to DBLayer r=KtorZ a=rvl

Relates to issue #154.

# Overview

- Implemented saving and loading of transaction history to SQLite
- Implemented saving and loading of wallet checkpoints to SQLite, including the state for sequential scheme address discovery.

# Comments

- The SqliteSpec testing is a bit light. However, there should be enough implemented for all the DBSpec tests to work. Also I plan to finish the QSM tests tomorrow which should provide even more coverage.
- Cascading deletes are not working with persistent-sqlite, which is annoying.
- DB indexes are missing on some fields. (Needs some custom SQL, can be fixed later)


289: show feature availability in API specification r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

N/A

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [ ] I have removed the"priority" from the specification and added a "status" indicating the availability of a particular feature in the API.

# Comments

<!-- Additional comments or screenshots to attach if any -->

As requested by Chris. Makes it easier for externals to know what's available or not. 

A preview:

![image](https://user-images.githubusercontent.com/5680256/58074137-bdc26f00-7ba4-11e9-95b5-33ff723e0f2a.png)

![image](https://user-images.githubusercontent.com/5680256/58074150-c6b34080-7ba4-11e9-83a5-943877e14fd7.png)

![image](https://user-images.githubusercontent.com/5680256/58074161-cfa41200-7ba4-11e9-8d83-0948b129e856.png)

![image](https://user-images.githubusercontent.com/5680256/58074177-dd599780-7ba4-11e9-867e-5047012b1f93.png)

![image](https://user-images.githubusercontent.com/5680256/58074188-e5193c00-7ba4-11e9-845c-cacfd34a9fc9.png)

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


292: Fix Change Calculation (#286) r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#286 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have extended our test to capture the issue highlighted in the bug
- [x] I have looked at the test failing
- [x] I have fixed the calculation of the change which was... weirdly way off the specification..

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


296: Improve bech32 coveralls r=KtorZ a=piotr-iohk

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->


# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [ ] I have replaced `isLeft` with `Left ...` here and there to improve coveralls score a bit


# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Rodney Lorrimar <[email protected]>
Co-authored-by: Pawel Jakubas <[email protected]>
Co-authored-by: KtorZ <[email protected]>
Co-authored-by: Piotr Stachyra <[email protected]>
Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

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

👍

@KtorZ
Copy link
Member Author

KtorZ commented May 21, 2019

bors retry

iohk-bors bot added a commit that referenced this pull request May 21, 2019
292: Fix Change Calculation (#286) r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#286 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [x] I have extended our test to capture the issue highlighted in the bug
- [x] I have looked at the test failing
- [x] I have fixed the calculation of the change which was... weirdly way off the specification..

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
@KtorZ KtorZ merged commit a015aff into master May 21, 2019
@iohk-bors iohk-bors bot deleted the KtorZ/286/fix-change-calculation branch May 21, 2019 13:17
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.

5 participants