Payload exploration #120

jonjove · 2022-09-21T16:01:13Z

What

This PR contains a still-in-progress exploration of having contracts expose a payload function to facilitate signing in wallets and other downstream systems. To make development as fast as possible, I've replicated/modified auth functionality in this crate rather than working in soroban-auth. The final implementation of course would be in soroban-auth.

Earlier discussions of this idea used names like f_payload but in general those names don't comply with the 10 character symbol requirement. The universal payload function is the reaction to that issue. The signature of payload is almost certain to change as this exploration develops.

This also incorporates an idea from @dmkozh about putting all signatures into a single argument at the end of the argument list. Exploration on this part is very preliminary, and is almost certain to change entirely.

cc @leighmcculloch who requested this PR

Why

Making signing easier is a huge piece of our work in the last several weeks.

Known limitations

Too many to enumerate

dmkozh · 2022-09-21T18:07:06Z

token_with_payload/src/contract.rs

+
+ verify_and_consume_nonce(&e, &from_id, &nonce);
+
+ crate::auth::check_auth::<TokenPayload>(


Is the idea to use/reuse different payloads for different functions that need auth? I suppose you omitted that from the PR, as only this fn uses the new auth.

It's exploratory work. I learned what I needed from this part so I didn't do the rest of the work (as noted, I expect everything to change). But in practice every function will use the same new auth.

dmkozh · 2022-09-21T18:16:36Z

token_with_payload/src/contract.rs

+ fn payload(
+ e: Env,
+ function: Symbol,
+ args: Vec<RawVal>,


For me the main value of the initial proposal for having fn_payload was that we have a typed interface to contract fn we need to sign. Here it's still not clear what the args should be, which I think is on of the main issues with signing fns. Or is this complimentary to the initial proposal and contracts still would introduce fn_payload for fns with non-trivial args mapping?

There's not really any easy way to do the f_payload thing as noted in the PR description. I agree typed interfaces are nice, and I would prefer that. For the sake of this exploration, I went this way because it meant I could move fast without solving other problems (like length limits). The real implementation might look different.

leighmcculloch · 2022-09-21T18:40:42Z

token_with_payload/src/contract.rs

+#[cfg_attr(feature = "export", contractimpl)]
+#[cfg_attr(not(feature = "export"), contractimpl(export = false))]


Just an fyi, this doesn't do anything anymore. contractimpl always exports.

Good to know. I copied and pasted this from the token example, so it should be removed there too.

…and extend implementation to xfer

…h_payload

jonjove · 2022-09-21T20:18:48Z

Just updated this in a variety of ways:

Changed the sigs parameter from Map<Identifier, Signature> to Map<Symbol, Signature>
Changed the payload return value to Map<Symbol, SaltedSignaturePayload> (ignore the weird name, it's temporary)
Reverted check_auth implementation back to something more like version in main
Used this technique to implement liquidity_pool_router_with_payload deposit

There are still a couple of pieces missing from this, notably:

payload return value should be Map<Symbol, (Identifier, SaltedSignaturePayload)> or something like it (how else does the signing agent know what key(s) to sign with)
there should be a function needs_sigs that determines whether to call payload at all

…aturePayload)>

jonjove · 2022-09-21T20:49:46Z

Added the missing pieces described above and fixed a couple of bugs. A wallet or other downstream system would use this as follows:

Wallet receives request to call Contract::f(x, y, z...)
Wallet calls Contract::has_sig(f). If false, stop here and just submit the request as received. If true, continue to step 3.
Wallet checks that the last parameter to f is an empty map (there might be other things that could be done at this step but this is reasonable for starters)
Wallet calls Contract::payload(f, [args excluding the last parameter which is an empty map], [])
Wallet takes the result of payload and requests the appropriate signatures using the Identifier and SaltedSignaturePayload in each value, then packages those signatures back into a map using the Symbol in each key
Wallet submits Contract::f(x, y, z...) with the last parameter (which was an empty map) replaced by the map of signatures produced in step 5

I know that this flow is pretty complex, but I think it covers an enormous number of cases including interesting cases that involve functions that forward some-but-not-all signatures to other functions. See liquidity_pool_router_with_payload deposit for context on how that works.

This is basically concept-complete at this point so feedback on the idea would be great. Sorry that the implementation is largely hideous, but I'm confident it could be cleaned up.

jonjove · 2022-09-21T20:59:45Z

I forgot to mention that the wallet needs to do some verification on the payloads it receives before requesting signatures. Let me think about exactly what needs to be checked.

dmkozh · 2022-09-21T22:11:30Z

liquidity_pool_router_with_payload/src/lib.rs

+ );
+
+ let mut callstack_a = callstack.clone();
+ callstack_a.push_back((deposit_a.token.clone(), symbol!("xfer")));


Should call stack a contain this contract (i.e. liquidity pool)?

Great question. I think it could go either way. Contracts would probably be mildly cheaper if we defined "call stack" to mean everything before this function--this means the simple case of passing a signature to the first contract incurs no call stack cost since it would be empty. But the whole system would probably be slightly less surprising if we defined "call stack" to mean everything including this function. What do you think?

Isn't call stack here deposit->xfer ,so no matter which option do we choose deposit happens before xfer and hence should be included no matter which option we choose? IIUC our goal should be to distinguish 'xfer' from 'deposit->xfer' in order to prevent frontrunning messing up things, right?

dmkozh · 2022-09-21T22:11:56Z

liquidity_pool_router_with_payload/src/lib.rs

+ }
+}
+
+pub trait PayloadTrait {


Would this trait be a part of a library? I don't see it used in auth now.

Yes I think so. It would probably be in the soroban-auth crate since implementing this trait would be part of implementing standard auth.

dmkozh · 2022-09-21T22:14:39Z

liquidity_pool_router_with_payload/src/lib.rs

+ function: Symbol,
+ args: Vec<RawVal>,
+ callstack: Vec<(BytesN<32>, Symbol)>,
+ ) -> Map<Symbol, (Identifier, SaltedSignaturePayload)> {


This looks nice, pretty close to what I've imagined, albeit a bit verbose. Maybe we could provide a simple implementation for single-signature contracts, while allowing more complex contracts to be flexible.

I'm very open to having multiple versions of this, and I actually think that's one of the major strengths of this approach. For example, we could add a function fn payload_v(e: Env) -> Symbol (payload_v is short for "payload version") that returns an identifier for what payload function to call. We could even make that fn payload_v(e: Env, function: Symbol) -> Symbol if we wanted to allow it to vary by function. Then we could have a couple standards at different complexity levels, and the wallet would just invoke the right one based on the return value of payload_v.

jonjove · 2022-09-26T20:49:26Z

liquidity_pool_router_with_payload/src/lib.rs

+ .get_unchecked(symbol!("sig"))
+ .unwrap();
+
+ let to_verify = SaltedSignaturePayload::V0(SaltedSignaturePayloadV0 {


I think this isn't quite right. I'm pretty sure I would need to add to_forward_a and to_forward_b to this signature payload.

jonjove added 5 commits September 21, 2022 10:14

Create token_with_payload from token with tests removed

db58272

Bump env to fix RawVal conversions

4a64d4d

Implement payload-based auth for approve in token_with_payload

259fc03

fmt

9e95acd

Intermediate commit

fe5c056

dmkozh reviewed Sep 21, 2022

View reviewed changes

leighmcculloch reviewed Sep 21, 2022

View reviewed changes

jonjove added 2 commits September 21, 2022 15:11

Revert check_auth changes in token_with_payload, change sigs format, …

40f3928

…and extend implementation to xfer

Implement payload-based auth for deposit in liquidity_pool_router_wit…

21071ac

…h_payload

jonjove added 5 commits September 21, 2022 15:27

Change return value of payload to Map<Symbol, (Identifier, SaltedSign…

d3b4441

…aturePayload)>

Use passed in callstack instead of getting the callstack

fb78922

Add has_sig functionality

fad5d9c

fmt

10c8801

Add underscore to unused Env

285358c

dmkozh reviewed Sep 21, 2022

View reviewed changes

jonjove commented Sep 26, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Payload exploration #120

Payload exploration #120

jonjove commented Sep 21, 2022

dmkozh Sep 21, 2022

jonjove Sep 21, 2022

dmkozh Sep 21, 2022

jonjove Sep 21, 2022

leighmcculloch Sep 21, 2022

jonjove Sep 21, 2022

jonjove commented Sep 21, 2022

jonjove commented Sep 21, 2022

jonjove commented Sep 21, 2022

dmkozh Sep 21, 2022

jonjove Sep 22, 2022

dmkozh Sep 26, 2022

dmkozh Sep 21, 2022

jonjove Sep 22, 2022

dmkozh Sep 21, 2022

jonjove Sep 22, 2022

jonjove Sep 26, 2022


		verify_and_consume_nonce(&e, &from_id, &nonce);

		crate::auth::check_auth::<TokenPayload>(

		#[cfg_attr(feature = "export", contractimpl)]
		#[cfg_attr(not(feature = "export"), contractimpl(export = false))]

Payload exploration #120

Are you sure you want to change the base?

Payload exploration #120

Conversation

jonjove commented Sep 21, 2022

What

Why

Known limitations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonjove commented Sep 21, 2022

jonjove commented Sep 21, 2022

jonjove commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment