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

Add maxCollateralInputs to ProtocolParameters #2813

Merged
merged 2 commits into from
Aug 11, 2021

Conversation

sevanspowell
Copy link
Contributor

@sevanspowell sevanspowell commented Aug 10, 2021

Issue Number

ADP-958

Overview

  • Included the maxCollateralInputs protocol parameter in the ProtocolParameters data type.

Comments

  • No tests exist for this structure, I believe this structure is tested implicitly via the ApiNetworkParameters structure. But since modifying that structure would encroach on ADP-1061, I'm going to leave this without tests for now. If anyone can think of a test they'd like to see, don't hesitate to suggest one!

Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Yep all good - hopefully some test case needs updating for the new field.

--
-- If this conversion would under/overflow, there is not much we can do except
-- to hastily exit.
unsafeToWord16 :: Integral n => n -> Word16
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps you could combine this with unsafeToWord64 and make

Suggested change
unsafeToWord16 :: Integral n => n -> Word16
unsafeToWord :: (Integral n, Integral w) => n -> w

@sevanspowell sevanspowell force-pushed the sevanspowell/adp-958/maximum-collateral-inputs branch from d40551e to 78411fd Compare August 11, 2021 01:49
@sevanspowell sevanspowell self-assigned this Aug 11, 2021
@sevanspowell sevanspowell marked this pull request as ready for review August 11, 2021 01:55
@rvl rvl force-pushed the sevanspowell/adp-958/maximum-collateral-inputs branch from db01eda to b70f385 Compare August 11, 2021 03:15
Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

Great, thanks. I added a test case for unsafeIntToWord, just to be sure, and took the opportunity to improve the way we crash the app.

@sevanspowell
Copy link
Contributor Author

bors r+

iohk-bors bot added a commit that referenced this pull request Aug 11, 2021
2813: Add maxCollateralInputs to ProtocolParameters r=sevanspowell a=sevanspowell

# Issue Number

ADP-958


# Overview

- Included the maxCollateralInputs protocol parameter in the ProtocolParameters data type.

# Comments

- No tests exist for this structure, I believe this structure is tested implicitly via the `ApiNetworkParameters` structure. But since modifying that structure would encroach on ADP-1061, I'm going to leave this without tests for now. If anyone can think of a test they'd like to see, don't hesitate to suggest one!


Co-authored-by: Samuel Evans-Powell <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 11, 2021

This PR was included in a batch that successfully built, but then failed to merge into master (it was a non-fast-forward update). It will be automatically retried.

iohk-bors bot added a commit that referenced this pull request Aug 11, 2021
2813: Add maxCollateralInputs to ProtocolParameters r=sevanspowell a=sevanspowell

# Issue Number

ADP-958


# Overview

- Included the maxCollateralInputs protocol parameter in the ProtocolParameters data type.

# Comments

- No tests exist for this structure, I believe this structure is tested implicitly via the `ApiNetworkParameters` structure. But since modifying that structure would encroach on ADP-1061, I'm going to leave this without tests for now. If anyone can think of a test they'd like to see, don't hesitate to suggest one!


Co-authored-by: Samuel Evans-Powell <[email protected]>
Co-authored-by: Rodney Lorrimar <[email protected]>
@sevanspowell
Copy link
Contributor Author

bors retry

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 11, 2021

Already running a review

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Aug 11, 2021

Build failed:

#2703

However it looks like it completed?

Finished in 665.8690 seconds, used 848.5850 seconds of CPU time
3953 examples, 0 failures, 42 pending

Slow spec items:
  test/unit/Cardano/Wallet/DB/SqliteSpec.hs:296:5: /Cardano.Wallet.DB.Sqlite/State machine test (SharedState 'Mainnet SharedKey)/Sequential/ (46417ms)
  test/unit/Cardano/Wallet/DB/SqliteSpec.hs:296:5: /Cardano.Wallet.DB.Sqlite/State machine test (RndState 'Mainnet)/Sequential/ (42107ms)
  test/unit/Cardano/Wallet/DB/SqliteSpec.hs:296:5: /Cardano.Wallet.DB.Sqlite/State machine test (SeqState 'Mainnet ShelleyKey)/Sequential/ (40582ms)
  test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs:350:9: /Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobin/Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobinSpec/Making change for many non-user-specified assets/prop_makeChangeForNonUserSpecifiedAssets_sum/ (19472ms)
  test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs:348:9: /Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobin/Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobinSpec/Making change for many non-user-specified assets/prop_makeChangeForNonUserSpecifiedAssets_order/ (16928ms)
  test/unit/Cardano/Wallet/Primitive/Migration/PlanningSpec.hs:110:9: /Cardano.Wallet.Primitive.Migration.Planning/Cardano.Wallet.Primitive.Migration.PlanningSpec/Categorizing UTxO entries/prop_categorizeUTxOEntries/ (15915ms)
  test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs:247:9: /Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobin/Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobinSpec/Performing a selection/prop_performSelection_large/ (14107ms)
  test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs:217:9: /Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobin/Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobinSpec/Coverage/prop_Large_UTxOIndex_coverage/ (13320ms)
  test/unit/Cardano/Wallet/Primitive/CoinSelection/MA/RoundRobinSpec.hs:308:9: /Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobin/Cardano.Wallet.Primitive.CoinSelection.MA.RoundRobinSpec/Making change/prop_makeChange/ (12487ms)
  test/unit/Cardano/Wallet/Primitive/AddressDerivation/MintBurnSpec.hs:69:9: /Cardano.Wallet.Primitive.AddressDerivation.MintBurn/Mint/Burn Policy key Address Derivation Properties/Using derivePolicyKeyAndHash returns same private key as using derivePolicyPrivateKey/ (11012ms)

@sevanspowell
Copy link
Contributor Author

bors retry

@rvl
Copy link
Contributor

rvl commented Aug 11, 2021

Oops, sorry I pushed a minor fix to .github/ISSUE_TEMPLATES/config.yml directly to master and tripped up bors. Will just merge now this since it's already passed bors.

@rvl rvl merged commit 3d4e5d7 into master Aug 11, 2021
@rvl rvl deleted the sevanspowell/adp-958/maximum-collateral-inputs branch August 11, 2021 06:27
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