-
Notifications
You must be signed in to change notification settings - Fork 20
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
Extracting an API for the Virtual Machine #48
Conversation
- All changes here reflect the fact that the kernel Address has been renamed.
- Bringing in the new changes to the base module. This includes now having AionTransaction in aion_api implement TransactionInterface. - So far this is co-existing with ITransaction and there are duplicate methods. A real cleanup needs to be done on this project.
- Some methods were duplicated (with differing names slightly) due to the introduction of the TransactionInterface type. This commit simply cleans this up to remove these duplications where possible.
- This is required so fastVM can implement VirtualMachine.
- The API has made some naming changes, those changes are reflected here.
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.
Hey, looks pretty sane so far.
Question: it looks like org.aion.base.type.Address
has become two classes/interfaces: org.aion.vm.api.interfaces.Address
and org.aion.base.type.AionAddress
.
Can you explain a bit what the difference is and when each should be used? I'm guessing one is an interface and the other an impl, but their code isn't in this PR so just want to check...
Also have one small comment in-line below.
|
||
// NOTE: a number of methods are now duplicated here (but with different names) because of issue | ||
// #734. | ||
// This should be eventually cleaned up, but is not currently a priority. |
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.
Might want to create an issue or jira to track and link it here
@aion-kelvin these are the current implementations: |
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.
LGTM
Spoke with Jeff and Nick a bit about AionAddress/Address. Conclusion was probably they don't need to be separated into interface and class, but we can fix that later. |
will be reopened in stages |
Issue: aionnetwork/aion#734
PR: aionnetwork/aion#761