stellar / soroban-rpc

RPC server for Soroban contracts.
18 stars 19 forks source link

Preflight / simulation should return auth-marked call stack #69

Open namankumar opened 8 months ago

namankumar commented 8 months ago

What problem does your feature solve?

There is no way for wallet to make the end user aware of the scope of the requested auth.

What would you like to see?

Simulation should return the following fields so that the wallet can display scope of the auth to the end user.

What alternatives are there?

None. Simulation today provides insufficient information to reconstruct the call stack.

2opremio commented 8 months ago

@namankumar doesn't result.auth in the simulateTransaction response contain everything needed?

Otherwise, can you clarify what's missing and what info you need exactly?

janewang commented 8 months ago

@2opremio Yes I can see that result.auth contains the auth invocation tree.

@namankumar could you clarify the what are the requirements for full call stack, including the top level invocation that you had in mind?

namankumar commented 8 months ago

It doesn't seem to connect the top level invocation with function calls later in the call stack. @aristidesstaffieri can shed more light on the specifics of what's missing

The wallet is expecting something like this:

swap_exact_tokens_for_tokens
    |_ foo (auth: user0)
    |_ bar 
    |_ token (auth: user1)
2opremio commented 8 months ago

@aristidesstaffieri please elaborate on exactly what’s needed. Thanks!

aristidesstaffieri commented 8 months ago

@aristidesstaffieri please elaborate on exactly what’s needed. Thanks!

We currently have everything we need in auth entries but the missing piece is in the top level host function. https://github.com/stellar/js-stellar-base/blob/6ca07504f7ff871083f01447578d5bc17b46ab81/types/index.d.ts#L896

The func key which represents the top level host function only includes the top level function called and not any sub invocations. Although the auth entries include this from their perspective, we have no way to show the user the call stack that the contract invocation will create.

I would expect in the swap case described above, that we would see something like swap() -> [transfer, transfer] but right now we only see the top level swap.

Even though each auth entry includes the root invocation from their perspective, there was a general reaction that we need both in order to better represent the invocation to users in our detail screen.

dmkozh commented 8 months ago

There is no way for wallet to make the end user aware of the scope of the requested auth.

The scope of the requested auth is defined by the auth entries and they only authorize what is being signed as a part of auth entries. So this statement is not valid as is.

Although the auth entries include this from their perspective, we have no way to show the user the call stack that the contract invocation will create.

I stand by my opinion that this is not necessary and would lead to confusion. Call tree is way too low level and will include a lot of noise (e.g. outgoing transfers from contracts).

Also, all the contract calls are already traced via diagnostic events. You may opt to display diagnostic events produced by simulation to 'power users', but it shouldn't be the feature that most of the users refer to. Also keep in mind that argument names are not included in the events and they're generally stripped from Wasm. While we could try to include the argument names into auth payloads, I don't think we can enforce this for generic contract calls.

I would suggest to focus more on the meaningful questions the user might have and the wallet/simulation could answer, such as:

Basically instead of offloading parsing potentially very complex traces to the user, I would try focusing on highlighting the important aspects of the invocations.