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

Restore Alice and Bob accounts #195

Closed
wants to merge 11 commits into from
Closed

Restore Alice and Bob accounts #195

wants to merge 11 commits into from

Conversation

JoshOrndorff
Copy link
Contributor

What does it do?

This PR changes the Rust-based genesis configurations to use the standard Alice and Bob accounts for sudo and prefunded accounts. This make Moonbeam more similar to other Substrate based chains. This does not remove the old "Gerald" (0x6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b) account from being prefunded in order to keep intergations that depend on it working.

It also fixes the --dev, --alice, --bob, etc flags to work with the author inherent.

Is there something left for follow-up PRs?

What alternative implementations were considered?

The part about making the --alice flags work could be accomplished more elegantly code-wise by patching substrate. But because maintaining a Substrate fork brings a significant overhead and because the author inherent will change when we start signing blocks, I decided to do a slightly less elegant fix here in Moonbeam.

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

Apps does not currently derive the Alice Bob, etc accounts correctly. polkadot-js/apps#4467 This may make it appear that this PR is incorrect, but really the problem is with Apps.

For example the default chainspec now pre-funds the Alice account. If you go to Apps and add the Alice account by mnemonic, it will appear that Alice was not prefunded. In reality Apps is not deriving the correct account, and the incorrect account that Apps derived is not prefunded. To work around this, import accounts into Apps by private key as described in that issue.

What value does it bring to the blockchain users?

Ability to quickly spin up testnets using standard accounts.

Checklist

  • Does it require a purge of the network?
  • You bumped the runtime version if there are breaking changes in the runtime ?
  • Does it require changes in documentation/tutorials ?

@JoshOrndorff JoshOrndorff changed the title Joshy alice bob Restore Alice and Bob accounts Jan 21, 2021
@jacogr
Copy link

jacogr commented Jan 21, 2021

The issue is not with apps - as per the screenshots posted by me on a standard chain and the tests it derives edcsa aligned with Substrate, unlike the comment above. The test keyring adds sr25519 development accounts, as per the Substrate default. Any chain-specific overrides needs to be manually added.

@JoshOrndorff JoshOrndorff added A0-pleasereview Pull request needs code review. B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes C3-medium Elevates a release containing this PR to "medium priority". labels Jan 22, 2021
@joelamouche
Copy link
Contributor

We should also add some js tests. Let me know if you want me to do that

@JoshOrndorff
Copy link
Contributor Author

@joelamouche I'm always a fan of more tests. I would love it if you could write the ones you have in mind in a followup PR. I'd like to get this in soon as it fixes --dev mode which @albertov19 uses in tutorials.

@JoshOrndorff JoshOrndorff added the A8-mergeoncegreen Pull request is reviewed well. label Jan 26, 2021
AccountId::from_str("6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b").unwrap(),
vec![AccountId::from_str("6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b").unwrap()],
// Sudo Account
get_account_id_from_seed::<ecdsa::Public>("Alice"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are using the Substrate ECDSA here which doesn't derive the seed to a Ethereum public address but to a Substrate public address. This won't be compatible with Ethereum/Moonbeam tools.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how to proceed. I can import these accounts into Apps via private key and use them. But I agree Apps cannot derive them from mnemonic. What should we be doing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

One solution is to skip the derivation part:
We derive Alice, Bob using substrate and store those PK/Pub in the code.
We store those PK/Pub directly in polkadotJs if that is possible (@joelamouche).

I don't have a better idea so far to achieve the Alice/Bob...

Copy link
Contributor

Choose a reason for hiding this comment

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

The way I see it there are two possible solutions:

  • We want Alice and Bob to derive from Ethereum derivation and we take the addresses simply derived from "bottom drive obey lake curtain smoke basket hold race lonely fit walk" in a canonic way, like in ganache-cli. Alice the address 0, Bob address 1, etc. We use the private keys generated bellow to derive Alice and Bob in rust and in the pokkadot-js suite.

Screenshot 2021-01-28 at 10 42 27

  • We don't mind using non-ethereum derivation and we keep non-ethereum derivation in rust and polkadot-js to derive Alice and Bob, mapping the substrate address to an ethereum address and using the key pairs everywhere else, as suggested in @crystalin 's previous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm leaning toward the second option, or just staying away from alice and bob altogether. I see two real cons to doing ethereum-style derivation:

  1. We would have to figure out how to do it in Rust (maybe could copy from open ethereum?)
  2. We would need to figure out how to do it in Polkadot js to Jaco's liking.

Copy link
Contributor

Choose a reason for hiding this comment

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

We would have to figure out how to do it in Rust

We would be using the private keys generated elsewhere from the mnemonic

We would need to figure out how to do it in Polkadot js to Jaco's liking

Yes that is indeed the biggest unknown... But basically either he won't let us change the dev addresses for Moonbeam or he will and those could be reused for any ethereum based parachain

@crystalin
Copy link
Collaborator

crystalin commented Jan 28, 2021 via email

@JoshOrndorff
Copy link
Contributor Author

I'm going to close this. #204 is getting close and it will also fix the --dev mode. And we can re-evaluate what to do with Alice and Bob if/when polkadot JS supports ethereum-style derivation.

@JoshOrndorff JoshOrndorff deleted the joshy-alice-bob branch July 6, 2021 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. A8-mergeoncegreen Pull request is reviewed well. B5-clientnoteworthy Changes should be mentioned in any downstream projects' release notes C3-medium Elevates a release containing this PR to "medium priority".
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants