Skip to content
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

Feature/eip 1283 #1173

Merged
merged 7 commits into from
Sep 10, 2018
Merged

Feature/eip 1283 #1173

merged 7 commits into from
Sep 10, 2018

Conversation

zilm13
Copy link
Collaborator

@zilm13 zilm13 commented Sep 6, 2018

Have not found any github tests for this, only unit provided

@zilm13 zilm13 requested a review from mkalinin September 6, 2018 21:11
@zilm13 zilm13 mentioned this pull request Sep 6, 2018
5 tasks
@@ -136,6 +137,7 @@ public Program(byte[] codeHash, byte[] ops, ProgramInvoke programInvoke, Transac
traceListener = new ProgramTraceListener(config.vmTrace());
this.memory = setupProgramListener(new Memory());
this.stack = setupProgramListener(new Stack());
this.originalRepo = programInvoke.getRepository().getSnapshotTo(programInvoke.getRepository().getRoot());
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooops, we could have not only RepositoryImpl here at least in tests.
Any ideas how can we get copy of Repo here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you, please, be more specific with the problem?

@@ -789,10 +791,29 @@ private int getReturnDataBufferSizeI() {
Arrays.copyOfRange(returnDataBuffer, off.intValueSafe(), off.intValueSafe() + size.intValueSafe());
}

@Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd not deprecate storageLoad cause SLOAD op should keep using it. getCurrentValue and storageLoad are different operations in terms of semantics, I'd decouple them from each other.

}

/*
* @return Storage data at the beginning of Program execution
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's extend description with a reference to the EIP-1283

@coveralls
Copy link

coveralls commented Sep 9, 2018

Coverage Status

Coverage increased (+0.2%) to 56.473% when pulling 53098a6 on feature/eip-1283 into 5c808c1 on develop.

*/
@Override
public Repository clone() {
return startTracking();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, RepositoryImpl itself is used for tracks, thus, it makes sense to return parent.startTracking() here which will create a clone.

@@ -235,4 +235,6 @@ void loadAccount(byte[] addr, HashMap<ByteArrayWrapper, AccountState> cacheAccou
HashMap<ByteArrayWrapper, ContractDetails> cacheDetails);

Repository getSnapshotTo(byte[] root);

Repository clone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a javadoc to highlight semantics of this call.

@mkalinin mkalinin merged commit 6b87540 into develop Sep 10, 2018
@zilm13 zilm13 deleted the feature/eip-1283 branch September 20, 2018 09:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants