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

Alpha Implementation of Specification #129

Merged
merged 30 commits into from
Apr 10, 2019
Merged

Alpha Implementation of Specification #129

merged 30 commits into from
Apr 10, 2019

Conversation

JakeOShannessy
Copy link
Contributor

This PR contains pretty much all of the spec implementation with the sole exception of secure cap delegation. Currently there is an exception made where any capability can be registered as long as you have a register capability.

Once that's implemented this can be considered complete. All tests have been updated to pass.

This updates the procedure list and procedure heap to use the correct
formats. It does not attempt to address minting and delegation and
simply allows anything to be registered. Minting and delegation will be
addressed in a later PR.
@Latrasis
Copy link
Member

@JakeOShannessy: Please let me know which issues are addressed here and when this is ready for review.

@JakeOShannessy
Copy link
Contributor Author

This addresses: #122, #119, #110. I'll add a few more tests today, then mark it as ready.

@JakeOShannessy
Copy link
Contributor Author

Still subject to #130, but that's an existing condition, so it's mergable.

@JakeOShannessy JakeOShannessy marked this pull request as ready for review April 1, 2019 11:32
@Latrasis
Copy link
Member

Latrasis commented Apr 8, 2019

So at Kernel.sol:704, we're setting that 0 is not a valid procedureId. Just to make sure, are there any other reasons than preventing collisions during a "null" currentProcedure?

@JakeOShannessy
Copy link
Contributor Author

So at Kernel.sol:704, we're setting that 0 is not a valid procedureId. Just to make sure, are there any other reasons than preventing collisions during a "null" currentProcedure?

That was previously a requirement from our original tests. There's no functional reason we need to prevent it.

@JakeOShannessy
Copy link
Contributor Author

I'll just leave a note here, some of those capability functions are quite hairy. The number of local solidity variables you can have runs out fast, so there is quite a bit of re-use of variables. Sorry about that. Welcome to the 1960's.

@JakeOShannessy
Copy link
Contributor Author

Let me know when you're done with comments, I was going to do some review when I saw your comments. So once you've had a look through I'll start fiddling and update re: the comments above.

@Latrasis
Copy link
Member

Latrasis commented Apr 8, 2019

Awesome work! As far as I'm concerned, with upgrading working (PROC_REG + PROC_DELETE), we can move forward with the rest.

This required some error code plumbing to be done. I also removed one
of the checks that returned an error code if the procedure name already
exists. This way we only check that problem once rather than having
unreachable code.
@JakeOShannessy
Copy link
Contributor Author

Ok great. I think first order of business is to add a test for that exact scenario.

Addressed.

This is a quick patch that sets the clists to zero length before
registering. The final choice for how we deal with clist deletion is to
be made in #129.
There use to be a special cap class to monkey patch the fact that the
capIndex value it not passed through correctly. This is now fixed.
@JakeOShannessy
Copy link
Contributor Author

In order to achieve some of the goals I needed to do a slight restructure. The fact that we use a custom method for storage impedes some of the structure that Solidity assumes, so it may not conform to the right principles. The most important thing was that the definition of kernel storage was able to be abstracted into its own layer, that is achieved. Here is a diagram of how everything is currently structured (this is included in docs/media):

Architecture

This doesn't use any libraries, only contract inheritance, we already introduce a lot of CALLs already, so best to avoid them if not necessary. KernelStorage and to some extent IKernel contain all the code for reading and writing to the kernel storage. ProcedureTable contains all the list/heap management logic for procedures and CapabilityManager contains all the logic for checking capabilities. There may be a little bit of overlap between these two at the moment.

This information is of particular importance to #120.

This commit gives TestKernel a constructor which takes an initial entry
procedure. This required test changes throughout. The implementation
will need to be updated to have a better architecture, but this
implements the concepts and updates tests for compatibility.

The next steps will be to move it to a level lower then TestKernel and
abstract some of the initial cap setup. The concept is documented in
in issue #130.
This moves the constructor and bootstrap mechanism to another layer in
KernelInstance.sol, as per #130.
@JakeOShannessy
Copy link
Contributor Author

Added boostrap procedure, then moved that into KernelInstance.sol. Inheritance is now like this:

Architecture

@Latrasis

This comment has been minimized.

@JakeOShannessy
Copy link
Contributor Author

I've moved the procedure table functions as requested, but they do depend on ProcedureTable, so a slight rearrangement was necessary.

Architecture

@JakeOShannessy
Copy link
Contributor Author

The tests pass locally, and if they pass on CI this should be merged.

@Latrasis Latrasis merged commit 0be311e into master Apr 10, 2019
@Latrasis Latrasis mentioned this pull request Apr 10, 2019
7 tasks
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.

2 participants