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

update: User inter-canister calls [SDK-702] #6

Merged
24 commits merged into from
Feb 12, 2020
Merged

Conversation

ghost
Copy link

@ghost ghost commented Jan 15, 2020

Overview

Still a work in progress. Want to show where my head's at.

This PR adds inter-canister calling to the LinkedUp app, and a whole lot of other stuff (basically everything necessary to get the demo working for Davos).

Note: This app will get another round of cleanup before being moved into the Examples repo.

Changes

  • Inter-canister messaging
  • Remove extra canisters
  • Move dependencies to imports
  • Hide implementation in utility modules
  • So much more!

Copy link

@matthewhammer matthewhammer left a comment

Choose a reason for hiding this comment

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

Looking really good thus far.

I noticed that you reverted back to hashMap's keys as blob-hashes, rather than the blobs themselves. I assume that's related to some other blocking issue, and not because you decided against that design (the code is still there, but commented).

My comments within the code are about using Option.unwrap, and about some thoughts about making things fancier that come up during the demos. Nothing above is urgent.

Copy link

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Some drive-by style comments :)


let adminIds : [PrincipalId] = [];

func getUserId (caller : Blob) : PrincipalId { Blob.hash(caller) };
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a gaping security hole: The Blob hash is not a cryptographic one, and (especially with user-picked identities) easy to create collisions for.

I think you simply want the PrincipalId to be the blob (or the new abstract PrincipalId created in dfinity/motoko#1128)

@hansl
Copy link
Contributor

hansl commented Feb 1, 2020

I'll keep this branch up to date with the latest DFX from source.

Is this something we want in Master @stanleygjones ?

@ghost
Copy link
Author

ghost commented Feb 3, 2020

Oh sorry, I didn't realize people were commenting on this PR. I hadn't meant to ignore all of your comments. I'll try to incorporate them and get this merged.

@ghost
Copy link
Author

ghost commented Feb 10, 2020

Finally got checks passing so let's review this puppy.

@ghost ghost marked this pull request as ready for review February 10, 2020 17:16
@ghost ghost self-requested a review as a code owner February 10, 2020 17:16
@ghost
Copy link
Author

ghost commented Feb 10, 2020

Finally got checks passing so let's review this puppy.

@ghost ghost mentioned this pull request Feb 10, 2020
@ghost ghost requested review from enzoh, hansl and matthewhammer February 10, 2020 17:30
@ghost ghost merged commit 2d8c347 into master Feb 12, 2020
@ghost ghost deleted the sj/inter-canister-calls branch February 12, 2020 18:14
This pull request was closed.
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.

5 participants