feat: ADR-036: Arbitrary Message Signature Specification

[fdymylja]

Description

tentative implementation for #7727

also there's an example implementation of a rest API that does signing and verification off offline messages with postman's collection for testing: https://github.com/fdymylja/adr036-example/blob/master/main.go

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[tac0turtle]

closing due to staleness. Once the sdk team has dev cycles or a contributor is willing to complete this we can reopen it.

Please do not reopen if there is not a plan to completion

[RiccardoM]

@marbar3778 I can pick up this PR. May you re-open it? Will work on it starting on Monday

[tac0turtle]

Are you able to push to this branch or want to open a new pr?

[RiccardoM]

@marbar3778 Oh yeah I don't have write permissions to the repo. I think the only way for me is to open a new PR. It would be a shame to loose all the history though 🤔

[tac0turtle]

@RiccardoM you should be able to push here now

[RiccardoM]

May I get one review from @robert-zaremba @webmaster128 please? I think there isn't much more work to be done in order to close this honestly

[tac0turtle]

one minor comment, but the rest looks good to me

[tac0turtle]

we should rename this

[alexanderbez]

Thank you @RiccardoM for helping getting this over the finish line! @aaronc expressed some concern during the triage call that was received from upstream SDK consumers:

FWIW, I think ADR36 is not right, at least for this auth challenge-response case. https://github.com/ethereum/EIPs/blob/master/EIPS/eip-4361.md is much more reasonable. The nonce MUST be set, and there must be an agreed "memo" to present to the user about this transaction. I also feel that even if the transaction itself is not put on-chain, that it should be POSSIBLE to put it on-chain, potentially on a different chain than the one represented by the key itself (well, via whatever pubkey->address scheme is used, if more than bech32 is needed to say where the key may be found).

Do you have an idea of how far off line we are with respect to EIP-4361 or if the concern is even valid? We can still get this merged, but I want to make sure we cover all bases, or least as many as possible to make this feature as viable as possible for clients.

[tac0turtle]

would love @zmanian input on the above as well

[blorgon1]

I've noticed the spec contains:

chain-id, account_number and sequence can all be assigned invalid values.

But the rules contained in that same section do not specify what the value for account_number should be (chain-id and sequence are specified as ""/adr-36/<chain_id> and 0).
Should the account number be specified (probably as 0)? The SignDoc contains the account number so it will affect the signature.

[zmanian]

for eip-4361 type applications, the nonce is server provided vs the on chain sequence number.

So the nonce would be set in the challenge

[tac0turtle]

converted to draft for now.

[frumioj]

Thank you @RiccardoM for helping getting this over the finish line! @aaronc expressed some concern during the triage call that was received from upstream SDK consumers:

FWIW, I think ADR36 is not right, at least for this auth challenge-response case. https://github.com/ethereum/EIPs/blob/master/EIPS/eip-4361.md is much more reasonable. The nonce MUST be set, and there must be an agreed "memo" to present to the user about this transaction. I also feel that even if the transaction itself is not put on-chain, that it should be POSSIBLE to put it on-chain, potentially on a different chain than the one represented by the key itself (well, via whatever pubkey->address scheme is used, if more than bech32 is needed to say where the key may be found).

Do you have an idea of how far off line we are with respect to EIP-4361 or if the concern is even valid? We can still get this merged, but I want to make sure we cover all bases, or least as many as possible to make this feature as viable as possible for clients.

I am one of the people that @aaronc references as an "upstream SDK consumer" with concerns about the text of this ADR.

Our specific use-case is where an ordinary (not web3) "website" which may or may not also be connected to a blockchain, may wish to accept a wallet-based authentication (see Keplr/Commonwealth for example). In this case, I'd like the rules to make it easier for the wallet user to understand what is going on, and also allow for the possibility of the transaction being put on-chain, and perhaps not on a chain already known to the user (keplr allows the server to propose chain parameters for a new key to be created, linked to a new chain unknown to the wallet, for example).

So, for example:

"The transaction for signing MUST contain a human-readable memo conforming to these general rules" (note: EIP 4361 gives an actual template)
"The entity making the request for signature MUST identify itself in such a way as to allow a human to understand who is asking for their signature, and noting that this human may be using a hardware ledger to verify the transaction."

The security parameters (e.g. nonce and timestamp) MUST be specified, and ideally it should be possible to verify the requester's signature over these fields, to ensure the signature is a valid request for the recipient.

[tac0turtle]

#12327

closing in favour of an issue for now