Merge pull request #32 from onflow/gio/fix-get-number-in-range

Fix `RandomConsumer.getNumberInRange()`

contracts/RandomConsumer.cdc

@@ -28,7 +28,7 @@ access(all) contract RandomConsumer {
     ///////////////////
 
     /// Retrieves a revertible random number in the range [min, max]. By leveraging the Cadence's revertibleRandom
-    /// method, this function ensures that the random number is generated within range without risk of bias.
+    /// method, this function ensures that the random number is generated within range without risk of modulo bias.
     ///
     /// @param min: The minimum value of the range
     /// @param max: The maximum value of the range
@@ -39,21 +39,22 @@ access(all) contract RandomConsumer {
         return min + revertibleRandom<UInt64>(modulo: max - min + 1)
     }
 
-    /// Retrieves a random number in the range [min, max] using the provided PRG
-    /// to source additional randomness if needed
+    /// Retrieves a random number in the range [min, max] using the provided PRG reference to source additional
+    /// randomness if needed. This method is implemented to avoid risk of modulo bias. Passing the PRG by reference
+    /// ensures that its state is advanced and numbers proceed down the PRG's random walk.
     ///
-    /// @param prg: The PRG to use for random number generation
+    /// @param prg: The PRG (passed by reference) to use for random number generation
     /// @param min: The minimum value of the range
     /// @param max: The maximum value of the range
     ///
     /// @return A random number in the range [min, max]
     ///
-    access(all) fun getNumberInRange(prg: Xorshift128plus.PRG, min: UInt64, max: UInt64): UInt64 {
+    access(all) fun getNumberInRange(prg: &Xorshift128plus.PRG, min: UInt64, max: UInt64): UInt64 {
         pre {
             min < max:
-                "RandomConsumer.getNumberInRange: Cannot get random number with the provided range! "
-                .concat(" The min must be less than the max. Provided min of ")
-                .concat(min.toString()).concat(" and max of ".concat(max.toString()))
+            "RandomConsumer.getNumberInRange: Cannot get random number with the provided range! "
+            .concat(" The min must be less than the max. Provided min of ")
+            .concat(min.toString()).concat(" and max of ".concat(max.toString()))
         }
         let range = max - min // Calculate the inclusive range of the random number
         let bitsRequired = UInt256(self._mostSignificantBit(range)) // Number of bits needed to cover the range
@@ -255,7 +256,8 @@ access(all) contract RandomConsumer {
 
             // Create PRG from the provided request & generate a random number & generate a random number in the range
             let prg = self._getPRGFromRequest(request: <-request)
-            let res = RandomConsumer.getNumberInRange(prg: prg, min: min, max: max)
+            let prgRef: &Xorshift128plus.PRG = &prg
+            let res = RandomConsumer.getNumberInRange(prg: prgRef, min: min, max: max)
 
             emit RandomnessFulfilled(requestUUID: reqUUID, randomResult: res)
 

flow.json

@@ -12,6 +12,7 @@
 			"source": "./contracts/RandomConsumer.cdc",
 			"aliases": {
 				"emulator": "f8d6e0586b0a20c7",
+				"mainnet": "45caec600164c9e6",
 				"testing": "0000000000000007",
 				"testnet": "ed24dbe901028c5c"
 			}

tests/random_consumer_tests.cdc

@@ -42,7 +42,6 @@ fun testRequestRandomnessSucceeds() {
     Test.expect(requestCanFulfillRes, Test.beSucceeded())
     let requestHeight = requestHeightRes.returnValue! as! UInt64
     let requestCanFulfill = requestCanFulfillRes.returnValue! as! Bool
-
 
     Test.assertEqual(expectedHeight, requestHeight)
     Test.assertEqual(false, requestCanFulfill)
@@ -63,3 +62,12 @@ fun testFulfillRandomnessSucceeds() {
     let fulfillRes = executeTransaction("./transactions/fulfill_random_request.cdc", [requestStoragePath], signer)
     Test.expect(fulfillRes, Test.beSucceeded())
 }
+
+access(all)
+fun testGetNumberInRangeUpdatesStateSucceeds() {
+    let diffPRGRes = executeScript("./scripts/prg_state_advances_on_range_results.cdc", [])
+    Test.expect(diffPRGRes, Test.beSucceeded())
+
+    let diff = diffPRGRes.returnValue! as! Bool
+    Test.assertEqual(true, diff)
+}

tests/scripts/prg_state_advances_on_range_results.cdc

@@ -0,0 +1,30 @@
+import "RandomBeaconHistory"
+import "RandomConsumer"
+import "Xorshift128plus"
+
+/// Test script to ensure PRG state advances across calls to
+access(all)
+fun main(): Bool {
+    // Instantiate PRG
+    let sor = RandomBeaconHistory.sourceOfRandomness(atBlockHeight: getCurrentBlock().height - 1)
+    let salt = UInt64.max
+    let prg = Xorshift128plus.PRG(sourceOfRandomness: sor.value, salt: salt.toBigEndianBytes())
+    // Reference new PRG
+    let prgRef: &Xorshift128plus.PRG = &prg
+
+    // Call params
+    let min: UInt64 = 0
+    let max: UInt64 = 100
+
+    // Ensure PRG state changes between calls
+    let preState0 = prg.state0
+    let preState1 = prg.state1
+
+    RandomConsumer.getNumberInRange(prg: prgRef, min: min, max: max)
+
+    let postState0 = prg.state0
+    let postState1 = prg.state1
+
+    // Ensure state on the PRG struct changed as a result of the getNumberInRange() call
+    return preState0 != postState0 && preState1 != postState1
+}