Test accounts logic in mina_base. #12805
Conversation
|
I notice that GitHub allows me to merge this PR even though there's no review and the CI is failing. I guess this is likely a problem in the |
Sventimir
force-pushed
the
sventimir/test-zkapp-command-logic2
branch
from
3701154
to
93ce5a2
Compare
Sventimir
force-pushed
the
sventimir/test-accounts
branch
from
0350b47
to
96ca0ab
Compare
Sventimir
force-pushed
the
sventimir/test-zkapp-command-logic2
branch
from
93ce5a2
to
ff56f0b
Compare
Sventimir
force-pushed
the
sventimir/test-accounts
branch
2 times, most recently
from
e22e047
to
fa6745a
Compare
Sventimir
force-pushed
the
sventimir/test-zkapp-command-logic2
branch
from
ff56f0b
to
5eb3655
Compare
|
!ci-build-me |
There was a problem hiding this comment.
I see that none of the quickcheck tests have specified the number of trials. The number of trials defaults to 10k. This is probably fine for these tests, as I would expect them to be fast, but sometimes the generators can be slow. Have you measured the amount of time it takes to run these new tests?
| |> Balance.to_amount |> Amount.to_uint64 | ||
| in | ||
| UInt64.Infix.(min_balance_at_start_slot - min_balance_at_end_slot) | ||
| if Global_slot.(end_slot <= start_slot) then UInt64.zero |
There was a problem hiding this comment.
When would this function ever be called with such arguments? Depending on context, I would almost prefer to "fail fast" in this case and raise an error, as miscomputing this value as 0 by passing in the parameters backwards could introduce logical errors to the account model.
There was a problem hiding this comment.
I think I have accidentally called it with reversed arguments and observed an overflow, which is obviously bad. Hence I added a specific test, which verifies that the function behaves correctly in this case. What the correct behaviour is, is debatable. I could change it to return an option, although I think it makes sense to keep the types simple. In a sense, 0 is a correct result, because over 0 blocks, the incremental balance is always 0.
If someone accidentally calls it with end_slot smaller than start_slot in the wild, it certainly means things have gone wrong somewhere else. I don't really like the idea that I have to check this function for correct result (Ok or Some or whatever) just because it could potentially be called backawrds. The caller should validate the arguments before calling the function, not afterwards. Having them pattern match on the result is punishing them for doing their job and complicating the code for the reader as well. I'd rather avoid that. I think it goes without saying that raising an exception is even worse, because it could crash a node in the middle of block production.
| @@ -0,0 +1,31 @@ | |||
| (library | |||
| (name account_test) | |||
There was a problem hiding this comment.
It might be a good idea to namespace this (library names must be globally unique). A common pattern for doing this would be to call this library mina_base.account_test. We don't do this everywhere now, but I'm trying to do this more because sometimes we hit conflicting library names, and dune generates unhelpful error messages when this happens.
There was a problem hiding this comment.
File "src/lib/mina_base/test/account/dune", line 2, characters 7-29:
2 | (name mina_base.account_test)
^^^^^^^^^^^^^^^^^^^^^^
Error: "mina_base.account_test" is an invalid library name.
Library names must be non-empty and composed only of the following
characters: 'A'..'Z', 'a'..'z', '_' or '0'..'9'.
Hint: mina_base_account_test would be a correct library name
Would mina_base_account_test be okay? Sounds a lot less appealing to me, but I guess it's better than nothing…
There was a problem hiding this comment.
Apologies for the confusion. It's a bit odd in dune because there is a difference between the local name of a library, and the public name. The local name can't have ., but the public one can. Here is an example of a library that uses this namespacing for the public name while setting a local name that just has an underscore in its place. It reads odd, but this does have the desired effect of fixing the global namespacing problem. https://github.com/MinaProtocol/mina/blob/develop/src/lib/mina_metrics/no_metrics/dune
These tests compile and run in a little over a minute on my laptop. I think it's quick enough, isn't it? |
|
!ci-build-me |
|
!ci-build-me |
gltrost
changed the base branch from
sventimir/test-zkapp-command-logic2
to
berkeley
gltrost
force-pushed
the
sventimir/test-accounts
branch
2 times, most recently
from
13c9c04
to
41842f4
Compare
The checked function is broken and will fail under certain circumstances. This is not very likely to happen, but theoretically possible. However, it's not trivial to fix, so we'll fix it in a separate PR.
Add ?min_vesting_periods param
gltrost
force-pushed
the
sventimir/test-accounts
branch
from
35ebfc9
to
07b112c
Compare
Explain your changes:
Explain how you tested your changes:
Checklist: