Skip to content

Frontier plus unified accounts#26

Merged
JoshOrndorff merged 73 commits into
mainfrom
joshy-frontier-plus-unified-accounts
Jul 11, 2023
Merged

Frontier plus unified accounts#26
JoshOrndorff merged 73 commits into
mainfrom
joshy-frontier-plus-unified-accounts

Conversation

@JoshOrndorff
Copy link
Copy Markdown
Collaborator

@JoshOrndorff JoshOrndorff commented Jun 26, 2023

This PR combines the work from both #9 and #20

I really hope that this unified account path will work out. My biggest question right now is whether it will work with pallet contracts and its associated tooling.

Update: This works with the contracts-ui as long as this patch is applied!

@JoshOrndorff
Copy link
Copy Markdown
Collaborator Author

Okay, so now CI is failing because cargo contract doesn't know how to send transaction to an AccountId20 chain. I'm willing to press on without that support because polkadot js knows how to do it and that means the contracts ui will still work. We would need to update the e2e tests to deploy and interact with the contract via polkadot js instead though.

@fbielejec
Copy link
Copy Markdown
Contributor

Okay, so now CI is failing because cargo contract doesn't know how to send transaction to an AccountId20 chain. I'm willing to press on without that support because polkadot js knows how to do it and that means the contracts ui will still work. We would need to update the e2e tests to deploy and interact with the contract via polkadot js instead though.

That's quite a chain reaction 😅 - I don't JS so I cannot help here, but maybe a step back is in order:

  • c-c is the "industry standard" for interacting with ink! SC's, and getting rid of that possibility will create a cascade of problems - maybe the right solution is to add support for other type of addresses to it, it might not be that hard, have you looked into it?
  • what is the reason to use a different address type actually in the first place?

@JoshOrndorff
Copy link
Copy Markdown
Collaborator Author

JoshOrndorff commented Jun 28, 2023

what is the reason to use a different address type actually in the first place

Our chain will host two smart contract platforms (pallet-evm and pallet-contracts). Pallet evm requires a 20byte ethereum-style account id. It is not just a limitation of the pallet either; this accountid type is hard-coded all the way down to the vm level; it is part of the evm semantics. On the other hand, Substrate and pallet-contracts have a nice flexible design that is generic over the account id. Knowing only this, the choice to use the ethereum-style accounts everywhere is the obvious one.

It gets tricky when you consider that most real chains that host pallet-contracts use the "normal" AccountId32. And therefore a lot of the tooling has not been designed with such flexibility in mind. Indeed much of the tooling has gotten away with hard-coding that account id type. Knowing this, the second potential strategy is to use the "normal" Substrate account id as the native account id and use it in pallet-contracts, and make users manually associate an evm address on-chain.

@notlesh pursued this strategy in #9 and you can explore it for yourself. The advantage is that all the pallet-contracts tools will work like they always do. The downside is that it requires student users (some of whom are new to blockchain) to understand and interact with this account mapping while we are trying to focus on writing and interacting with contracts.

So the question in my mind is What will hinder users more, the account mapping, or reduced tooling for pallet-contracts. This brings us to your next point

the right solution is to add support for other type of addresses to it, it might not be that hard, have you looked into it?

Yes, I have hacked on two important tools in the pallet-contracts ecosystem.

The Contracts-UI: issue, PR: I have this basically working. Ideally the maintainers would help us put a nice user-setting on it, but worst case, I can host this ui with my small patch and it will work. This was relatively simple because it is based on polkadot-js which already has full support that is used in production for ethereum signatures.

Cargo Contract: issue, PR: The compilation, testing, project-scaffolding stuff is all working as usual, but the stuff about interacting with the chain is not. I made an attempt in my PR, but I don't feel very close. This is based on sub-xt which appears to be generic over signature type, but I don't know whether any non AccountId32 types are used in production.


In my opinion, the contracts ui is enough for interacting with the chain, and I would prefer to keep the ethereum-style accounts. I think this shows off Substrate's flexibility, and is a good chance to encourage ecosystem tools to support the same genericity as Substrate.

However, I value your onpinions as well.

@fbielejec
Copy link
Copy Markdown
Contributor

fbielejec commented Jun 28, 2023

In my opinion, the contracts ui is enough for interacting with the chain, and I would prefer to keep the ethereum-style accounts. I think this shows off Substrate's flexibility, and is a good chance to encourage ecosystem tools to support the same genericity as Substrate.

However, I value your onpinions as well.

In my experience contracts-ui and such "manual" tools are good for one-off, simple cases, yet once there are multiple contracts involved in case of non-trivial deployments, or simply to reduce tedious repetitions you'd want to script some automation.

Adding on more tool to the mix breeds a bunch of problems, e.g. having a node.js instance, pulling node-modules, dockerizing for reproducibility across OS's and environments ..

Even if you harness polkadot/js for the deployment of the contracts, one will still have to use cargo-contract - for compiling them.

YMMV, but there is value in having a one-stop-shop for all of the above tasks (one of a big selling points of the Rust itself is cargo).

The above is just my 2 cents about using polkadot/js, I don't have a strong opinion on the address type issue, if it can be achieved in a relatively painless manner & without a significant overhead than why not. But if it drags on too long or proves to be a rabbit hole than it's not worth the effort IMO.

Comment thread account/src/lib.rs
Comment thread node/src/chain_spec.rs
Comment thread node/src/cli.rs Outdated
Comment thread runtime/src/lib.rs
Comment thread runtime/src/precompiles.rs
@JoshOrndorff
Copy link
Copy Markdown
Collaborator Author

@notlesh Does this PR (or your original) contain the self-contained transaction type? https://paritytech.github.io/frontier/rustdocs/fp_self_contained/index.html

If not, that may affect metamask-like tools. We should address that in a follow-up.

@notlesh notlesh mentioned this pull request Jul 6, 2023
@notlesh
Copy link
Copy Markdown
Contributor

notlesh commented Jul 6, 2023

@notlesh Does this PR (or your original) contain the self-contained transaction type? https://paritytech.github.io/frontier/rustdocs/fp_self_contained/index.html

If not, that may affect metamask-like tools. We should address that in a follow-up.

Good catch, it does look like that's missing. I opened #35 for this.

JoshOrndorff and others added 3 commits July 11, 2023 02:17
@JoshOrndorff JoshOrndorff reopened this Jul 11, 2023
@JoshOrndorff JoshOrndorff merged commit 00baa0f into main Jul 11, 2023
@JoshOrndorff JoshOrndorff deleted the joshy-frontier-plus-unified-accounts branch July 11, 2023 06:33
This was referenced Jul 11, 2023
JoshOrndorff added a commit that referenced this pull request Dec 13, 2023
…-frontier-plus-unified-accounts"

Joshy found the docs at https://git.github.io/htmldocs/howto/revert-a-faulty-merge.html very helpful in this process.

This reverts commit 00baa0f, reversing
changes made to c207fc5.
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.

4 participants