-
Notifications
You must be signed in to change notification settings - Fork 374
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
Reserve supports multiple exchanges #6299
Changes from 1 commit
ee65bae
90dea2d
e2e8e91
4aa87f5
1dac950
065dcbb
bca92c8
1bd660c
d7ce70e
2852d80
0deb24d
c3269fd
11633d6
db37799
28a0c78
b94186a
e72df67
fdbdc9b
ae301d0
e5eadd7
de9472e
e96880b
98c85b2
0235c34
f3943b2
dc7c49b
108b86e
5ed889f
1bf4209
acbc501
b1a46e9
03ce0c1
89c6beb
0423759
588f0b7
5ede531
b439e76
721273a
dd97748
7da57a2
ecb3f3c
d163051
ffc785b
9dc4018
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -311,6 +311,11 @@ contract Reserve is | |
emit SpenderRemoved(spender); | ||
} | ||
|
||
function _isAllowedToSpendExchange(address spender) private view returns (bool) { | ||
return | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this function necessary? Please include a comment explaining why |
||
isExchangeSpender[spender] || (registry.getAddressForOrDie(EXCHANGE_REGISTRY_ID) == spender); | ||
} | ||
|
||
/** | ||
* @notice Gives an address permission to spend Reserve without limit. | ||
* @param spender The address that is allowed to spend Reserve funds. | ||
|
@@ -385,7 +390,7 @@ contract Reserve is | |
* @return Returns true if the transaction succeeds. | ||
*/ | ||
function transferExchangeGold(address payable to, uint256 value) external returns (bool) { | ||
martinvol marked this conversation as resolved.
Show resolved
Hide resolved
|
||
require(isExchangeSpender[msg.sender], "a non-exchangeSpender can't start a transfer"); | ||
require(_isAllowedToSpendExchange(msg.sender), "Address not allowed to spend"); | ||
return _transferGold(to, value); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ contract('Reserve', (accounts: string[]) => { | |
const nonOwner: string = accounts[1] | ||
const spender: string = accounts[2] | ||
const exchangeAddress: string = accounts[3] | ||
const exchangeSpenderAddress: string = accounts[4] | ||
const aTobinTaxStalenessThreshold: number = 600 | ||
const aTobinTax = toFixed(0.005) | ||
const aTobinTaxReserveRatio = toFixed(2) | ||
|
@@ -291,13 +292,13 @@ contract('Reserve', (accounts: string[]) => { | |
}) | ||
|
||
it('should emit addExchangeSpender event on add', async () => { | ||
const resp = await reserve.addExchangeSpender(exchangeAddress) | ||
const resp = await reserve.addExchangeSpender(exchangeSpenderAddress) | ||
const log = resp.logs[0] | ||
assert.equal(resp.logs.length, 1) | ||
assertLogMatches2(log, { | ||
event: 'ExchangeSpenderAdded', | ||
args: { | ||
exchangeSpender: exchangeAddress, | ||
exchangeSpender: exchangeSpenderAddress, | ||
}, | ||
}) | ||
}) | ||
|
@@ -314,57 +315,59 @@ contract('Reserve', (accounts: string[]) => { | |
|
||
describe('#removeExchangeSpender(exchangeAddress)', () => { | ||
beforeEach(async () => { | ||
await reserve.addExchangeSpender(exchangeAddress) | ||
await reserve.addExchangeSpender(exchangeSpenderAddress) | ||
}) | ||
|
||
it('only allows owner', async () => { | ||
await assertRevert(reserve.removeExchangeSpender(nonOwner, 0, { from: nonOwner })) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the first argument be |
||
}) | ||
|
||
// TODO should allow exchange | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remove TODO? |
||
|
||
it('should emit removeExchangeSpender event on remove', async () => { | ||
const resp = await reserve.removeExchangeSpender(exchangeAddress, 0) | ||
const resp = await reserve.removeExchangeSpender(exchangeSpenderAddress, 0) | ||
const log = resp.logs[0] | ||
assert.equal(resp.logs.length, 1) | ||
assertLogMatches2(log, { | ||
event: 'ExchangeSpenderRemoved', | ||
args: { | ||
exchangeSpender: exchangeAddress, | ||
exchangeSpender: exchangeSpenderAddress, | ||
}, | ||
}) | ||
}) | ||
|
||
it('has the right list of exchange spenders', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed? |
||
const spenders = await reserve.getExchangeSpenders() | ||
assert.equal(spenders.length, 1) | ||
assert.equal(spenders[0], exchangeAddress) | ||
assert.equal(spenders[0], exchangeSpenderAddress) | ||
}) | ||
|
||
it('has the right list of exchange after removing one', async () => { | ||
await reserve.removeExchangeSpender(exchangeAddress, 0) | ||
await reserve.removeExchangeSpender(exchangeSpenderAddress, 0) | ||
const spenders = await reserve.getExchangeSpenders() | ||
assert.equal(spenders.length, 0) | ||
}) | ||
|
||
it("can't be removed twice", async () => { | ||
await reserve.removeExchangeSpender(exchangeAddress, 0) | ||
await assertRevert(reserve.removeExchangeSpender(exchangeAddress, 0)) | ||
await reserve.removeExchangeSpender(exchangeSpenderAddress, 0) | ||
await assertRevert(reserve.removeExchangeSpender(exchangeSpenderAddress, 0)) | ||
}) | ||
|
||
it("can't delete an index out of range", async () => { | ||
await assertRevert(reserve.removeExchangeSpender(exchangeAddress, 1)) | ||
await assertRevert(reserve.removeExchangeSpender(exchangeSpenderAddress, 1)) | ||
}) | ||
|
||
it('removes from a big array', async () => { | ||
await reserve.addExchangeSpender(accounts[1]) | ||
await reserve.removeExchangeSpender(exchangeAddress, 0) | ||
await reserve.removeExchangeSpender(exchangeSpenderAddress, 0) | ||
const spenders = await reserve.getExchangeSpenders() | ||
assert.equal(spenders.length, 1) | ||
assert.equal(spenders[0], accounts[1]) | ||
}) | ||
|
||
it("doesn't remove an address with the wrong index", async () => { | ||
await reserve.addExchangeSpender(accounts[1]) | ||
assertRevert(reserve.removeExchangeSpender(exchangeAddress, 1)) | ||
assertRevert(reserve.removeExchangeSpender(exchangeSpenderAddress, 1)) | ||
}) | ||
}) | ||
martinvol marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
@@ -402,7 +405,6 @@ contract('Reserve', (accounts: string[]) => { | |
await web3.eth.sendTransaction({ to: reserve.address, value: aValue, from: accounts[0] }) | ||
await reserve.addSpender(spender) | ||
await reserve.addOtherReserveAddress(otherReserveAddress) | ||
await reserve.addExchangeSpender(exchangeAddress) | ||
}) | ||
|
||
it('should allow an exchange to call transferExchangeGold', async () => { | ||
|
@@ -418,8 +420,11 @@ contract('Reserve', (accounts: string[]) => { | |
}) | ||
|
||
it('should not allow removed exchange spender addresses to call transferExchangeGold', async () => { | ||
await reserve.removeExchangeSpender(exchangeAddress, 0) | ||
await assertRevert(reserve.transferExchangeGold(nonOwner, aValue, { from: exchangeAddress })) | ||
await reserve.addExchangeSpender(exchangeSpenderAddress) | ||
await reserve.removeExchangeSpender(exchangeSpenderAddress, 0) | ||
await assertRevert( | ||
reserve.transferExchangeGold(nonOwner, aValue, { from: exchangeSpenderAddress }) | ||
) | ||
}) | ||
|
||
it('should not allow freezing more gold than is available', async () => { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not making this public?