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

[VM] Address logic and code restructure #21

Merged
merged 33 commits into from
Nov 19, 2019
Merged

Conversation

austinabell
Copy link
Contributor

  • Implements Address Type #15 but excludes cbor and json marshalling and unmarshalling because a consistent process should be done in another issue instead of being grouped with this one
  • Supresses or fixes linting issues (warnings were really starting to annoy me, sorry I didn't do in another PR)
  • Used existing test vectors from go implementations

Basically what the encoding and decoding does is be able to ambiguously decode addresses for the different addressing protocols (ID, Actor, Secp, BLS) and be able to encode them back to a consistent string. Other functionality includes encoding and decoding from bytes and verifying a checksum for a given address. Some random tests can be added for addresses, but I didn't think it was necessary.

Copy link
Member

@ansermino ansermino left a comment

Choose a reason for hiding this comment

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

Awesome work!

Generally I feel like this should not "support" an undefined protocol/address. Looking at go-filecoin it seems they mostly return errors for these things, although they do sometimes return the undefined address as well. From a design perspective I think their implementation choice is kind of shitty and should be avoided here.

vm/actor/src/address/mod.rs Outdated Show resolved Hide resolved
vm/actor/src/address/mod.rs Outdated Show resolved Hide resolved
vm/actor/src/address/mod.rs Outdated Show resolved Hide resolved
vm/actor/src/address/mod.rs Outdated Show resolved Hide resolved
@austinabell
Copy link
Contributor Author

The goal was to keep logic and capabilities the same as the other implementations because of the lack of a defined spec on the addressing but thinking about it more it probably would be better to just wrap the address in an Option anytime it is used in other modules to achieve basically the same thing

@ansermino ansermino self-requested a review November 18, 2019 21:55
vm/src/types/mod.rs Outdated Show resolved Hide resolved
vm/src/types/address/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@GregTheGreek GregTheGreek left a comment

Choose a reason for hiding this comment

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

New commits LGTM

@austinabell austinabell requested a review from ec2 November 19, 2019 17:01
@austinabell austinabell changed the title [VM] Address type, encoding and decoding [VM] Address logic and code restructure Nov 19, 2019
@austinabell austinabell merged commit e79cc5f into master Nov 19, 2019
@austinabell austinabell deleted the austin/vmaddressing branch November 19, 2019 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants