966 grants spike

[alpe]

See #803
Design spike! Do not merge
Based on #966

[mergify]

⚠️ The sha of the head commit of this PR conflicts with #966. Mergify cannot evaluate rules on this PR. ⚠️

[giansalex]

I tested max-calls with this transaction and works fine.

Transaction

{
   "body":{
      "messages":[
         {
            "@type":"/cosmos.authz.v1beta1.MsgGrant",
            "granter":"wasm1tr944mwl56j7yhqkgq5xmah8w7udjwpmj33a4y",
            "grantee":"wasm14vhcdsyf83ngsrrqc92kmw8q9xakqjm048cr0y",
            "grant":{
               "authorization":{
                  "@type":"/cosmwasm.wasm.v1.ContractExecutionAuthorization",
                  "grants":[
                     {
                        "contract":"wasm14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9s0phg4d",
                        "infinite_calls":{  
                        },
                        "accepted_message_keys":{
                           "messages":[
                              "claim",
                              "bond"
                           ]
                        }
                     },
                     {
                        "contract":"wasm1suhgf5svhu4usrurvxzlgn54ksxmn8gljarjtxqnapv8kjnp4nrss5maay",
                        "max_calls":{
                            "remaining":"1"
                         },
                        "allow_all_wildcard":{
                        }
                     }
                  ]
               },
               "expiration":"2023-09-07T01:54:56Z"
            }
         }
      ],
      "memo":"",
      "timeout_height":"0",
      "extension_options":[
         
      ],
      "non_critical_extension_options":[
         
      ]
   },
   "auth_info":{
      "signer_infos":[
         
      ],
      "fee":{
         "amount":[
            
         ],
         "gas_limit":"200000",
         "payer":"",
         "granter":""
      }
   },
   "signatures":[
      
   ]
}

Grants list:

grants:
- authorization:
    '@type': /cosmwasm.wasm.v1.ContractExecutionAuthorization
    grants:
    - accepted_message_keys:
        messages:
        - claim
        - bond
      contract: wasm14hj2tavq8fpesdwxxcu44rty3hh90vhujrvcmstl4zr3txmfvw9s0phg4d
      infinite_calls: {}
    - allow_all_wildcard: {}
      contract: wasm1suhgf5svhu4usrurvxzlgn54ksxmn8gljarjtxqnapv8kjnp4nrss5maay
      max_calls:
        remaining: "1"
  expiration: "2023-09-07T01:54:56Z"
  grantee: wasm14vhcdsyf83ngsrrqc92kmw8q9xakqjm048cr0y
  granter: wasm1tr944mwl56j7yhqkgq5xmah8w7udjwpmj33a4y
pagination:
  next_key: null
  total: "0"

I was thinking about funds, should a restriction be added? no funds allowed, max/unlimited funds, ...

[alpe]

@giansalex thanks for the feedback! 🏄

I was thinking about funds, should a restriction be added? no funds allowed, max/unlimited funds, ...

Do you have a use case in mind that you can share? The amount submitted on execution is payed by the sender and may be granted via SendAuthorization (not tested in which order authorizations are applied)

• Any thoughts how the CLI params should look like? It won't be trivial as it needs to cover multiple contracts with multiple accepted message types. I am also missing a "merge" or "append" command in the SDK to modify an existing authz. So it will be set/ remove all at once or we do it fully on the client side

• Should we allow multiple grants for the same contract address? Like ["contractA:foo,bar:1", "contractA:bar:unlimited", "contractA:*:100"]

[giansalex]

Do you have a use case in mind that you can share? The amount submitted on execution is payed by the sender and may be granted via SendAuthorization (not tested in which order authorizations are applied)

SendAuthorization only works for bank.MsgSend, I have tested and it is possible to send granter funds to the contract
something like allow_funds: bool could be added, as a user I would feel safer if the grantee cannot send funds from my balance, unless I explicitly state so.

Any thoughts how the CLI params should look like? It won't be trivial as it needs to cover multiple contracts with multiple accepted message types. I am also missing a "merge" or "append" command in the SDK to modify an existing authz. So it will be set/ remove all at once or we do it fully on the client side.

For complex parameters osmosis use a config file
on frontend side, I see that each app (grantee) updates all the permissions it needs. (e.g. auto-compounders).

Should we allow multiple grants for the same contract address? Like ["contractA:foo,bar:1", "contractA:bar:unlimited", "contractA:*:100"]

Maybe useful in other scenarios.

[alpe]

Thanks for the feedback!

allow_funds: bool could be added, as a user I would feel safer

very important point! I think this is mandatory and we can apply the same model as bank or staking authz and have a

repeated cosmos.base.v1beta1.Coin deposit_limit = 1
      [(gogoproto.nullable) = false, (gogoproto.castrepeated) = "github.com/cosmos/cosmos-sdk/types.Coins"];
}

(or better name). This adds some complexity though as we would be in a situation where limit can be 0Stake by intention or 0 because it was all spent. Imagine [contractA:foo,1,0Stake, contractA:*,*,10Stake]. What would be the algorithm to delete a grant? Even more complicated with multiple tokens.

[giansalex]

a situation where limit can be 0Stake by intention or 0 because it was all spent.

In the second case, the grant must be deleted when all funds are spent, considering that a grant is deleted if deposit-limit or max-calls are spent, whichever occurs first.

Imagine [contractA:foo,1,0Stake, contractA:,,10Stake]. What would be the algorithm to delete a grant?

yes, it becomes more complex, maybe only allow one contract for now.

[alpe]

I thought a bit about the amount limit and we can reduce complexity a lot when we add it to the execution count block (renamed to execution_limit below):

    // ExecutionLimit specifies number of executions or spendable amounts
    oneof execution_limit {
      InfiniteCalls infinite_calls = 2;
      MaxCalls max_calls = 3;
      MaxFunds max_funds = 4; // contains Coin amounts only
    }

The logic becomes easier as infinite and max_calls can not be used when coins are sent. When max_calls is down to 0 the grant can be deleted. Same when max_funds becomes empty. I have pushed an update.
I would also expect the UX for the CLI will benefit from this change. WDYT?

[giansalex]

I have a use case for unlimited funds: claim staking rewards and call cw-liquid-staking:bond,
another field could be added like InfiniteCalls: UnlimitedFunds

[alpe]

Sorry, I am busy with the v0.29 milestone but will follow up as soon as I can. Other feedback is welcome, too

[nicolaslara]

The design here seems to be focused on providing specific authorizations for a few use cases. Can we turn this on its head and instead have a general design where an authorization uses a contract to determine if a transaction is authorized? This would give uses much more flexibility as they can write their own contracts instead of depending on the foresight of the devs implementing the grant at the go level.

There are two requirements for this to work. We need to be able to:

• Query a contract to determine if a tx should be authorized
• Send an execute message to those contracts so that they can update their internal state in case the transaction is authorized and included in the block

I envision this to be done as a tree of authorizations (and I have an initial implementation of it here: https://github.com/nicolaslara/authorizations) but anyone can handle it differently depending on their use cases (a more concrete implementation of this is how I've been trying to integrate authorizations into daodao: DA0-DA0/dao-contracts#358)

What's the argument for implementing the specific cases (execution_limit, send authorizations, message filtering) at the wasmd / go level?

[mvid]

This feature would help me unblock my dev and product teams, if there is anything I can do to help move it forward, please let me know

[alpe]

Do we really need this? I would prefer to not provide Infinite(s) but have high MaxCalls instead. Less options for the CLI or other places that display state.

[alpe]

Sorry for the delay and thanks for the feedback!

@nicolaslara

. Can we turn this on its head and instead have a general design where an authorization uses a contract to determine if a transaction is authorized...

Good idea but you can do this already with a generic wasm execute msg granted for an (auth) contract that acts as a proxy. In fact, I was arguing the same in the beginning but the end user UX would not be great and the contract(s) hard to trust. Explorers and other frontends may want to display the state.

@nicolaslara, @mvid and all others:
The common use case as I understand them were covered by the limits and filters defined. Please share other concrete use cases, so that the design can be extended.

[nicolaslara]

you can do this already with a generic wasm execute msg granted for an (auth) contract that acts as a proxy.

yes. I would argue that this becomes more complex and has worse UX than defining a specific "generic authorization" interface that wallets and others know how to display and query. Having a generic authz authorization that queries for auth avoids having to give a contract full exec permissions and the complex wrapping from the user's perspective. This could be the "authorization of last resort" for people that need something more complex.

If you think it makes sense for wasmd to provide that, I'll be happy to code that authorization and add it to this PR.

[nicolaslara]

Please share other concrete use cases, so that the design can be extended.

Not a concrete use case, but I think the filtering could be turned into deep matching: instead of matching only on the message type, we could do this recursively. This would allow is, for example, to authorize a swap between two specific tokens, as opposed to authorizing all swaps.

The counter-argument here is that for something like that you can just write a contract that deals with the specifics and authorize that particular contract.

[ethanfrey]

"allowances" in cw20-base are basically this contract-specific authorisation, and it counts how many tokens they have sent (deep understanding of message format).

This can always be done, just takes some work. I don't think a Go-Rust hybrid would be easier. I do think finding some common idiom for such in contracts is a good idea, so they have similar interfaces for authz type stuff (fully in contracts).

However, there are some who wish more stuff with native authz which must be limited to general stuff that works on all contracts. I think they both make sense, but wouldn't mix the two.

[ethanfrey]

This seems to have been hanging for a month and I am getting plenty of dms asking for it.

I would like to get a basic version in merged to main, with an eye to keep it extensive for more prs as follow up (both before 0.30 release and backwards compatible after it)

[alpe]

Closing this in favour of #1077 . Please comment on the new design