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

A proposal for chain-id's, to help avoid using the wrong network #49

Merged
merged 18 commits into from
Aug 4, 2022

Conversation

cbeck88
Copy link
Contributor

@cbeck88 cbeck88 commented Jul 25, 2022

text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
Copy link

@jgreat jgreat left a comment

Choose a reason for hiding this comment

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

I think making the networks aware of who they are in a human readable way is a good way to reduces client and operational support burden.

Clear errors like "Service network_id doesn't match expected network_id" would be really helpful.

@wjuan-mob wjuan-mob self-requested a review July 25, 2022 22:00
text/0049-network-ids.md Outdated Show resolved Hide resolved
Copy link

@voloshyn voloshyn left a comment

Choose a reason for hiding this comment

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

I like the idea:

  1. the overhead on the SDKs will be minimal;
  2. we don't have to come up with and maintain separate data testssets for separate networks;
  3. does not require token range reservation, so if someone wants to mint a token in hexspeak like 0xABADCAFEDEADBEEF they can;
    If network-id is a header field, adding it to Fog seems simple enough too, I think it makes sense to add it from the get-go.

text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
text/0049-network-ids.md Outdated Show resolved Hide resolved
Chris Beck and others added 6 commits July 26, 2022 18:01
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Chris Beck and others added 3 commits July 26, 2022 19:57
Co-authored-by: Nick Santana <nick@mobilecoin.com>
Co-authored-by: Nick Santana <nick@mobilecoin.com>
@eranrund
Copy link
Contributor

This seems reasonable to me. I do think the network id should make it into the enclave. We don't have to worry about releasing a new enclave immediately but there's no reason to not include this in a future release.
I also think we should add it to fog - I haven't seen a convincing argument why only part of the network services (e.g. consensus) should have this but not others (e.g. fog). It's not a particularly complicated change, and it is backwards-compatible for clients.

Thanks for writing this!

@cbeck88
Copy link
Contributor Author

cbeck88 commented Jul 27, 2022

Yeah I was trying to just control the scope of the change, but in POC work it seems like it's pretty easy to add it to fog also. I will update the proposal.

I think a second step should be that:

  • network id makes it into the enclave
  • network id is part of the transaction and under the signature, and checked by the enclave
  • James proposed (offline) that network id is in the block header. this will give mobilecoind and fog-ingest visibility into it
  • James also thinks we should call it "chain id" and we shouldn't have two separate ids if there's no good reason.

Right now I'm just trying to do the network-id part because it can ship without an enclave upgrade

@eranrund
Copy link
Contributor

I like those suggestions, I think the MCIP should cover the whole desirable change, we can implement at whatever pace we like.

@cbeck88 cbeck88 changed the title A proposal for network-id's, to help avoid using the wrong network A proposal for chain-id's, to help avoid using the wrong network Jul 31, 2022
text/0049-chain-ids.md Outdated Show resolved Hide resolved
text/0049-chain-ids.md Outdated Show resolved Hide resolved
@cbeck88 cbeck88 added the final-comment-period This RFC is in the final-comment period for the next 24h label Aug 4, 2022
@cbeck88 cbeck88 merged commit ff414fb into mobilecoinfoundation:main Aug 4, 2022
cbeck88 pushed a commit that referenced this pull request Aug 4, 2022
cbeck88 pushed a commit that referenced this pull request Aug 4, 2022
cbeck88 pushed a commit that referenced this pull request Aug 4, 2022
@cbeck88
Copy link
Contributor Author

cbeck88 commented Aug 4, 2022

sorry guys, this wasn't supposed to merge yet, we were supposed to wait for final comment period, but i fat-fingered it

Please, let's move any further comments here:

#51

cbeck88 pushed a commit that referenced this pull request Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period This RFC is in the final-comment period for the next 24h
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants