Fixing selection default mechanism (#718)

packages/common/src/types/Aliases.ts

@@ -7,6 +7,7 @@ import { RelayFailureInfo } from './RelayFailureInfo'
 import { RelayRegisteredEventInfo } from './GSNContractsDataTypes'
 import { HttpProvider, IpcProvider, WebsocketProvider } from 'web3-core'
 import { JsonRpcPayload, JsonRpcResponse } from 'web3-core-helpers'
+import BN from 'bn.js'
 
 export type Address = string
 export type EventName = string
@@ -20,7 +21,7 @@ export type PingFilter = (pingResponse: PingResponse, gsnTransactionDetails: Gsn
 export type AsyncDataCallback = (relayRequest: RelayRequest) => Promise<PrefixedHexString>
 
 export type RelayFilter = (registeredEventInfo: RelayRegisteredEventInfo) => boolean
-export type AsyncScoreCalculator = (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]) => Promise<number>
+export type AsyncScoreCalculator = (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]) => Promise<BN>
 
 export function notNull<TValue> (value: TValue | null | undefined): value is TValue {
   return value !== null && value !== undefined

packages/dev/test/provider/KnownRelaysManager.test.ts

@@ -3,7 +3,7 @@ import { ChildProcessWithoutNullStreams } from 'child_process'
 import { HttpProvider } from 'web3-core'
 import { ether } from '@openzeppelin/test-helpers'
 
-import { KnownRelaysManager, DefaultRelayScore } from '@opengsn/provider/dist/KnownRelaysManager'
+import { KnownRelaysManager, DefaultRelayScore, DefaultRelayFilter } from '@opengsn/provider/dist/KnownRelaysManager'
 import { ContractInteractor } from '@opengsn/common/dist/ContractInteractor'
 import { GSNConfig } from '@opengsn/provider/dist/GSNConfigurator'
 import {
@@ -21,6 +21,7 @@ import { RelayInfoUrl, RelayRegisteredEventInfo } from '@opengsn/common/dist/typ
 import { createClientLogger } from '@opengsn/provider/dist/ClientWinstonLogger'
 import { registerForwarderForGsn } from '@opengsn/common/dist/EIP712/ForwarderUtil'
 import { defaultEnvironment } from '@opengsn/common/dist/Environments'
+import { toBN } from 'web3-utils'
 
 const StakeManager = artifacts.require('StakeManager')
 const Penalizer = artifacts.require('Penalizer')
@@ -161,7 +162,7 @@ contract('KnownRelaysManager 2', function (accounts) {
   let logger: LoggerInterface
 
   const transactionDetails = {
-    gas: '0x10000',
+    gas: '0x10000000',
     gasPrice: '0x300000',
     from: '',
     data: '',
@@ -252,6 +253,23 @@ contract('KnownRelaysManager 2', function (accounts) {
       assert.equal(relays.length, 1)
       assert.equal(relays[0].relayUrl, 'stakeAndAuthorization2')
     })
+
+    it('should use DefaultRelayFilter to remove unsuitable relays when none was provided', async function () {
+      const knownRelaysManagerWithFilter = new KnownRelaysManager(contractInteractor, logger, config)
+      // @ts-ignore
+      assert.equal(knownRelaysManagerWithFilter.relayFilter.toString(), DefaultRelayFilter.toString())
+    })
+
+    describe('DefaultRelayFilter', function () {
+      it('should filter expensive relayers', function () {
+        const eventInfo = { relayUrl: 'url', relayManager: accounts[0] }
+        assert.isFalse(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '101', baseRelayFee: 1e16.toString() }))
+        assert.isFalse(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '99', baseRelayFee: 2e17.toString() }))
+        assert.isFalse(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '101', baseRelayFee: 2e17.toString() }))
+        assert.isTrue(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '100', baseRelayFee: 1e17.toString() }))
+        assert.isTrue(DefaultRelayFilter({ ...eventInfo, pctRelayFee: '50', baseRelayFee: '0' }))
+      })
+    })
   })
 
   describe('#getRelaysSortedForTransaction()', function () {
@@ -309,19 +327,24 @@ contract('KnownRelaysManager 2', function (accounts) {
         const relayScoreOneFailure = await DefaultRelayScore(relayInfoHighFee, transactionDetails, [failure])
         const relayScoreTenFailures = await DefaultRelayScore(relayInfoHighFee, transactionDetails, Array(10).fill(failure))
         const relayScoreLowFees = await DefaultRelayScore(relayInfoLowFee, transactionDetails, [])
-        assert.isAbove(relayScoreNoFailures, relayScoreOneFailure)
-        assert.isAbove(relayScoreOneFailure, relayScoreTenFailures)
-        assert.isAbove(relayScoreLowFees, relayScoreNoFailures)
+        assert.isTrue(relayScoreNoFailures.gt(relayScoreOneFailure))
+        assert.isTrue(relayScoreOneFailure.gt(relayScoreTenFailures))
+        assert.isTrue(relayScoreLowFees.gt(relayScoreNoFailures))
+      })
+      it('should use DefaultRelayScore to remove unsuitable relays when none was provided', async function () {
+        const knownRelaysManagerWithFilter = new KnownRelaysManager(contractInteractor, logger, configureGSN({}))
+        // @ts-ignore
+        assert.equal(knownRelaysManagerWithFilter.scoreCalculator.toString(), DefaultRelayScore.toString())
       })
     })
   })
 
   describe('getRelaysSortedForTransaction', function () {
-    const biasedRelayScore = async function (relay: RelayRegisteredEventInfo): Promise<number> {
+    const biasedRelayScore = async function (relay: RelayRegisteredEventInfo): Promise<BN> {
       if (relay.relayUrl === 'alex') {
-        return await Promise.resolve(1000)
+        return await Promise.resolve(toBN(1000))
       } else {
-        return await Promise.resolve(100)
+        return await Promise.resolve(toBN(100))
       }
     }
     const knownRelaysManager = new KnownRelaysManager(contractInteractor, logger, configureGSN({}), undefined, biasedRelayScore)

packages/dev/test/provider/RelaySelectionManager.test.ts

@@ -7,7 +7,7 @@ import { HttpClient } from '@opengsn/common/dist/HttpClient'
 import { HttpWrapper } from '@opengsn/common/dist/HttpWrapper'
 import { PingResponse } from '@opengsn/common/dist/PingResponse'
 import { RelaySelectionManager } from '@opengsn/provider/dist/RelaySelectionManager'
-import { EmptyFilter, KnownRelaysManager } from '@opengsn/provider/dist/KnownRelaysManager'
+import { DefaultRelayFilter, KnownRelaysManager } from '@opengsn/provider/dist/KnownRelaysManager'
 import { GasPricePingFilter } from '@opengsn/provider/dist/RelayClient'
 import { PartialRelayInfo, RelayInfo } from '@opengsn/common/dist/types/RelayInfo'
 import { PingFilter } from '@opengsn/common/dist/types/Aliases'
@@ -75,7 +75,7 @@ contract('RelaySelectionManager', function (accounts) {
         logger
       }).init()
       httpClient = new HttpClient(new HttpWrapper(), this.logger)
-      knownRelaysManager = new KnownRelaysManager(contractInteractor, logger, configureGSN({}), EmptyFilter)
+      knownRelaysManager = new KnownRelaysManager(contractInteractor, logger, configureGSN({}), DefaultRelayFilter)
       stubGetRelaysSorted = sinon.stub(knownRelaysManager, 'getRelaysSortedForTransaction')
       stubGetRelaysSorted.returns(Promise.resolve([[eventInfo]]))
       relaySelectionManager = await new RelaySelectionManager(transactionDetails, knownRelaysManager, httpClient, GasPricePingFilter, logger, config).init()
@@ -120,7 +120,7 @@ contract('RelaySelectionManager', function (accounts) {
         const penalizer = await Penalizer.new(defaultEnvironment.penalizerConfiguration.penalizeBlockDelay, defaultEnvironment.penalizerConfiguration.penalizeBlockExpiration)
         relayHub = await deployHub(stakeManager.address, penalizer.address)
         await stake(stakeManager, relayHub, relayManager, accounts[0])
-        await register(relayHub, relayManager, accounts[2], preferredRelayUrl, '666', '777')
+        await register(relayHub, relayManager, accounts[2], preferredRelayUrl, '666', '77')
 
         await contractInteractor.initDeployment({
           relayHubAddress: relayHub.address,
@@ -162,7 +162,7 @@ contract('RelaySelectionManager', function (accounts) {
         assert.equal(nextRelay.relayInfo.relayUrl, preferredRelayUrl)
         assert.equal(nextRelay.relayInfo.relayManager, relayManager)
         assert.equal(nextRelay.relayInfo.baseRelayFee, '666')
-        assert.equal(nextRelay.relayInfo.pctRelayFee, '777')
+        assert.equal(nextRelay.relayInfo.pctRelayFee, '77')
       })
     })
 

packages/provider/src/KnownRelaysManager.ts

@@ -16,22 +16,34 @@ import {
 } from '@opengsn/common/dist/types/GSNContractsDataTypes'
 import { LoggerInterface } from '@opengsn/common/dist/LoggerInterface'
 import { ContractInteractor } from '@opengsn/common/dist/ContractInteractor'
-
-export const EmptyFilter: RelayFilter = (): boolean => {
+import { MAX_INTEGER } from 'ethereumjs-util'
+import { toBN } from 'web3-utils'
+import BN from 'bn.js'
+
+export const DefaultRelayFilter: RelayFilter = function (registeredEventInfo: RelayRegisteredEventInfo): boolean {
+  const maxPctRelayFee = 100
+  const maxBaseRelayFee = 1e17
+  if (
+    parseInt(registeredEventInfo.pctRelayFee) > maxPctRelayFee ||
+    parseInt(registeredEventInfo.baseRelayFee) > maxBaseRelayFee
+  ) {
+    return false
+  }
   return true
 }
 /**
  * Basic score is reversed transaction fee, higher is better.
  * Relays that failed to respond recently will be downgraded for some period of time.
  */
-export const DefaultRelayScore = async function (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]): Promise<number> {
-  const gasLimit = parseInt(txDetails.gas ?? '0')
-  const gasPrice = parseInt(txDetails.gasPrice ?? '0')
-  const pctFee = parseInt(relay.pctRelayFee)
-  const baseFee = parseInt(relay.baseRelayFee)
-  const transactionCost = baseFee + (gasLimit * gasPrice * (100 + pctFee)) / 100
-  let score = Math.max(Number.MAX_SAFE_INTEGER - transactionCost, 0)
-  score = score * Math.pow(0.9, failures.length)
+export const DefaultRelayScore: AsyncScoreCalculator = async function (relay: RelayRegisteredEventInfo, txDetails: GsnTransactionDetails, failures: RelayFailureInfo[]): Promise<BN> {
+  const gasLimit = toBN(txDetails.gas ?? '0')
+  const gasPrice = toBN(txDetails.gasPrice ?? '0')
+  const pctFee = toBN(relay.pctRelayFee)
+  const baseFee = toBN(relay.baseRelayFee)
+  const transactionCost = baseFee.add(gasLimit.mul(gasPrice).muln((100 + pctFee.toNumber()) / 100)
+  )
+  let score = MAX_INTEGER.sub(transactionCost)
+  score = score.muln(Math.pow(0.9, failures.length))
   return score
 }
 
@@ -51,7 +63,7 @@ export class KnownRelaysManager {
   constructor (contractInteractor: ContractInteractor, logger: LoggerInterface, config: GSNConfig, relayFilter?: RelayFilter, scoreCalculator?: AsyncScoreCalculator) {
     this.config = config
     this.logger = logger
-    this.relayFilter = relayFilter ?? EmptyFilter
+    this.relayFilter = relayFilter ?? DefaultRelayFilter
     this.scoreCalculator = scoreCalculator ?? DefaultRelayScore
     this.contractInteractor = contractInteractor
   }
@@ -173,9 +185,9 @@ export class KnownRelaysManager {
   }
 
   async _sortRelaysInternal (gsnTransactionDetails: GsnTransactionDetails, activeRelays: RelayInfoUrl[]): Promise<RelayInfoUrl[]> {
-    const scores = new Map<string, number>()
+    const scores = new Map<string, BN>()
     for (const activeRelay of activeRelays) {
-      let score = 0
+      let score = toBN(0)
       if (isInfoFromEvent(activeRelay)) {
         const eventInfo = activeRelay as RelayRegisteredEventInfo
         score = await this.scoreCalculator(eventInfo, gsnTransactionDetails, this.relayFailures.get(activeRelay.relayUrl) ?? [])
@@ -187,9 +199,9 @@ export class KnownRelaysManager {
       .filter(isInfoFromEvent)
       .map(value => (value as RelayRegisteredEventInfo))
       .sort((a, b) => {
-        const aScore = scores.get(a.relayManager) ?? 0
-        const bScore = scores.get(b.relayManager) ?? 0
-        return bScore - aScore
+        const aScore = scores.get(a.relayManager) ?? toBN(0)
+        const bScore = scores.get(b.relayManager) ?? toBN(0)
+        return bScore.cmp(aScore)
       })
   }
 

packages/provider/src/RelayClient.ts

@@ -19,7 +19,7 @@ import { HttpClient } from '@opengsn/common/dist/HttpClient'
 import { HttpWrapper } from '@opengsn/common/dist/HttpWrapper'
 import { RelaySelectionManager } from './RelaySelectionManager'
 import { RelayedTransactionValidator } from './RelayedTransactionValidator'
-import { DefaultRelayScore, EmptyFilter, KnownRelaysManager } from './KnownRelaysManager'
+import { DefaultRelayScore, DefaultRelayFilter, KnownRelaysManager } from './KnownRelaysManager'
 import { createClientLogger } from './ClientWinstonLogger'
 import { defaultGsnConfig, defaultLoggerConfiguration, GSNConfig, GSNDependencies } from './GSNConfigurator'
 
@@ -448,7 +448,7 @@ export class RelayClient {
 
     const httpClient = overrideDependencies?.httpClient ?? new HttpClient(new HttpWrapper(), this.logger)
     const pingFilter = overrideDependencies?.pingFilter ?? GasPricePingFilter
-    const relayFilter = overrideDependencies?.relayFilter ?? EmptyFilter
+    const relayFilter = overrideDependencies?.relayFilter ?? DefaultRelayFilter
     const asyncApprovalData = overrideDependencies?.asyncApprovalData ?? EmptyDataCallback
     const asyncPaymasterData = overrideDependencies?.asyncPaymasterData ?? EmptyDataCallback
     const scoreCalculator = overrideDependencies?.scoreCalculator ?? DefaultRelayScore