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

Implement modulo argument of revertibleRandom #2986

Merged
merged 32 commits into from
Jan 16, 2024

Conversation

tarakby
Copy link
Contributor

@tarakby tarakby commented Dec 20, 2023

Closes #2712

Description

Specifically the item:

  • modulo is an optional parameter, if provided the returned value r satisfies 0<= r < modulo. The function panics if modulo == 0 - algorithm implemented should be sample rejection.

Docs is updated in a separate PR against the doc repo: onflow/cadence-lang.org#37


  • Targeted PR against master branch
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work
  • Code follows the standards mentioned here
  • Updated relevant documentation
  • Re-reviewed Files changed in the Github PR explorer
  • Added appropriate labels

@tarakby tarakby self-assigned this Dec 20, 2023
@tarakby tarakby added the Language Breaking Change Breaks Cadence contracts deployed on Mainnet label Dec 20, 2023
Copy link

github-actions bot commented Dec 20, 2023

Cadence Benchstat comparison

This branch with compared with the base branch onflow:master commit 9c630d3
The command for i in {1..N}; do go test ./... -run=XXX -bench=. -benchmem -shuffle=on; done was used.
Bench tests were run a total of 7 times on each branch.

Collapsed results for better readability

old.txtnew.txt
time/opdelta
DecodeBatchEventsCCF-4129ms ± 0%368ms ± 0%~(p=1.000 n=1+1)
DecodeBatchEventsJSON-4390ms ± 0%385ms ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowFees.FeesDeducted-42.80µs ± 0%2.81µs ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowFees.TokensWithdrawn-42.17µs ± 0%2.09µs ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowIDTableStaking.DelegatorRewardsPaid-42.79µs ± 0%2.79µs ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowIDTableStaking.EpochTotalRewardsPaid-43.13µs ± 0%3.08µs ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowIDTableStaking.NewWeeklyPayout-42.13µs ± 0%2.12µs ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowIDTableStaking.RewardsPaid-42.49µs ± 0%2.52µs ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowToken.TokensDeposited-42.59µs ± 0%2.53µs ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowToken.TokensDeposited_with_nil_receiver-42.51µs ± 0%2.46µs ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowToken.TokensMinted-42.11µs ± 0%2.09µs ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowToken.TokensWithdrawn-42.63µs ± 0%2.60µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowFees.FeesDeducted-49.52µs ± 0%9.50µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowFees.TokensWithdrawn-45.58µs ± 0%5.50µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowIDTableStaking.DelegatorRewardsPaid-48.75µs ± 0%8.54µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowIDTableStaking.EpochTotalRewardsPaid-411.8µs ± 0%11.7µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowIDTableStaking.NewWeeklyPayout-45.71µs ± 0%5.63µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowIDTableStaking.RewardsPaid-47.40µs ± 0%7.26µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowToken.TokensDeposited-47.74µs ± 0%7.64µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowToken.TokensDeposited_with_nil_receiver-47.08µs ± 0%6.92µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowToken.TokensMinted-45.63µs ± 0%5.55µs ± 0%~(p=1.000 n=1+1)
DecodeJSON/FlowToken.TokensWithdrawn-47.70µs ± 0%7.58µs ± 0%~(p=1.000 n=1+1)
EncodeBatchEventsCCF-488.9ms ± 0%88.0ms ± 0%~(p=1.000 n=1+1)
EncodeBatchEventsJSON-4103ms ± 0%103ms ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowFees.FeesDeducted-42.01µs ± 0%1.96µs ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowFees.TokensWithdrawn-41.57µs ± 0%1.57µs ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowIDTableStaking.DelegatorRewardsPaid-41.91µs ± 0%1.89µs ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowIDTableStaking.EpochTotalRewardsPaid-42.26µs ± 0%2.23µs ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowIDTableStaking.NewWeeklyPayout-41.71µs ± 0%1.58µs ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowIDTableStaking.RewardsPaid-41.74µs ± 0%1.73µs ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowToken.TokensDeposited-41.80µs ± 0%1.77µs ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowToken.TokensDeposited_with_nil_receiver-41.78µs ± 0%1.77µs ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowToken.TokensMinted-41.59µs ± 0%1.55µs ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowToken.TokensWithdrawn-41.79µs ± 0%1.79µs ± 0%~(all equal)
EncodeJSON/FlowFees.FeesDeducted-42.47µs ± 0%2.48µs ± 0%~(p=1.000 n=1+1)
EncodeJSON/FlowFees.TokensWithdrawn-41.40µs ± 0%1.41µs ± 0%~(p=1.000 n=1+1)
EncodeJSON/FlowIDTableStaking.DelegatorRewardsPaid-42.20µs ± 0%2.22µs ± 0%~(p=1.000 n=1+1)
EncodeJSON/FlowIDTableStaking.EpochTotalRewardsPaid-43.08µs ± 0%3.06µs ± 0%~(p=1.000 n=1+1)
EncodeJSON/FlowIDTableStaking.NewWeeklyPayout-41.42µs ± 0%1.42µs ± 0%~(p=1.000 n=1+1)
EncodeJSON/FlowIDTableStaking.RewardsPaid-41.83µs ± 0%1.98µs ± 0%~(p=1.000 n=1+1)
EncodeJSON/FlowToken.TokensDeposited-42.12µs ± 0%2.11µs ± 0%~(p=1.000 n=1+1)
EncodeJSON/FlowToken.TokensDeposited_with_nil_receiver-41.65µs ± 0%1.66µs ± 0%~(p=1.000 n=1+1)
EncodeJSON/FlowToken.TokensMinted-41.40µs ± 0%1.40µs ± 0%~(p=1.000 n=1+1)
EncodeJSON/FlowToken.TokensWithdrawn-42.10µs ± 0%2.07µs ± 0%~(p=1.000 n=1+1)
ExportType/composite_type-4307ns ± 0%287ns ± 0%~(p=1.000 n=1+1)
ExportType/simple_type-473.4ns ± 0%74.0ns ± 0%~(p=1.000 n=1+1)
InterpretRecursionFib-42.19ms ± 0%2.30ms ± 0%~(p=1.000 n=1+1)
NewInterpreter/new_interpreter-4988ns ± 0%918ns ± 0%~(p=1.000 n=1+1)
NewInterpreter/new_sub-interpreter-4466ns ± 0%456ns ± 0%~(p=1.000 n=1+1)
ParseArray-46.12ms ± 0%5.89ms ± 0%~(p=1.000 n=1+1)
ParseDeploy/byte_array-48.90ms ± 0%8.80ms ± 0%~(p=1.000 n=1+1)
ParseDeploy/decode_hex-41.00ms ± 0%0.99ms ± 0%~(p=1.000 n=1+1)
ParseFungibleToken/With_memory_metering-4153µs ± 0%157µs ± 0%~(p=1.000 n=1+1)
ParseFungibleToken/Without_memory_metering-4124µs ± 0%120µs ± 0%~(p=1.000 n=1+1)
ParseInfix-45.23µs ± 0%5.17µs ± 0%~(p=1.000 n=1+1)
QualifiedIdentifierCreation/One_level-42.17ns ± 0%2.17ns ± 0%~(p=1.000 n=1+1)
QualifiedIdentifierCreation/Three_levels-483.8ns ± 0%83.6ns ± 0%~(p=1.000 n=1+1)
RuntimeScriptNoop-43.62µs ± 0%3.61µs ± 0%~(p=1.000 n=1+1)
SuperTypeInference/arrays-4251ns ± 0%251ns ± 0%~(p=1.000 n=1+1)
SuperTypeInference/composites-492.8ns ± 0%92.2ns ± 0%~(p=1.000 n=1+1)
SuperTypeInference/integers-4314ns ± 0%314ns ± 0%~(p=1.000 n=1+1)
ValueIsSubtypeOfSemaType-473.6ns ± 0%85.0ns ± 0%~(p=1.000 n=1+1)
 
alloc/opdelta
DecodeBatchEventsCCF-466.1MB ± 0%66.1MB ± 0%~(p=1.000 n=1+1)
DecodeBatchEventsJSON-4244MB ± 0%244MB ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowFees.FeesDeducted-41.39kB ± 0%1.39kB ± 0%~(all equal)
DecodeCCF/FlowFees.TokensWithdrawn-41.20kB ± 0%1.20kB ± 0%~(all equal)
DecodeCCF/FlowIDTableStaking.DelegatorRewardsPaid-41.47kB ± 0%1.47kB ± 0%~(all equal)
DecodeCCF/FlowIDTableStaking.EpochTotalRewardsPaid-41.48kB ± 0%1.48kB ± 0%~(all equal)
DecodeCCF/FlowIDTableStaking.NewWeeklyPayout-41.25kB ± 0%1.25kB ± 0%~(all equal)
DecodeCCF/FlowIDTableStaking.RewardsPaid-41.37kB ± 0%1.37kB ± 0%~(all equal)
DecodeCCF/FlowToken.TokensDeposited-41.31kB ± 0%1.31kB ± 0%~(all equal)
DecodeCCF/FlowToken.TokensDeposited_with_nil_receiver-41.30kB ± 0%1.30kB ± 0%~(all equal)
DecodeCCF/FlowToken.TokensMinted-41.20kB ± 0%1.20kB ± 0%~(all equal)
DecodeCCF/FlowToken.TokensWithdrawn-41.32kB ± 0%1.32kB ± 0%~(all equal)
DecodeJSON/FlowFees.FeesDeducted-46.00kB ± 0%6.00kB ± 0%~(all equal)
DecodeJSON/FlowFees.TokensWithdrawn-43.60kB ± 0%3.60kB ± 0%~(all equal)
DecodeJSON/FlowIDTableStaking.DelegatorRewardsPaid-45.43kB ± 0%5.43kB ± 0%~(all equal)
DecodeJSON/FlowIDTableStaking.EpochTotalRewardsPaid-47.35kB ± 0%7.35kB ± 0%~(all equal)
DecodeJSON/FlowIDTableStaking.NewWeeklyPayout-43.64kB ± 0%3.64kB ± 0%~(all equal)
DecodeJSON/FlowIDTableStaking.RewardsPaid-44.54kB ± 0%4.54kB ± 0%~(all equal)
DecodeJSON/FlowToken.TokensDeposited-44.88kB ± 0%4.88kB ± 0%~(all equal)
DecodeJSON/FlowToken.TokensDeposited_with_nil_receiver-44.46kB ± 0%4.46kB ± 0%~(all equal)
DecodeJSON/FlowToken.TokensMinted-43.60kB ± 0%3.60kB ± 0%~(all equal)
DecodeJSON/FlowToken.TokensWithdrawn-44.88kB ± 0%4.88kB ± 0%~(all equal)
EncodeBatchEventsCCF-462.4MB ± 0%62.4MB ± 0%~(p=1.000 n=1+1)
EncodeBatchEventsJSON-439.1MB ± 0%39.1MB ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowFees.FeesDeducted-41.22kB ± 0%1.22kB ± 0%~(all equal)
EncodeCCF/FlowFees.TokensWithdrawn-41.17kB ± 0%1.17kB ± 0%~(all equal)
EncodeCCF/FlowIDTableStaking.DelegatorRewardsPaid-41.44kB ± 0%1.44kB ± 0%~(all equal)
EncodeCCF/FlowIDTableStaking.EpochTotalRewardsPaid-41.41kB ± 0%1.41kB ± 0%~(all equal)
EncodeCCF/FlowIDTableStaking.NewWeeklyPayout-41.34kB ± 0%1.34kB ± 0%~(all equal)
EncodeCCF/FlowIDTableStaking.RewardsPaid-41.42kB ± 0%1.42kB ± 0%~(all equal)
EncodeCCF/FlowToken.TokensDeposited-41.22kB ± 0%1.22kB ± 0%~(all equal)
EncodeCCF/FlowToken.TokensDeposited_with_nil_receiver-41.20kB ± 0%1.20kB ± 0%~(all equal)
EncodeCCF/FlowToken.TokensMinted-41.17kB ± 0%1.17kB ± 0%~(all equal)
EncodeCCF/FlowToken.TokensWithdrawn-41.22kB ± 0%1.22kB ± 0%~(all equal)
EncodeJSON/FlowFees.FeesDeducted-4864B ± 0%864B ± 0%~(all equal)
EncodeJSON/FlowFees.TokensWithdrawn-4504B ± 0%504B ± 0%~(all equal)
EncodeJSON/FlowIDTableStaking.DelegatorRewardsPaid-4888B ± 0%888B ± 0%~(all equal)
EncodeJSON/FlowIDTableStaking.EpochTotalRewardsPaid-41.08kB ± 0%1.08kB ± 0%~(all equal)
EncodeJSON/FlowIDTableStaking.NewWeeklyPayout-4552B ± 0%552B ± 0%~(all equal)
EncodeJSON/FlowIDTableStaking.RewardsPaid-4752B ± 0%752B ± 0%~(all equal)
EncodeJSON/FlowToken.TokensDeposited-4776B ± 0%776B ± 0%~(all equal)
EncodeJSON/FlowToken.TokensDeposited_with_nil_receiver-4640B ± 0%640B ± 0%~(all equal)
EncodeJSON/FlowToken.TokensMinted-4512B ± 0%512B ± 0%~(all equal)
EncodeJSON/FlowToken.TokensWithdrawn-4768B ± 0%768B ± 0%~(all equal)
ExportType/composite_type-4120B ± 0%120B ± 0%~(all equal)
ExportType/simple_type-40.00B 0.00B ~(all equal)
InterpretRecursionFib-41.00MB ± 0%1.00MB ± 0%~(all equal)
NewInterpreter/new_interpreter-4928B ± 0%928B ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-4200B ± 0%200B ± 0%~(all equal)
ParseArray-42.65MB ± 0%2.76MB ± 0%~(p=1.000 n=1+1)
ParseDeploy/byte_array-44.22MB ± 0%4.22MB ± 0%~(p=1.000 n=1+1)
ParseDeploy/decode_hex-4214kB ± 0%214kB ± 0%~(p=1.000 n=1+1)
ParseFungibleToken/With_memory_metering-429.7kB ± 0%29.7kB ± 0%~(p=1.000 n=1+1)
ParseFungibleToken/Without_memory_metering-429.7kB ± 0%29.7kB ± 0%~(p=1.000 n=1+1)
ParseInfix-41.92kB ± 0%1.93kB ± 0%~(p=1.000 n=1+1)
QualifiedIdentifierCreation/One_level-40.00B 0.00B ~(all equal)
QualifiedIdentifierCreation/Three_levels-464.0B ± 0%64.0B ± 0%~(all equal)
RuntimeScriptNoop-43.21kB ± 0%3.21kB ± 0%~(all equal)
SuperTypeInference/arrays-496.0B ± 0%96.0B ± 0%~(all equal)
SuperTypeInference/composites-40.00B 0.00B ~(all equal)
SuperTypeInference/integers-40.00B 0.00B ~(all equal)
ValueIsSubtypeOfSemaType-448.0B ± 0%48.0B ± 0%~(all equal)
 
allocs/opdelta
DecodeBatchEventsCCF-41.48M ± 0%1.48M ± 0%~(p=1.000 n=1+1)
DecodeBatchEventsJSON-44.70M ± 0%4.70M ± 0%~(p=1.000 n=1+1)
DecodeCCF/FlowFees.FeesDeducted-430.0 ± 0%30.0 ± 0%~(all equal)
DecodeCCF/FlowFees.TokensWithdrawn-426.0 ± 0%26.0 ± 0%~(all equal)
DecodeCCF/FlowIDTableStaking.DelegatorRewardsPaid-430.0 ± 0%30.0 ± 0%~(all equal)
DecodeCCF/FlowIDTableStaking.EpochTotalRewardsPaid-432.0 ± 0%32.0 ± 0%~(all equal)
DecodeCCF/FlowIDTableStaking.NewWeeklyPayout-426.0 ± 0%26.0 ± 0%~(all equal)
DecodeCCF/FlowIDTableStaking.RewardsPaid-429.0 ± 0%29.0 ± 0%~(all equal)
DecodeCCF/FlowToken.TokensDeposited-431.0 ± 0%31.0 ± 0%~(all equal)
DecodeCCF/FlowToken.TokensDeposited_with_nil_receiver-429.0 ± 0%29.0 ± 0%~(all equal)
DecodeCCF/FlowToken.TokensMinted-426.0 ± 0%26.0 ± 0%~(all equal)
DecodeCCF/FlowToken.TokensWithdrawn-431.0 ± 0%31.0 ± 0%~(all equal)
DecodeJSON/FlowFees.FeesDeducted-4126 ± 0%126 ± 0%~(all equal)
DecodeJSON/FlowFees.TokensWithdrawn-471.0 ± 0%71.0 ± 0%~(all equal)
DecodeJSON/FlowIDTableStaking.DelegatorRewardsPaid-4102 ± 0%102 ± 0%~(all equal)
DecodeJSON/FlowIDTableStaking.EpochTotalRewardsPaid-4159 ± 0%159 ± 0%~(all equal)
DecodeJSON/FlowIDTableStaking.NewWeeklyPayout-470.0 ± 0%70.0 ± 0%~(all equal)
DecodeJSON/FlowIDTableStaking.RewardsPaid-487.0 ± 0%87.0 ± 0%~(all equal)
DecodeJSON/FlowToken.TokensDeposited-495.0 ± 0%95.0 ± 0%~(all equal)
DecodeJSON/FlowToken.TokensDeposited_with_nil_receiver-486.0 ± 0%86.0 ± 0%~(all equal)
DecodeJSON/FlowToken.TokensMinted-471.0 ± 0%71.0 ± 0%~(all equal)
DecodeJSON/FlowToken.TokensWithdrawn-495.0 ± 0%95.0 ± 0%~(all equal)
EncodeBatchEventsCCF-4950k ± 0%950k ± 0%~(p=1.000 n=1+1)
EncodeBatchEventsJSON-4853k ± 0%853k ± 0%~(p=1.000 n=1+1)
EncodeCCF/FlowFees.FeesDeducted-419.0 ± 0%19.0 ± 0%~(all equal)
EncodeCCF/FlowFees.TokensWithdrawn-419.0 ± 0%19.0 ± 0%~(all equal)
EncodeCCF/FlowIDTableStaking.DelegatorRewardsPaid-419.0 ± 0%19.0 ± 0%~(all equal)
EncodeCCF/FlowIDTableStaking.EpochTotalRewardsPaid-419.0 ± 0%19.0 ± 0%~(all equal)
EncodeCCF/FlowIDTableStaking.NewWeeklyPayout-419.0 ± 0%19.0 ± 0%~(all equal)
EncodeCCF/FlowIDTableStaking.RewardsPaid-419.0 ± 0%19.0 ± 0%~(all equal)
EncodeCCF/FlowToken.TokensDeposited-420.0 ± 0%20.0 ± 0%~(all equal)
EncodeCCF/FlowToken.TokensDeposited_with_nil_receiver-420.0 ± 0%20.0 ± 0%~(all equal)
EncodeCCF/FlowToken.TokensMinted-419.0 ± 0%19.0 ± 0%~(all equal)
EncodeCCF/FlowToken.TokensWithdrawn-420.0 ± 0%20.0 ± 0%~(all equal)
EncodeJSON/FlowFees.FeesDeducted-419.0 ± 0%19.0 ± 0%~(all equal)
EncodeJSON/FlowFees.TokensWithdrawn-412.0 ± 0%12.0 ± 0%~(all equal)
EncodeJSON/FlowIDTableStaking.DelegatorRewardsPaid-416.0 ± 0%16.0 ± 0%~(all equal)
EncodeJSON/FlowIDTableStaking.EpochTotalRewardsPaid-425.0 ± 0%25.0 ± 0%~(all equal)
EncodeJSON/FlowIDTableStaking.NewWeeklyPayout-412.0 ± 0%12.0 ± 0%~(all equal)
EncodeJSON/FlowIDTableStaking.RewardsPaid-415.0 ± 0%15.0 ± 0%~(all equal)
EncodeJSON/FlowToken.TokensDeposited-419.0 ± 0%19.0 ± 0%~(all equal)
EncodeJSON/FlowToken.TokensDeposited_with_nil_receiver-414.0 ± 0%14.0 ± 0%~(all equal)
EncodeJSON/FlowToken.TokensMinted-413.0 ± 0%13.0 ± 0%~(all equal)
EncodeJSON/FlowToken.TokensWithdrawn-418.0 ± 0%18.0 ± 0%~(all equal)
ExportType/composite_type-43.00 ± 0%3.00 ± 0%~(all equal)
ExportType/simple_type-40.00 0.00 ~(all equal)
InterpretRecursionFib-418.9k ± 0%18.9k ± 0%~(all equal)
NewInterpreter/new_interpreter-415.0 ± 0%15.0 ± 0%~(all equal)
NewInterpreter/new_sub-interpreter-44.00 ± 0%4.00 ± 0%~(all equal)
ParseArray-459.6k ± 0%59.6k ± 0%~(p=1.000 n=1+1)
ParseDeploy/byte_array-489.4k ± 0%89.4k ± 0%~(p=1.000 n=1+1)
ParseDeploy/decode_hex-463.0 ± 0%63.0 ± 0%~(all equal)
ParseFungibleToken/With_memory_metering-4778 ± 0%778 ± 0%~(all equal)
ParseFungibleToken/Without_memory_metering-4778 ± 0%778 ± 0%~(all equal)
ParseInfix-448.0 ± 0%48.0 ± 0%~(all equal)
QualifiedIdentifierCreation/One_level-40.00 0.00 ~(all equal)
QualifiedIdentifierCreation/Three_levels-42.00 ± 0%2.00 ± 0%~(all equal)
RuntimeScriptNoop-451.0 ± 0%51.0 ± 0%~(all equal)
SuperTypeInference/arrays-43.00 ± 0%3.00 ± 0%~(all equal)
SuperTypeInference/composites-40.00 0.00 ~(all equal)
SuperTypeInference/integers-40.00 0.00 ~(all equal)
ValueIsSubtypeOfSemaType-41.00 ± 0%1.00 ± 0%~(all equal)
 

Copy link

codecov bot commented Dec 20, 2023

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (bf15275) 80.21% compared to head (b7378f4) 80.29%.
Report is 3 commits behind head on master.

Files Patch % Lines
runtime/stdlib/random.go 92.16% 17 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2986      +/-   ##
==========================================
+ Coverage   80.21%   80.29%   +0.08%     
==========================================
  Files         359      359              
  Lines       84285    84484     +199     
==========================================
+ Hits        67607    67837     +230     
+ Misses      14247    14223      -24     
+ Partials     2431     2424       -7     
Flag Coverage Δ
unittests 80.29% <92.68%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tarakby tarakby changed the title Tarak/2712 revertible random modulo Implement revertibleRandom with modulo argument Dec 22, 2023
@tarakby tarakby changed the title Implement revertibleRandom with modulo argument Implement revertibleRandom with a modulo argument Dec 22, 2023
runtime/runtime_test.go Outdated Show resolved Hide resolved
@tarakby tarakby marked this pull request as ready for review December 22, 2023 22:32
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Great work!

runtime/stdlib/random.go Outdated Show resolved Hide resolved
runtime/stdlib/random.go Outdated Show resolved Hide resolved
Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Nice!

runtime/stdlib/random.go Outdated Show resolved Hide resolved
runtime/runtime_test.go Outdated Show resolved Hide resolved
@turbolent turbolent changed the title Implement revertibleRandom with a modulo argument Implement modulo argument of revertibleRandom Jan 4, 2024
@turbolent
Copy link
Member

@AlexHentschel Could you please review the implementation?

@turbolent
Copy link
Member

I improved the tests in 0f775bc: Each test case (which potentially might run in concurrently) should use its own Runtime, and all sub-tests are now parallelized

Copy link
Member

@turbolent turbolent left a comment

Choose a reason for hiding this comment

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

Looks good now, great work!

Should be ready to get merged once the memory metering has been added.

@turbolent turbolent enabled auto-merge January 9, 2024 17:40
@turbolent
Copy link
Member

@dsainati1 @SupunS Could you also please have a look?

@turbolent turbolent disabled auto-merge January 9, 2024 17:41
@tarakby
Copy link
Contributor Author

tarakby commented Jan 9, 2024

@thanks @turbolent for the comments and commits.
I am still making changes to optimize the long test and enable more type testing (other than the metering). Please do not review till I see how all these changes work on CI. I will ping you once the PR is ready to be merged.

@tarakby tarakby requested a review from turbolent January 9, 2024 19:49
@turbolent
Copy link
Member

@tarakby 1f0042f looks good 👌

@turbolent
Copy link
Member

turbolent commented Jan 9, 2024

@tarakby can you please have a look at the new commits?

  • 325e3d4 reverts 395fb0a, the batching isn't necessary in the optimized test
  • 83ff4f9 improves the tests a bit, and also removes the statistical tests from the runtime tests (they are brought back at the end)
  • 8e0bfad just refactors out the revertible random function out of the host function wrapper, so it can be called with "normal" arguments instead of having to construct an interpreter invocation
  • 8131b27 re-adds the statistical tests. Instead of running a script, it directly calls the revertible random function, which is much more efficient. It also allows testing all types

@tarakby
Copy link
Contributor Author

tarakby commented Jan 10, 2024

Nice, thanks @turbolent 👌🏼
I am fine with 8131b27. Although nothing really enforces that revertibleRandom calls the tested stdlib function under the hood, It's a good compromise to keep the statistical test in CI.

I added a minor improvement in 41117f8. The PR is ready for merging.

@turbolent
Copy link
Member

@tarakby Sounds good! 41117f8 looks good 👌

Agreed, the PR should be ready for review now. @dsainati1 @SupunS could you please have a final look?

@turbolent turbolent merged commit 934a7af into master Jan 16, 2024
12 of 14 checks passed
@turbolent turbolent deleted the tarak/2712-revertibleRandom-modulo branch January 16, 2024 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Language Breaking Change Breaks Cadence contracts deployed on Mainnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update revertibleRandom interface (FLIP 120)
5 participants