Skip to content

SIP-209 Update feeRateForExchange function signature #1686

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.

Sign up for GitHub

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

Merged
jjgonecrypto merged 10 commits into from
Feb 5, 2022
Merged

SIP-209 Update feeRateForExchange function signature #1686

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c8bbfb2
Update feeRateForExchange signature
barrasso Feb 4, 2022
5e5780a
update revert message
barrasso Feb 4, 2022
ed2947a
update setup.js
barrasso Feb 4, 2022
74cc601
update releases.json
barrasso Feb 4, 2022
2b63abe
fixing publish tests
jjgonecrypto Feb 4, 2022
27b129c
Fixing tests
jjgonecrypto Feb 4, 2022
bfdbb27
Merge branch 'develop' into exchanger-lyra-fix
jjgonecrypto Feb 4, 2022
147641c
Fixing remaining tests
jjgonecrypto Feb 5, 2022
b61bb58
Trying to fix coverage
jjgonecrypto Feb 5, 2022
41cd893
Commentation
jjgonecrypto Feb 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 4 additions & 6 deletions contracts/Exchanger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -784,12 +784,10 @@ contract Exchanger is Owned, MixinSystemSettings, IExchanger {
/// @param sourceCurrencyKey The source currency key
/// @param destinationCurrencyKey The destination currency key
/// @return The exchange fee rate, and whether the rates are too volatile
function feeRateForExchange(bytes32 sourceCurrencyKey, bytes32 destinationCurrencyKey)
external
view
returns (uint feeRate, bool tooVolatile)
{
return _feeRateForExchange(sourceCurrencyKey, destinationCurrencyKey);
function feeRateForExchange(bytes32 sourceCurrencyKey, bytes32 destinationCurrencyKey) external view returns (uint) {
(uint feeRate, bool tooVolatile) = _feeRateForExchange(sourceCurrencyKey, destinationCurrencyKey);
require(!tooVolatile, "too volatile");
return feeRate;
}

/// @notice public function to get the dynamic fee rate for a given exchange
Expand Down
5 changes: 1 addition & 4 deletions contracts/interfaces/IExchanger.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,7 @@ interface IExchanger {

function hasWaitingPeriodOrSettlementOwing(address account, bytes32 currencyKey) external view returns (bool);

function feeRateForExchange(bytes32 sourceCurrencyKey, bytes32 destinationCurrencyKey)
external
view
returns (uint exchangeFeeRate, bool tooVolatile);
function feeRateForExchange(bytes32 sourceCurrencyKey, bytes32 destinationCurrencyKey) external view returns (uint);

function dynamicFeeRateForExchange(bytes32 sourceCurrencyKey, bytes32 destinationCurrencyKey)
external
Expand Down
6 changes: 3 additions & 3 deletions hardhat.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ require('dotenv').config();

const path = require('path');

/// for some weird reason the order of these imports is important:
/// ./hardhat needs to be imported after hardhat-interact (error otherwise)
/// the order of these imports is important (due to custom overrides):
/// ./hardhat needs to be imported after hardhat-interact and after solidity-coverage.
/// and hardhat-gas-reporter needs to be imported after ./hardhat (otherwise no gas reports)
require('hardhat-interact');
require('solidity-coverage');
require('./hardhat');
require('@nomiclabs/hardhat-truffle5');
require('@nomiclabs/hardhat-ethers');
require('solidity-coverage');
require('hardhat-gas-reporter');

const {
Expand Down
9 changes: 8 additions & 1 deletion hardhat/tasks/task-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ subtask(TASK_TEST_RUN_MOCHA_TESTS).setAction(async ({ testFiles }, { config }) =
return testFailures;
});

let coverage = false;

task('coverage').setAction(async (taskArguments, hre, runSuper) => {
coverage = true;
await runSuper(taskArguments);
});

task('test')
.addFlag('optimizer', 'Compile with the optimizer')
.addFlag('gas', 'Compile gas usage')
Expand Down Expand Up @@ -57,7 +64,7 @@ task('test')

// When using CircleCI, output the test metadata
// See https://circleci.com/docs/2.0/collect-test-data
if (isCI) {
if (isCI && !coverage) {
hre.config.mocha.reporter = 'mocha-junit-reporter';
hre.config.mocha.reporterOptions = {
mochaFile: '/tmp/junit/test-results.[hash].xml',
Expand Down
5 changes: 5 additions & 0 deletions publish/releases.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,11 @@
"layer": "both",
"sources": ["FeePool"],
"released": "both"
},
{
"sip": 209,
"layer": "both",
"sources": ["Exchanger"]
}
],
"releases": [
Expand Down
140 changes: 62 additions & 78 deletions test/contracts/Exchanger.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ contract('Exchanger (spec tests)', async accounts => {
const itCalculatesFeeRateForExchange = () => {
describe('Given exchangeFeeRates are configured and when calling feeRateForExchange()', () => {
it('for two long synths, returns the regular exchange fee', async () => {
const actualFeeRate = (await exchanger.feeRateForExchange(sEUR, sBTC))[0];
const actualFeeRate = await exchanger.feeRateForExchange(sEUR, sBTC);
assert.bnEqual(actualFeeRate, exchangeFeeRate, 'Rate must be the exchange fee rate');
});
});
Expand Down Expand Up @@ -507,7 +507,7 @@ contract('Exchanger (spec tests)', async accounts => {
assert.bnEqual(destinationFee, exchangeFeeIncurred(effectiveValue, bipsCrypto));
});
it('then return the feeRate', async () => {
const exchangeFeeRate = (await exchanger.feeRateForExchange(sUSD, sBTC))[0];
const exchangeFeeRate = await exchanger.feeRateForExchange(sUSD, sBTC);
assert.bnEqual(feeRate, exchangeFeeRate);
});
});
Expand Down Expand Up @@ -536,7 +536,7 @@ contract('Exchanger (spec tests)', async accounts => {
assert.bnEqual(destinationFee, exchangeFeeIncurred(effectiveValue, bipsFX));
});
it('then return the feeRate', async () => {
const exchangeFeeRate = (await exchanger.feeRateForExchange(sUSD, sEUR))[0];
const exchangeFeeRate = await exchanger.feeRateForExchange(sUSD, sEUR);
assert.bnEqual(feeRate, exchangeFeeRate);
});
});
Expand Down Expand Up @@ -585,120 +585,110 @@ contract('Exchanger (spec tests)', async accounts => {
const maxDynamicFeeRate = toBN(EXCHANGE_MAX_DYNAMIC_FEE);

it('initial fee is correct', async () => {
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sBTC), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sBTC), bipsCrypto);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sBTC), [0, false]);
});

describe('fee is caluclated correctly when rates spike or drop', () => {
describe('fee is calculated correctly when rates spike or drop', () => {
it('.3% spike is below threshold', async () => {
await updateRates([sETH], [toUnit(100.3)]);
// spike
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sETH), bipsCrypto);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [0, false]);
// control
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sBTC), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sBTC), bipsCrypto);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sBTC, sBTC), [0, false]);
});

it('.3% drop is below threshold', async () => {
await updateRates([sETH], [toUnit(99.7)]);
// spike
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sETH), bipsCrypto);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [0, false]);
// control
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sBTC), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sBTC), bipsCrypto);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sBTC, sBTC), [0, false]);
});

it('1% spike result in correct dynamic fee', async () => {
await updateRates([sETH], [toUnit(101)]);
// price diff ratio (1%)- threshold
const expectedDynamicFee = toUnit(0.01).sub(threshold);
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [
bipsCrypto.add(expectedDynamicFee),
false,
]);
assert.bnEqual(
await exchanger.feeRateForExchange(sUSD, sETH),
bipsCrypto.add(expectedDynamicFee)
);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [
expectedDynamicFee,
false,
]);
// control
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sBTC), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sBTC), bipsCrypto);
});

it('1% drop result in correct dynamic fee', async () => {
await updateRates([sETH], [toUnit(99)]);
// price diff ratio (1%)- threshold
const expectedDynamicFee = toUnit(0.01).sub(threshold);
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [
bipsCrypto.add(expectedDynamicFee),
false,
]);
assert.bnEqual(
await exchanger.feeRateForExchange(sUSD, sETH),
bipsCrypto.add(expectedDynamicFee)
);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [
expectedDynamicFee,
false,
]);
// control
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sBTC), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sBTC), bipsCrypto);
});

it('5% spike result in correct dynamic fee', async () => {
await updateRates([sETH], [toUnit(105)]);
// price diff ratio (5%)- threshold
const expectedDynamicFee = toUnit(0.05).sub(threshold);
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [
bipsCrypto.add(expectedDynamicFee),
false,
]);
assert.bnEqual(
await exchanger.feeRateForExchange(sUSD, sETH),
bipsCrypto.add(expectedDynamicFee)
);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [
expectedDynamicFee,
false,
]);
// control
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sBTC), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sBTC), bipsCrypto);
});

it('5% drop result in correct dynamic fee', async () => {
await updateRates([sETH], [toUnit(95)]);
// price diff ratio (5%)- threshold
const expectedDynamicFee = toUnit(0.05).sub(threshold);
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [
bipsCrypto.add(expectedDynamicFee),
false,
]);
assert.bnEqual(
await exchanger.feeRateForExchange(sUSD, sETH),
bipsCrypto.add(expectedDynamicFee)
);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [
expectedDynamicFee,
false,
]);
// control
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sBTC), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sBTC), bipsCrypto);
});

it('10% spike is over the max and is too volatile', async () => {
await updateRates([sETH], [toUnit(110)]);
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [
bipsCrypto.add(maxDynamicFeeRate),
true,
]);
await assert.revert(exchanger.feeRateForExchange(sUSD, sETH), 'too volatile');
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [
maxDynamicFeeRate,
true,
]);
// view reverts
await assert.revert(
exchanger.getAmountsForExchange(toUnit('1'), sUSD, sETH),
'too volatile'
);

// control
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sBTC), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sBTC), bipsCrypto);
});

it('10% drop result in correct dynamic fee', async () => {
await updateRates([sETH], [toUnit(90)]);
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [
bipsCrypto.add(maxDynamicFeeRate),
true,
]);
await assert.revert(exchanger.feeRateForExchange(sUSD, sETH), 'too volatile');
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [
maxDynamicFeeRate,
true,
Expand All @@ -709,7 +699,7 @@ contract('Exchanger (spec tests)', async accounts => {
'too volatile'
);
// control
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sBTC), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sBTC), bipsCrypto);
});

it('trading between two spiked rates is correctly calculated ', async () => {
Expand All @@ -718,19 +708,19 @@ contract('Exchanger (spec tests)', async accounts => {
const expectedDynamicFee = toUnit(0.02)
.sub(threshold)
.mul(toBN(2));
assert.deepEqual(await exchanger.feeRateForExchange(sBTC, sETH), [
bipsCrypto.add(expectedDynamicFee),
false,
]);
assert.bnEqual(
await exchanger.feeRateForExchange(sBTC, sETH),
bipsCrypto.add(expectedDynamicFee)
);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sBTC, sETH), [
expectedDynamicFee,
false,
]);
// reverse direction is the same
assert.deepEqual(await exchanger.feeRateForExchange(sETH, sBTC), [
bipsCrypto.add(expectedDynamicFee),
false,
]);
assert.bnEqual(
await exchanger.feeRateForExchange(sETH, sBTC),
bipsCrypto.add(expectedDynamicFee)
);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sETH, sBTC), [
expectedDynamicFee,
false,
Expand All @@ -740,19 +730,13 @@ contract('Exchanger (spec tests)', async accounts => {
it('trading between two spiked respects max fee and volatility flag', async () => {
// spike each 3% so that total dynamic fee is 6% which is more than the max
await updateRates([sETH, sBTC], [toUnit(103), toUnit(5150)]);
assert.deepEqual(await exchanger.feeRateForExchange(sBTC, sETH), [
bipsCrypto.add(maxDynamicFeeRate),
true,
]);
await assert.revert(exchanger.feeRateForExchange(sBTC, sETH), 'too volatile');
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sBTC, sETH), [
maxDynamicFeeRate,
true,
]);
// reverse direction is the same
assert.deepEqual(await exchanger.feeRateForExchange(sETH, sBTC), [
bipsCrypto.add(maxDynamicFeeRate),
true,
]);
await assert.revert(exchanger.feeRateForExchange(sETH, sBTC), 'too volatile');
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sETH, sBTC), [
maxDynamicFeeRate,
true,
Expand Down Expand Up @@ -810,10 +794,10 @@ contract('Exchanger (spec tests)', async accounts => {
await updateRates([sETH], [toUnit(105)]);
// (price diff ratio (5%)- threshold)
let expectedDynamicFee = toUnit(0.05).sub(threshold);
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [
bipsCrypto.add(expectedDynamicFee),
false,
]);
assert.bnEqual(
await exchanger.feeRateForExchange(sUSD, sETH),
bipsCrypto.add(expectedDynamicFee)
);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [
expectedDynamicFee,
false,
Expand All @@ -824,10 +808,10 @@ contract('Exchanger (spec tests)', async accounts => {
// next round
await updateRates([sETH], [toUnit(105)]);
expectedDynamicFee = multiplyDecimal(expectedDynamicFee, decay);
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [
bipsCrypto.add(expectedDynamicFee),
false,
]);
assert.bnEqual(
await exchanger.feeRateForExchange(sUSD, sETH),
bipsCrypto.add(expectedDynamicFee)
);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [
expectedDynamicFee,
false,
Expand All @@ -836,10 +820,10 @@ contract('Exchanger (spec tests)', async accounts => {
// another round
await updateRates([sETH], [toUnit(105)]);
expectedDynamicFee = multiplyDecimal(expectedDynamicFee, decay);
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [
bipsCrypto.add(expectedDynamicFee),
false,
]);
assert.bnEqual(
await exchanger.feeRateForExchange(sUSD, sETH),
bipsCrypto.add(expectedDynamicFee)
);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [
expectedDynamicFee,
false,
Expand All @@ -849,7 +833,7 @@ contract('Exchanger (spec tests)', async accounts => {
for (let i = 0; i < EXCHANGE_DYNAMIC_FEE_ROUNDS - 3; i++) {
await updateRates([sETH], [toUnit(105)]);
}
assert.deepEqual(await exchanger.feeRateForExchange(sUSD, sETH), [bipsCrypto, false]);
assert.bnEqual(await exchanger.feeRateForExchange(sUSD, sETH), bipsCrypto);
assert.deepEqual(await exchanger.dynamicFeeRateForExchange(sUSD, sETH), [0, false]);
});
});
Expand Down Expand Up @@ -3725,7 +3709,7 @@ contract('Exchanger (spec tests)', async accounts => {
await systemSettings.setExchangeFeeRateForSynths([sUSD], [newFxBIPS], {
from: owner,
});
const sUSDRate = (await exchanger.feeRateForExchange(empty, sUSD))[0];
const sUSDRate = await exchanger.feeRateForExchange(empty, sUSD);
assert.bnEqual(sUSDRate, newFxBIPS);
});

Expand All @@ -3739,13 +3723,13 @@ contract('Exchanger (spec tests)', async accounts => {
}
);
// Read all rates
const sAUDRate = (await exchanger.feeRateForExchange(empty, sAUD))[0];
const sAUDRate = await exchanger.feeRateForExchange(empty, sAUD);
assert.bnEqual(sAUDRate, newFxBIPS);
const sUSDRate = (await exchanger.feeRateForExchange(empty, sUSD))[0];
const sUSDRate = await exchanger.feeRateForExchange(empty, sUSD);
assert.bnEqual(sUSDRate, newFxBIPS);
const sBTCRate = (await exchanger.feeRateForExchange(empty, sBTC))[0];
const sBTCRate = await exchanger.feeRateForExchange(empty, sBTC);
assert.bnEqual(sBTCRate, newCryptoBIPS);
const sETHRate = (await exchanger.feeRateForExchange(empty, sETH))[0];
const sETHRate = await exchanger.feeRateForExchange(empty, sETH);
assert.bnEqual(sETHRate, newCryptoBIPS);
});
});
Expand Down
Loading