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

[dip-196] Initial onchain network identity DIP draft #200

Merged
merged 1 commit into from
Aug 26, 2021

Conversation

gregnazario
Copy link
Contributor

Summary

DIP for Onchain Network Identity

@gregnazario
Copy link
Contributor Author

@aching @davidiw Could I get some feedback on this DIP? I followed DIP 0 as the instructions, but the process has changed. I can update the instructions if we need to

Copy link
Contributor

@davidiw davidiw left a comment

Choose a reason for hiding this comment

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

Coming along well

# Implementations

N/A

Copy link
Contributor

Choose a reason for hiding this comment

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

recommend an Examples section of how the Diem tech stack would leverage this -- e.g., event emitting, pick up identity, update networking stack... mention other enhancements that you built out to help mitigate attacks (TBF and connection limits)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't specified in DIP-0. We should update the process even if it's just suggested.

dips/dip-196.md Outdated
Comment on lines 50 to 54
## Consuming Network Identity

Consumers of `NetworkIdentity` may keep track of a list of accounts to watch. The consumers then can watch the event streams of each account's `NetworkIdentity` for updates and update accordingly.

A consumer may choose to subscribe to all accounts, in which it will watch the `Treasury compliance` account updates and keep track of which accounts to watch accordingly.
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 provide the code snippet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently reworking some of the technical details involving integration with move. Can come up with examples and code snippets once that's done.

@gregnazario
Copy link
Contributor Author

@aching Do you have any other comments for this DIP?

Copy link
Contributor

@aching aching left a comment

Choose a reason for hiding this comment

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

Nice work!

@aching
Copy link
Contributor

aching commented Aug 24, 2021

Have you had a chance to get to the feedback? I think once addressed, we can commit the draft and make more progress here.

@gregnazario
Copy link
Contributor Author

Have you had a chance to get to the feedback? I think once addressed, we can commit the draft and make more progress here.

I've addressed all the feedback but examples / code snippets, as I'm still waiting on changes in the move language to support vec<vec>

Copy link
Contributor

@aching aching left a comment

Choose a reason for hiding this comment

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

Looks good for a draft! Hopefully others will review it as well for the next phase.

@aching aching merged commit 1118d6c into diem:main Aug 26, 2021
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