Skip to content

Commit

Permalink
remove old & not relevant todos
Browse files Browse the repository at this point in the history
  • Loading branch information
fedejinich committed Jul 4, 2022
1 parent c4dc6d7 commit ec02192
Show file tree
Hide file tree
Showing 10 changed files with 24 additions and 40 deletions.
2 changes: 1 addition & 1 deletion rskj-core/src/main/java/co/rsk/storagerent/RentedNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public long rollbackFee(long executionBlockTimestamp) {
rentDue(getNodeSize(), duration(executionBlockTimestamp)),
rentCap(),
0); // there are no thresholds for rollbacks, we want to make the user to pay something
return (long) (computedRent * 0.25); // todo(fedejinich) avoid casting?
return (long) (computedRent * 0.25);
}

@Override
Expand Down
1 change: 0 additions & 1 deletion rskj-core/src/main/java/co/rsk/trie/Trie.java
Original file line number Diff line number Diff line change
Expand Up @@ -928,7 +928,6 @@ private Trie internalPut(TrieKeySlice key, byte[] value, boolean isRecursiveDele
// reference equality
if (newNode == node) {
// ...but then it's replaced with the new created leaf node. 2/2
// todo(fedejinich) Is it just used to create the trie root?
return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ public synchronized void addStorageBytes(RskAddress addr, DataWord key, byte[] v
// conversion here only applies if this is called directly. If suppose this only occurs in tests, but it can
// also occur in precompiled contracts that store data directly using this method.
if (value == null || value.length == 0) {
internalPut(triekey, null); // todo(fedejinich) why put(key, null) and not deleteRecursive(key) ?
internalPut(triekey, null);
} else {
internalPut(triekey, value);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ protected byte[] internalGet(byte[] key, boolean readsContractCode) {
if(readsContractCode) {
trackNodeReadContractOperation(key, isSuccessful);
} else {
// todo(fedejinich) should track get() success with a bool (value != null)
trackNodeReadOperation(key, isSuccessful);
}

Expand Down Expand Up @@ -208,7 +207,6 @@ protected Uint24 internalGetValueLength(byte[] key) {
protected Iterator<DataWord> internalGetStorageKeys(RskAddress addr) {
Iterator<DataWord> storageKeys = super.internalGetStorageKeys(addr);

// todo(fedejinich) how should I track the right key/s?
boolean result = !storageKeys.equals(Collections.emptyIterator());
byte[] accountStoragePrefixKey = trieKeyMapper.getAccountStoragePrefixKey(addr);

Expand All @@ -217,8 +215,6 @@ protected Iterator<DataWord> internalGetStorageKeys(RskAddress addr) {
return storageKeys;
}

// todo(fedejinich) remove all this unecesary "trackNode..." methods, there should only be one method

protected void trackNodeWriteOperation(byte[] key) {
trackNode(key, WRITE_OPERATION, true);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1329,7 +1329,6 @@ public void callToPrecompiledAddress(MessageCall msg, PrecompiledContract contra
return;
}

// todo(fedejinich) should i add the storage rent manager here also?
Repository track = getStorage().startTracking();

RskAddress senderAddress = getOwnerRskAddress();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

import static org.ethereum.db.OperationType.*;

// todo(fedejinich) this seems unnecessary, should get rid of it
public class MutableRepositoryTestable extends MutableRepositoryTracked {
public MutableRepositoryTestable(MutableTrie mutableTrie,
MutableRepositoryTracked parentRepository, boolean enableTracking) {
Expand Down
47 changes: 20 additions & 27 deletions rskj-core/src/test/java/co/rsk/db/MutableRepositoryTrackedTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public void internalGet_getCode_shouldTriggerNodeTrackingIfValueItsPresent() {

// a nonexistent account
spyRepository.getCode(randomAccountAddress());
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -51,16 +51,15 @@ public void internalGet_getCode_shouldTriggerNodeTrackingIfValueItsPresent() {
spyRepository.saveCode(accAddress1, "someCode".getBytes(StandardCharsets.UTF_8));

spyRepository.getCode(accAddress1);
// todo(fedejinich) explain invokes
// verifyNodeTracking(4, 3, 0);

verifyNodeTracking(3, 3, 1);
}

@Test
public void internalGet_isContract_shouldTriggerNodeTrackingIfValueItsPresent() {
// a nonexistent account
spyRepository.isContract(randomAccountAddress());
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -72,7 +71,7 @@ public void internalGet_isContract_shouldTriggerNodeTrackingIfValueItsPresent()
spyRepository.saveCode(accAddress1, "someCode".getBytes(StandardCharsets.UTF_8));

spyRepository.isContract(accAddress1);
// todo(fedejinich) explain invokes

verifyNodeTracking(2, 3, 0);
}

Expand All @@ -82,14 +81,14 @@ public void internalGet_getStorageBytes_shouldTriggerNodeTrackingIfValueItsPrese

// should track a nonexistent storage key
spyRepository.getStorageBytes(accAddress1, DataWord.ZERO);
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();

// should track a nonexistent account
spyRepository.getStorageBytes(randomAccountAddress(), DataWord.ONE);
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -101,7 +100,7 @@ public void internalGet_getStorageBytes_shouldTriggerNodeTrackingIfValueItsPrese
"something".getBytes(StandardCharsets.UTF_8));

spyRepository.getStorageBytes(accAddress1, DataWord.ONE);
// todo(fedejinich) explain invokes

verifyNodeTracking(3, 4, 0);
}

Expand All @@ -111,14 +110,14 @@ public void internalGet_getStorageValue_shouldTriggerNodeTrackingIfValueItsPrese

// nonexistent storage key
spyRepository.getStorageValue(accAddress1, DataWord.valueOf(2));
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();

// nonexistent account
spyRepository.getStorageValue(randomAccountAddress(), DataWord.ONE);
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -132,15 +131,14 @@ public void internalGet_getStorageValue_shouldTriggerNodeTrackingIfValueItsPrese

spyRepository.getStorageValue(accAddress1, DataWord.ONE);

// todo(fedejinich) explain invokes
verifyNodeTracking(3, 4, 0);
}

@Test
public void internalGet_getAccountState_shouldTriggerNodeTrackingIfValueItsPresent() {
// should track a nonexistent account state
spyRepository.getAccountState(randomAccountAddress());
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -154,7 +152,7 @@ public void internalGet_getAccountState_shouldTriggerNodeTrackingIfValueItsPrese
"something".getBytes(StandardCharsets.UTF_8));

spyRepository.getAccountState(accAddress1);
// todo(fedejinich) explain invokes

verifyNodeTracking(3, 4, 0);
}

Expand All @@ -163,14 +161,14 @@ public void internalGet_getAccountState_shouldTriggerNodeTrackingIfValueItsPrese
@Test
public void internalPut_setupContract_shouldTrackNodesIfValueItsPresent() {
spyRepository.setupContract(randomAccountAddress());
// todo(fedejinich) explain invokes

verifyNodeTracking(0, 1, 0);
}

@Test
public void internalPut_saveCode_shouldTrackNodesIfValueItsPresent() {
spyRepository.saveCode(randomAccountAddress(), "something".getBytes(StandardCharsets.UTF_8));
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 2, 0);
}

Expand All @@ -181,7 +179,7 @@ public void internalPut_addStorageBytes_shouldTrackNodesIfValueItsPresent() {
spyRepository.createAccount(accAddress1);

spyRepository.addStorageBytes(accAddress1, DataWord.ONE, "something".getBytes(StandardCharsets.UTF_8));
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 2, 0);
}

Expand All @@ -193,7 +191,6 @@ public void internalPut_updateAccountState_shouldTrackNodesIfValueItsPresent() {

spyRepository.updateAccountState(accAddress1, new AccountState());

// todo(fedejinich) explain invokes
verifyNodeTracking(0, 2, 0);
}

Expand All @@ -203,7 +200,7 @@ public void internalPut_updateAccountState_shouldTrackNodesIfValueItsPresent() {
public void internalGetValueHash_getCodeHashStandard_shouldTrackNodesIfValueItsPresent() {
// track a nonexistent account state
spyRepository.getCodeHashStandard(randomAccountAddress());
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -214,7 +211,6 @@ public void internalGetValueHash_getCodeHashStandard_shouldTrackNodesIfValueItsP

spyRepository.getCodeHashStandard(accAddress1);

// todo(fedejinich) explain invokes
verifyNodeTracking(2, 1, 0);
}

Expand All @@ -235,7 +231,6 @@ public void internalGetValueHash_getCodeHashNonStandard_shouldTrackNodesIfValueI

spyRepository.getCodeHashNonStandard(accAddress1);

// todo(fedejinich) explain invokes
verifyNodeTracking(2, 1, 0);
}

Expand All @@ -251,7 +246,7 @@ private void verifyNodeTracking(int invokedReads, int invokedWrites, int invoked
public void internalGetValueLength_isExist_shouldTrackNodesIfValueItsPresent() {
// should track a nonexistent account state
spyRepository.isExist(randomAccountAddress());
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -263,15 +258,14 @@ public void internalGetValueLength_isExist_shouldTrackNodesIfValueItsPresent() {
spyRepository.isExist(accAddress1);

// one at createAccount(), the other one at isExist()
// todo(fedejinich) explain invokes
verifyNodeTracking(1, 1, 0);
}

@Test
public void internalGetValueLength_getCodeLength_shouldTrackNodesIfValueItsPresent() {
// should track a nonexistent account state
spyRepository.getCodeLength(randomAccountAddress());
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -283,7 +277,7 @@ public void internalGetValueLength_getCodeLength_shouldTrackNodesIfValueItsPrese
spyRepository.saveCode(accAddress1, "something".getBytes(StandardCharsets.UTF_8));

spyRepository.getCodeLength(accAddress1);
// todo(fedejinich) explain invokes

verifyNodeTracking(3, 3, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -294,7 +288,6 @@ public void internalGetValueLength_getCodeLength_shouldTrackNodesIfValueItsPrese
spyRepository.getCodeLength(accAddress2);

// because it's an address that doesn't have any code
// todo(fedejinich) explain invokes
verifyNodeTracking(2, 1, 0);
}

Expand All @@ -304,7 +297,7 @@ public void internalGetValueLength_getCodeLength_shouldTrackNodesIfValueItsPrese
public void internalGetStorageKeys_getStorageKeys_shouldTrackNodesIfValueItsPresent() {
// should track on nonexistent account state
spyRepository.getStorageKeys(randomAccountAddress());
// todo(fedejinich) explain invokes

verifyNodeTracking(1, 0, 0);

spyRepository = newMutableRepositoryTestable();
Expand All @@ -316,7 +309,7 @@ public void internalGetStorageKeys_getStorageKeys_shouldTrackNodesIfValueItsPres
spyRepository.saveCode(accAddress1, "something".getBytes(StandardCharsets.UTF_8));

spyRepository.getStorageKeys(accAddress1);
// todo(fedejinich) explain invokes

verifyNodeTracking(2, 3, 0);
}

Expand Down
2 changes: 1 addition & 1 deletion rskj-core/src/test/java/co/rsk/mine/MinerServerTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public void buildBlockToMineCheckThatLastTransactionIsForREMASC() {
when(tx1.getHash()).thenReturn(new Keccak256(s1));
when(tx1.getEncoded()).thenReturn(new byte[32]);

Repository repository = repositoryLocator.startTrackingAt(blockStore.getBestBlock().getHeader()); // todo(fedejinich) this should be MutableRepositoryTracked right?
Repository repository = repositoryLocator.startTrackingAt(blockStore.getBestBlock().getHeader());
MutableRepositoryTracked track = mock(MutableRepositoryTracked.class);
BlockTxSignatureCache blockTxSignatureCache = mock(BlockTxSignatureCache.class);
Mockito.doReturn(repository.getRoot()).when(track).getRoot();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public void tokenTransfer() throws FileNotFoundException, DslProcessorException
checkStorageRent(world,"tx02", 0, 0, 4, 0);
checkStorageRent(world,"tx03", 0, 0, 3, 0);

// transfer(senderAccountState, contractCode, balance1, balance2, storageRoot, ...) todo(fedejinich) i think i'm missing one node, ask shree
// transfer(senderAccountState, contractCode, balance1, balance2, storageRoot, ...)
checkStorageRent(world,"tx04", 15001, 0, 5, 0);

// balanceOf
Expand All @@ -102,7 +102,6 @@ public void internalTransactionFailsButOverallEndsOk() throws FileNotFoundExcept

// rollbackRent should be >0, we want to "penalize" failed access
checkStorageRent(world, "tx04", 2781, 280, 8, 10);
// todo(fedejinich) check that contract C doesn't commits state changes and doesn't updates its timestamp
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ public class BlockResultDTOTest {
private BlockStore blockStore;
public static final Transaction TRANSACTION = new TransactionBuilder().buildRandomTransaction();

// todo(fedejinich) currently RemascTx(blockNumber) has a bug, thats why I initialize this way
public static final RemascTransaction REMASC_TRANSACTION = new RemascTransaction(new RemascTransaction(1).getEncoded());

@Before
Expand Down

0 comments on commit ec02192

Please sign in to comment.