diff --git a/contracts/javascore/aggregator/src/main/java/relay/aggregator/RelayAggregator.java b/contracts/javascore/aggregator/src/main/java/relay/aggregator/RelayAggregator.java index f4f41542..ec475a99 100644 --- a/contracts/javascore/aggregator/src/main/java/relay/aggregator/RelayAggregator.java +++ b/contracts/javascore/aggregator/src/main/java/relay/aggregator/RelayAggregator.java @@ -29,6 +29,7 @@ import score.annotation.External; import score.ObjectReader; import scorex.util.ArrayList; +import scorex.util.HashMap; public class RelayAggregator { private final Integer DEFAULT_SIGNATURE_THRESHOLD = 1; @@ -57,6 +58,8 @@ public RelayAggregator(Address _admin) { public void setAdmin(Address _admin) { adminOnly(); + Context.require(admin.get() != _admin, "admin already set"); + // add new admin as relayer addRelayer(_admin); @@ -74,7 +77,8 @@ public Address getAdmin() { @External public void setSignatureThreshold(int threshold) { adminOnly(); - Context.require(threshold >= 1, "invalid threshold value: should be >= 1"); + Context.require(threshold > 0 && threshold <= relayers.size(), + "threshold value should be at least 1 and not greater than relayers size"); signatureThreshold.set(threshold); } @@ -93,30 +97,29 @@ public Address[] getRelayers() { } @External - public void addRelayers(Address[] newRelayers) { + public void setRelayers(Address[] newRelayers, int threshold) { adminOnly(); - Context.require(newRelayers != null && newRelayers.length != 0, "new relayers cannot be empty"); - - for (Address newRelayer : newRelayers) { - Boolean exits = relayersLookup.get(newRelayer); - if (exits == null) { + if (newRelayers.length > 0) { + HashMap newRelayersMap = new HashMap(); + for (Address newRelayer : newRelayers) { + newRelayersMap.put(newRelayer, true); addRelayer(newRelayer); } - } - } - @External - public void removeRelayers(Address[] relayersToBeRemoved) { - adminOnly(); + Address adminAdrr = admin.get(); + for (int i = 0; i < relayers.size(); i++) { + Address oldRelayer = relayers.get(i); + if (!oldRelayer.equals(adminAdrr) && !newRelayersMap.containsKey(oldRelayer)) { + removeRelayer(oldRelayer); + } + } + } - Context.require(relayersToBeRemoved != null && relayersToBeRemoved.length != 0, - "relayers to be removed cannot be empty"); + Context.require(threshold > 0 && threshold <= relayers.size(), + "threshold value should be at least 1 and not greater than relayers size"); - for (Address relayerToBeRemoved : relayersToBeRemoved) { - Context.require(relayerToBeRemoved != admin.get(), "admin cannot be removed from relayers list"); - removeRelayer(relayerToBeRemoved); - } + signatureThreshold.set(threshold); } @External(readonly = true) @@ -248,17 +251,21 @@ private void relayersOnly() { } private void addRelayer(Address newRelayer) { - relayers.add(newRelayer); - relayersLookup.set(newRelayer, true); + if (relayersLookup.get(newRelayer) == null) { + relayers.add(newRelayer); + relayersLookup.set(newRelayer, true); + } } private void removeRelayer(Address oldRelayer) { - relayersLookup.set(oldRelayer, null); - Address top = relayers.pop(); - for (int i = 0; i < relayers.size(); i++) { - if (oldRelayer.equals(relayers.get(i))) { - relayers.set(i, top); - break; + if (relayersLookup.get(oldRelayer)) { + relayersLookup.set(oldRelayer, null); + Address top = relayers.pop(); + for (int i = 0; i < relayers.size(); i++) { + if (oldRelayer.equals(relayers.get(i))) { + relayers.set(i, top); + break; + } } } } diff --git a/contracts/javascore/aggregator/src/test/java/relay/aggregator/RelayAggregatorTest.java b/contracts/javascore/aggregator/src/test/java/relay/aggregator/RelayAggregatorTest.java index d44e84a2..0da3391f 100644 --- a/contracts/javascore/aggregator/src/test/java/relay/aggregator/RelayAggregatorTest.java +++ b/contracts/javascore/aggregator/src/test/java/relay/aggregator/RelayAggregatorTest.java @@ -10,6 +10,7 @@ import score.Address; import score.Context; import score.UserRevertedException; +import scorex.util.HashSet; import com.iconloop.score.test.Account; import com.iconloop.score.test.Score; @@ -69,7 +70,7 @@ void setup() throws Exception { Address[] relayers = new Address[] { adminAc.getAddress(), relayerOneAc.getAddress(), relayerTwoAc.getAddress(), relayerThreeAc.getAddress() }; - aggregator.invoke(adminAc, "addRelayers", (Object) relayers); + aggregator.invoke(adminAc, "setRelayers", (Object) relayers, 2); aggregatorSpy = (RelayAggregator) spy(aggregator.getInstance()); aggregator.setInstance(aggregatorSpy); @@ -118,68 +119,91 @@ public void testSetSignatureThreshold() { public void testSetSignatureThreshold_unauthorised() { int threshold = 3; - Executable action = () -> aggregator.invoke(relayerOneAc, "setSignatureThreshold", threshold); + Executable action = () -> aggregator.invoke(relayerOneAc, + "setSignatureThreshold", threshold); UserRevertedException e = assertThrows(UserRevertedException.class, action); - assertEquals("Reverted(0): Unauthorized: caller is not the leader relayer", e.getMessage()); + assertEquals("Reverted(0): Unauthorized: caller is not the leader relayer", + e.getMessage()); } @Test - public void testAddRelayers() { + public void testSetRelayers() { Account relayerFiveAc = sm.createAccount(); - Address[] newRelayers = new Address[] { relayerFourAc.getAddress(), relayerFiveAc.getAddress() }; + Address[] newRelayers = new Address[] { relayerThreeAc.getAddress(), relayerFourAc.getAddress(), + relayerFiveAc.getAddress() }; - aggregator.invoke(adminAc, "addRelayers", (Object) newRelayers); + Integer threshold = 3; + aggregator.invoke(adminAc, "setRelayers", (Object) newRelayers, threshold); Address[] updatedRelayers = (Address[]) aggregator.call("getRelayers"); - assertTrue(updatedRelayers[updatedRelayers.length - 1].equals(relayerFiveAc.getAddress())); - assertTrue(updatedRelayers[updatedRelayers.length - 2].equals(relayerFourAc.getAddress())); + Address[] expectedRelayers = new Address[] { adminAc.getAddress(), relayerThreeAc.getAddress(), + relayerFourAc.getAddress(), + relayerFiveAc.getAddress() }; + + HashSet
updatedRelayersSet = new HashSet
(); + for (Address rlr : updatedRelayers) { + updatedRelayersSet.add(rlr); + } + + HashSet
expectedRelayersSet = new HashSet
(); + for (Address rlr : expectedRelayers) { + expectedRelayersSet.add(rlr); + } + + assertEquals(expectedRelayersSet, updatedRelayersSet); + + Integer result = (Integer) aggregator.call("getSignatureThreshold"); + assertEquals(threshold, result); } @Test - public void testAddRelayers_unauthorised() { + public void testSetRelayers_unauthorized() { Account relayerFiveAc = sm.createAccount(); - Address[] newRelayers = new Address[] { relayerFourAc.getAddress(), relayerFiveAc.getAddress() }; + Address[] newRelayers = new Address[] { relayerThreeAc.getAddress(), relayerFourAc.getAddress(), + relayerFiveAc.getAddress() }; - Executable action = () -> aggregator.invoke(relayerOneAc, "addRelayers", (Object) newRelayers); + Integer threshold = 3; + Executable action = () -> aggregator.invoke(relayerOneAc, "setRelayers", + (Object) newRelayers, threshold); UserRevertedException e = assertThrows(UserRevertedException.class, action); - assertEquals("Reverted(0): Unauthorized: caller is not the leader relayer", e.getMessage()); + assertEquals("Reverted(0): Unauthorized: caller is not the leader relayer", + e.getMessage()); + } @Test - public void testRemoveRelayers() { - Address[] relayerToBeRemoved = new Address[] { relayerOneAc.getAddress(), - relayerTwoAc.getAddress() }; - - aggregator.invoke(adminAc, "removeRelayers", (Object) relayerToBeRemoved); + public void testSetRelayers_invalidThreshold() { + Account relayerFiveAc = sm.createAccount(); + Address[] newRelayers = new Address[] { relayerThreeAc.getAddress(), relayerFourAc.getAddress(), + relayerFiveAc.getAddress() }; - Address[] updatedRelayers = (Address[]) aggregator.call("getRelayers"); + Integer threshold = 5; + Executable action = () -> aggregator.invoke(adminAc, "setRelayers", + (Object) newRelayers, threshold); + UserRevertedException e = assertThrows(UserRevertedException.class, action); - Boolean removed = true; - for (Address rlr : updatedRelayers) { - if (rlr.equals(relayerOneAc.getAddress()) || rlr.equals(relayerTwoAc.getAddress())) { - removed = false; - break; - } - } + assertEquals("Reverted(0): threshold value should be at least 1 and not greater than relayers size", + e.getMessage()); - assertTrue(removed); - assertEquals(updatedRelayers[1], relayerThreeAc.getAddress()); } @Test - public void testRemoveRelayers_unauthorised() { - Address[] relayerToBeRemoved = new Address[] { relayerOneAc.getAddress(), - relayerTwoAc.getAddress() }; - - aggregator.invoke(adminAc, "removeRelayers", (Object) relayerToBeRemoved); + public void testSetRelayers_invalidThresholdZero() { + Account relayerFiveAc = sm.createAccount(); + Address[] newRelayers = new Address[] { relayerThreeAc.getAddress(), relayerFourAc.getAddress(), + relayerFiveAc.getAddress() }; - Executable action = () -> aggregator.invoke(relayerFourAc, "removeRelayers", (Object) relayerToBeRemoved); + Integer threshold = 0; + Executable action = () -> aggregator.invoke(adminAc, "setRelayers", + (Object) newRelayers, threshold); UserRevertedException e = assertThrows(UserRevertedException.class, action); - assertEquals("Reverted(0): Unauthorized: caller is not the leader relayer", e.getMessage()); + assertEquals("Reverted(0): threshold value should be at least 1 and not greater than relayers size", + e.getMessage()); + } @Test @@ -197,11 +221,13 @@ public void testPacketSubmitted_true() throws Exception { byte[] dataHash = Context.hash("sha-256", data); byte[] sign = relayerOne.sign(dataHash); - aggregator.invoke(relayerOneAc, "submitPacket", srcNetwork, srcContractAddress, srcSn, srcHeight, dstNetwork, + aggregator.invoke(relayerOneAc, "submitPacket", srcNetwork, + srcContractAddress, srcSn, srcHeight, dstNetwork, dstContractAddress, data, sign); - boolean submitted = (boolean) aggregator.call("packetSubmitted", relayerOneAc.getAddress(), srcNetwork, + boolean submitted = (boolean) aggregator.call("packetSubmitted", + relayerOneAc.getAddress(), srcNetwork, srcContractAddress, srcSn); assertEquals(submitted, true); } @@ -212,7 +238,8 @@ public void testPacketSubmitted_false() throws Exception { BigInteger srcSn = BigInteger.ONE; String srcContractAddress = "hxjuiod"; - boolean submitted = (boolean) aggregator.call("packetSubmitted", relayerOneAc.getAddress(), srcNetwork, + boolean submitted = (boolean) aggregator.call("packetSubmitted", + relayerOneAc.getAddress(), srcNetwork, srcContractAddress, srcSn); assertEquals(submitted, false); } @@ -232,12 +259,14 @@ public void testSubmitPacket() throws Exception { byte[] dataHash = Context.hash("sha-256", data); byte[] sign = relayerOne.sign(dataHash); - aggregator.invoke(relayerOneAc, "submitPacket", srcNetwork, srcContractAddress, srcSn, srcHeight, dstNetwork, + aggregator.invoke(relayerOneAc, "submitPacket", srcNetwork, + srcContractAddress, srcSn, srcHeight, dstNetwork, dstContractAddress, data, sign); String pktID = Packet.createId(srcNetwork, srcContractAddress, srcSn); - verify(aggregatorSpy).PacketRegistered(srcNetwork, srcContractAddress, srcSn, srcHeight, dstNetwork, + verify(aggregatorSpy).PacketRegistered(srcNetwork, srcContractAddress, srcSn, + srcHeight, dstNetwork, dstContractAddress, data); verify(aggregatorSpy).setSignature(pktID, relayerOneAc.getAddress(), sign); } @@ -257,12 +286,14 @@ public void testSubmitPacket_thresholdReached() throws Exception { byte[] dataHash = Context.hash("sha-256", data); byte[] signAdmin = admin.sign(dataHash); - aggregator.invoke(adminAc, "submitPacket", srcNetwork, srcContractAddress, srcSn, srcHeight, dstNetwork, + aggregator.invoke(adminAc, "submitPacket", srcNetwork, srcContractAddress, + srcSn, srcHeight, dstNetwork, dstContractAddress, data, signAdmin); byte[] signOne = relayerOne.sign(dataHash); - aggregator.invoke(relayerOneAc, "submitPacket", srcNetwork, srcContractAddress, srcSn, srcHeight, dstNetwork, + aggregator.invoke(relayerOneAc, "submitPacket", srcNetwork, + srcContractAddress, srcSn, srcHeight, dstNetwork, dstContractAddress, data, signOne); @@ -277,7 +308,8 @@ public void testSubmitPacket_thresholdReached() throws Exception { assertArrayEquals(signAdmin, decodedSigs[0]); assertArrayEquals(signOne, decodedSigs[1]); - verify(aggregatorSpy).PacketAcknowledged(srcNetwork, srcContractAddress, srcSn, srcHeight, dstNetwork, + verify(aggregatorSpy).PacketAcknowledged(srcNetwork, srcContractAddress, + srcSn, srcHeight, dstNetwork, dstContractAddress, data, encodedSigs); } @@ -295,7 +327,8 @@ public void testSubmitPacket_unauthorized() throws Exception { byte[] dataHash = Context.hash("sha-256", data); byte[] sign = relayerFour.sign(dataHash); - Executable action = () -> aggregator.invoke(relayerFourAc, "submitPacket", srcNetwork, srcContractAddress, + Executable action = () -> aggregator.invoke(relayerFourAc, "submitPacket", + srcNetwork, srcContractAddress, srcSn, srcHeight, dstNetwork, dstContractAddress, data, sign); @@ -319,10 +352,12 @@ public void testSubmitPacket_duplicate() throws Exception { byte[] dataHash = Context.hash("sha-256", data); byte[] sign = relayerOne.sign(dataHash); - aggregator.invoke(relayerOneAc, "submitPacket", srcNetwork, srcContractAddress, srcSn, srcHeight, dstNetwork, + aggregator.invoke(relayerOneAc, "submitPacket", srcNetwork, + srcContractAddress, srcSn, srcHeight, dstNetwork, dstContractAddress, data, sign); - Executable action = () -> aggregator.invoke(relayerOneAc, "submitPacket", srcNetwork, srcContractAddress, srcSn, + Executable action = () -> aggregator.invoke(relayerOneAc, "submitPacket", + srcNetwork, srcContractAddress, srcSn, srcHeight, dstNetwork, dstContractAddress, data, sign); ;