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

Turn on integration tests for Jörmungandr (Part II: Transactions scenarios) #475

Merged
merged 11 commits into from
Jun 26, 2019

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Jun 25, 2019

Issue Number

#358

Overview

  • I have reviewed the divvyFee function from the fee adjustment module in order to avoid propagating rounding issues. This now makes sure that we only generate selections that have just the right amount of fee.

  • I have added property-based tests for divvyFee to ensure that the implementation was done correctly.

  • I have removed the temporary integration scenarios

  • I have added an invariant to adjustForFee about null fees. Fees shouldn't be null.

  • I have adjusted the genesis configuration for Jörmungandr to use an arbitrary non-zero constant for fee calculation.

  • I added a 'feeEstimator' function to the integration Context. This allows to write integration scenarios upon estimated fee values that works for both the bridge and jormungandr.

  • I adjusted some error message assertions regarding address parsing, making them a bit more lenient since messages are different between both targets.

  • I implemented a fee estimator for the bridge and jormungandr

  • I moved the 0-amount output tests into the http-bridge package since those are specific to the byron nodes.

  • I enabled the 'Transactions' scenarios for Jörmungandr.

Comments

@KtorZ KtorZ requested review from Anviking and paweljakubas June 25, 2019 19:10
@KtorZ KtorZ self-assigned this Jun 25, 2019
verify r
[ expectResponseCode HTTP.status403
, expectErrorMessage errMsg403InvalidTransaction
]
Copy link
Member Author

Choose a reason for hiding this comment

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

moved into http-bridge specific location.

{ csInps = [10,10]
, csOuts = [7,7]
, csChngs = [3,3]
})
Copy link
Member Author

Choose a reason for hiding this comment

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

Now forbidden by the invariant.

Copy link
Member

Choose a reason for hiding this comment

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

But why? Why should we disallow 0 fees?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are actually several issues caused by using null fees in our coin selection and fee adjustment algorithms (for instance, this could yield transactions with no inputs). Since there's no particular business requirements regarding supporting a 0-fee policy, we can save us the extra work and simply requires fees to be non-null.

Copy link
Member

Choose a reason for hiding this comment

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

👌

@@ -296,13 +288,21 @@ spec = do
property $ isValidSelection cs

before getSystemDRG $ describe "Fee Adjustment properties" $ do
it "No fee gives back the same selection"
(\_ -> property propSameSelection)
Copy link
Member Author

Choose a reason for hiding this comment

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

Now forbidden by the invariant.

@@ -479,9 +534,13 @@ instance Arbitrary TxIn where
<*> scale (`mod` 3) arbitrary -- No need for a high indexes

instance Arbitrary Coin where
shrink (Coin c) = Coin <$> shrink (fromIntegral c)
shrink (Coin c) = Coin <$> filter (> 0) (shrink $ fromIntegral c)
Copy link
Member Author

Choose a reason for hiding this comment

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

shrinker was not shrinking to valid values.

@KtorZ KtorZ force-pushed the KtorZ/jormungandr-transactions-scenarios branch from a5017d5 to 07632e0 Compare June 25, 2019 19:31
KtorZ added 11 commits June 26, 2019 09:27
The 'downside' is that the last output will pay the surplus caused by rounding issues which is... probably fine
as we are talking about a few lovelace. This has one major benefit however and is that the fee adjustment
algorithm should now stricly balance transaction with no fee surplus at all
Since the fee policy and calculation is actually network-specific, this allows writing some integration
scenarios that relies on the fee values, without being tight to a particular backend
The errors are actually slightly different between both platform... So we can't really have them be that
precise in the integration scenarios
@KtorZ KtorZ force-pushed the KtorZ/jormungandr-transactions-scenarios branch from 07632e0 to 64e30f9 Compare June 26, 2019 07:28
@KtorZ KtorZ merged commit 55b70f5 into master Jun 26, 2019
@Anviking Anviking deleted the KtorZ/jormungandr-transactions-scenarios branch June 26, 2019 11:46
@KtorZ KtorZ added this to the Jörmungandr Integration Testing milestone Jul 2, 2019
@KtorZ KtorZ mentioned this pull request Jul 4, 2019
3 tasks
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.

2 participants