OG-658 Fixing selection default mechanism #718
Conversation
shahafn
changed the title
|
|
||
| export const DefaultRelayFilter: RelayFilter = function (registeredEventInfo: RelayRegisteredEventInfo): boolean { | ||
| const maxPctRelayFee = 100 | ||
| const maxBaseRelayFee = 1e17 |
There was a problem hiding this comment.
- should be exported constants
- on mainnet, might 0.1eth is a good value , but on (say) polygon, a maximum of 0.1 matic is no good. you might say "so the client should override the default" - but the relayers can't control the clients, and most clients will stick to defaults.
- So this makes the "maxBaseRelayFee" useless on cheap networks - even though I think it is more important there (relayer's cost has almost nothing to do with gas price, so a factor is less favorable)
There was a problem hiding this comment.
- Why should they be exported? it's a default implementation, by no means it has any bearing on other implementations, I don't see why we should export these.
| 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()) |
There was a problem hiding this comment.
did you check toString that its not constant for all functions?
There was a problem hiding this comment.
Yes, it is the function code as string.
| const baseFee = toBN(relay.baseRelayFee) | ||
| const transactionCost = baseFee.add(gasLimit.mul(gasPrice).muln((100 + pctFee.toNumber()) / 100) | ||
| ) | ||
| let score = MAX_INTEGER.sub(transactionCost) |
There was a problem hiding this comment.
Why do we want to make it the max possible number? we do want to have a number higher than max possible gas so 100eth gas price * arbitrum gas limit yields 1e20 * 1e10 ~ 1e30
our current score uses only negative parameters, but we should allow algorithms to increase the score. by using MAX_INTEGER we actively prevent it.
There was a problem hiding this comment.
(yes, and I also don't think using BN is good thing - it ties our code to some BigNumber implementation, and if/when we change to ethers, we to change the API (or carry a requirement for another BN library)
We don't need the "bit accuracy" of big numbers at all
There was a problem hiding this comment.
I don't understand your suggestion.
There was a problem hiding this comment.
forget it. what we need is weighted scoring, and this is not something we'll now.
No description provided.