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

Implementation for derived submodule addresses #8516

Closed
4 tasks done
robert-zaremba opened this issue Feb 4, 2021 · 15 comments
Closed
4 tasks done

Implementation for derived submodule addresses #8516

robert-zaremba opened this issue Feb 4, 2021 · 15 comments
Assignees
Labels
C:Crypto T: ADR An issue or PR relating to an architectural decision record

Comments

@robert-zaremba
Copy link
Collaborator

Summary

While trying to close ADR-028, we found that we need to analyze derived submodule addresses - which is not fully covered by the ADR-028 yet.

Problem Definition

Modules may have sub accounts and that subaccount can derive other accounts. Example: cosmwasm smart-contracts.

In the latest ADR-028 update we left notes for the approach we are leaning into.

Example:

submodule := address.Module(moduleName, key)
derivedSubmoduleAccount := address.Derived(submodule, key2)

However we miss more real-world examples.

Proposal

  • Finalize ADR-028 and add missing functions to types/address package.
  • Validate if this will create breaking changes

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@robert-zaremba robert-zaremba added C:Crypto T: ADR An issue or PR relating to an architectural decision record labels Feb 4, 2021
@robert-zaremba
Copy link
Collaborator Author

@ethanfrey - would love your feedback here. How account addresses are created in cosmwasm? What kind of derivation mechanism are you using? How long is the derivation chain?

@ethanfrey
Copy link
Contributor

Cosmwasm doesn't use module sub accounts. It uses normal accounts. We just generate a "random" hash and claim it, assuming there will be no colision. It is hash of the code id and the contract sequence.

I think with adr028 we can generate our keys in another way to avoid collisions, but are pretty agnostic. I have 2 uint64 in the module and want to generate a unique address for each one. Beyond that no real preference, so if you define some canonical mapping, we can adjust

@aaronc
Copy link
Member

aaronc commented Feb 4, 2021

Just to clarify, this would specifically be about a contract being able to create any number of addresses that it controls. If for instance it managed its own account types.

@ethanfrey
Copy link
Contributor

ethanfrey commented Feb 4, 2021

Just to clarify, this would specifically be about a contract being able to create any number of addresses that it controls. If for instance it managed its own account types.

Interesting. Our current model is only one address per contract. I have used multiple sub-contracts to simulate multiple addresses, which works but is rather inefficient. So each contract could have an "address space" that is the idea?

We don't use module accounts at all as they have some differences from "external account" currently. And contracts are permissioned like external actors (untrusted). I really don't see the need for a separate "module account" type. Just some way to derive independent/non-conflicting addresses for each module.

It seems like you are working towards something like:

external: sha256(pubkey_type || pubkey_bytes)
module: sha256(module_name)
sub-module: sha256(module_name || key)

cosmwasm currently: sha256("C" || code_id || contract_counter) - more or less like a sub-module.

See https://github.com/CosmWasm/wasmd/blob/4141cc36f8de344711013bae37b532b27dc1dee8/x/wasm/internal/keeper/keeper.go#L678-L691 and https://github.com/CosmWasm/wasmd/blob/4141cc36f8de344711013bae37b532b27dc1dee8/x/wasm/internal/keeper/keeper.go#L752-L757

I don't know what the functional difference between an external and a sub-module account would be here, except the derivation process. As of 0.39, there was a different way to send to an external account than to send to a module account. This may well have changes since then. Maybe if I understand that, I can respond better.

If there is a new standard that accomplishes basically what we are doing already, I am happy to adjust our derivation in the first wasmd release that uses 0.42.

@aaronc
Copy link
Member

aaronc commented Feb 4, 2021

So ADR 028 describes module account address derivation. We would of course appreciate any thoughts you have on it.

Module accounts within x/auth mainly serve to not support PubKey afaik and imho x/auth should be greatly simplified and not use such abstractions. But that's a bit out of scope of this issue...

@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Feb 4, 2021

@ethanfrey - thanks for chiming in. What's codeID and contractSequence? Sounds like a code hash and a counter respectively.

So, are currently you are using 32 byte addresses? Is anyone using comswasm in production? Are address breaking changes still allowed in cosmwasm?

@robert-zaremba
Copy link
Collaborator Author

robert-zaremba commented Feb 4, 2021

It seems like you are working towards something like [ ... ]

Not really. Each address is constructed from two arguments: scheme and a key. For example, submodule address is:

sha256(sha256("module"), moduleName || 0 || key)
        |--- scheme --|

@ethanfrey
Copy link
Contributor

@robert-zaremba
Yes, Terra and Secret mainnet use CosmWasm, and many testnets. We use 20 byte addresses like the rest of the sdk (please see linked code). I just skipped the truncation while describing all the derivations to focus on the prehash data.

@ethanfrey
Copy link
Contributor

@aaronc I did some reflection last night and came to the conclusion that we do not need multiple contract addresses. Whatever we could do with that, we could also do via contract composition, which makes it opt-in and a much simpler API for all normal use cases.

That said, I am willing to update a future release with a different derivation scheme to match that of your submodules, each contract getting one key but using the same pattern you use for core modules for the prehash

@robert-zaremba
Copy link
Collaborator Author

That said, I am willing to update a future release

That would create a breaking change - how will you deal with it? Will the new algorithm apply only to new smart contract addresses?

@ethanfrey
Copy link
Contributor

We have a lot of wasm breaking changes coming up in the next release and would need a fresh start anyway.

@robert-zaremba
Copy link
Collaborator Author

What's the planned date of the next wasm release?

@ethanfrey
Copy link
Contributor

Late Feb or early March. Ideally based on 0.42 if that is released and stable by then

@sunnya97
Copy link
Member

sunnya97 commented May 7, 2021

@robert-zaremba was this closed by #9088?

@robert-zaremba
Copy link
Collaborator Author

Yes, thanks @sunnya97 for checking.
Closed by #9088

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Crypto T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

No branches or pull requests

4 participants