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

mediate object methods in wallet (simple data-only args) #3901

Closed
dckc opened this issue Sep 27, 2021 · 16 comments
Closed

mediate object methods in wallet (simple data-only args) #3901

dckc opened this issue Sep 27, 2021 · 16 comments
Assignees
Labels
devex developer experience Governance Governance needs-design wallet

Comments

@dckc
Copy link
Member

dckc commented Sep 27, 2021

Describe the bug

In the demo we just did (#3869 ) most of the UI limitations were due to time constraints, but one that I don't know how to address even if I had lots of time is: if we sent invitations using wallet deposit facet, how would a voting dApp get access to voting rights, which currently come via zoe getOfferResults?

Additional context

@dtribble seemed to suggest that this is a solved problem, but I haven't looked into the details...

For apps like the treasury with nested offers, the offer result is a particular structure that the wallet recognizes so that it can provide sub invites

... so to be conservative, I'm raising this issue.

cc @rowgraus @katelynsills @Chris-Hibbert @michaelfig @samsiegart

@dckc dckc added bug Something isn't working wallet devex developer experience Governance Governance labels Sep 27, 2021
@dckc dckc added the Zoe package: Zoe label Sep 27, 2021
@katelynsills katelynsills removed bug Something isn't working Zoe package: Zoe labels Sep 28, 2021
@katelynsills
Copy link
Contributor

We have a design for this, but it's probably not adequately represented in an issue outside of the fee charging plans. (see mention of proxy-like objects in #3294 (comment)).

But essentially, it's the same thing even if we're not thinking about fee purses:

The wallet creates a new proxy-like object and sends it to the dapp, so that the dapp can call methods on the object as if it is the offerResult directly, but when a method is called, the wallet gets the message and can present it to the user for approval. If approved, the wallet calls the same method on the original object.

@katelynsills
Copy link
Contributor

One key design point is - Dapps should never, ever get direct access to an offerResult that belongs to a dapp user. OfferResults are often very high authority objects. For example, in the case of the Treasury vaults, an offer result is what allows the user to close a vault. If a dapp gets access, the dapp can take the user's collateral by paying back the loan, which because of overcollateralization, means they steal a lot of money from the user. We can't let that happen.

@dckc
Copy link
Member Author

dckc commented Oct 8, 2021

@Chris-Hibbert best I can tell, we need to re-structure voting rights offer result to match this invitationMakers structure...

    return harden({
      uiNotifier: notifier,
      invitationMakers: Far('invitation makers', {
        AdjustBalances: vault.makeAdjustBalancesInvitation,
        CloseVault: vault.makeCloseInvitation,
      }),
      vault,
    });

-- https://github.com/Agoric/agoric-sdk/blob/master/packages/treasury/src/vaultManager.js#L266-L273

A few examples of that style plus the addOffer makeContinuingInvitation test are the only docs I see.

@dckc dckc added the Risk: High label Oct 8, 2021
@dckc
Copy link
Member Author

dckc commented Oct 8, 2021

@rowgraus it's not clear that the existing invitationMakers technique is feasible. What do you think of changing the wallet protocol as @katelynsills suggested Sep 27? #3901 (comment)

cc @dtribble @michaelfig @samsiegart

@dckc
Copy link
Member Author

dckc commented Oct 13, 2021

@michaelfig and I chatted about this... it does seem at least possible to use the continuingInvitation / invitationMakers technique... electorate could mint payments... more later...

@katelynsills
Copy link
Contributor

Hi @dckc, please don't change the contracts so that things that should be arguments are payments. That's doing a lot of work for a worse user experience. We know already that we need to build this out in the wallet. Let's do that instead.

@dckc
Copy link
Member Author

dckc commented Oct 13, 2021

In a meeting just now we discussed having the wallet mediate calls to use objects (as suggested Sep 27 above). The code that checks for continuingInvitation / invitationMakers in the wallet could fall back to supporting arbitrary method calls on the use object. Given that the arguments are all passable, it doesn't even require a full membrane. I can see how an exploratory UI could be just a few hours work. It's unlikely to fit in for this week's demo, but if @samsiegart and/or I can fit it in, @katelynsills , @dtribble are supportive (and I didn't hear any objections).

Meanwhile, @katelynsills , when you say we know we need to build this out in the wallet, I don't quite understand. As to me personally, I don't know for sure. As far as I know, using offers / payments for voting could work fine. I don't plan to do any work in that direction, but as far as I know, it isn't ruled out as a possible design.

@katelynsills
Copy link
Contributor

Meanwhile, @katelynsills , when you say we know we need to build this out in the wallet, I don't quite understand... As far as I know, using offers / payments for voting could work fine. I don't plan to do any work in that direction, but as far as I know, it isn't ruled out as a possible design.

Sure, I can explain. We need our wallet UI to support users calling methods on object capabilities because that is how we have chosen as a company to represent access control. By contrast, Ethereum uses addresses and the addresses' ownership of tokens for access control. The only reason we have a different access control story because we allow access through holding an object and calling methods on it. If we didn't allow users to hold object capabilities, we would be the same as Ethereum and have no access control differentiation. So not supporting object capabilities in the wallet would be throwing away a huge part of who we are as a company, and what we've been working towards.

And, because we've explicitly chosen the object capability approach, our contracts use object capabilities for access control. For instance, when a user opens a vault in the Treasury, they receive an object that has methods for adding collateral, increasing the loan, and closing the loan entirely. Only the holder of this object can take these actions.

The wallet holds all of the user's objects, and needs to correctly handle valuable object capabilities like the vault. But, there's a problem, which is that the wallet doesn't know what vaults are, or how to represent a vault object to a user. This is where dapps shine: they know the specifics of contracts and know the business meaning of particular objects. However, dapps are not trusted by the user, and should not be. Dapps should never have direct access to the vault object, because then they would be able to close the loan and take the collateral for themselves, stealing assets from the user for a hefty profit.

To put it simply: dapps have all of the domain specific knowledge, but none of the authority, and wallets have all of the authority and none of the domain specific knowledge.

The solution we came up with is that the dapp can tell the wallet what to do with the vault based on the user's actions in the dapp UI, and the wallet can choose to show this request to the user for approval, or otherwise have some logic for doing the action or not. To accomplish this, the wallet gives the dapp a proxy-like object so that the dapp has the experience of calling methods on the object capability, but the wallet has complete control over whether the calls go through. This shouldn't be too hard, especially since we know we need to rewrite the wallet backend entirely anyways for it to be auditable.

@dckc, to your question of what has been ruled out, you should consider the idea of eliminating the use of object capabilities in contracts and replacing them with tokens ruled out, by me. As engineering lead on Zoe, I would not support any restrictions on the design of Zoe or Zoe contracts that eliminate the use of object capabilities by users. We need our wallet to support object capabilities, because that's our product story on access control.

@dckc
Copy link
Member Author

dckc commented Oct 13, 2021

I think this is a first... someone trying to persuade me to use / support ocaps. :)

But ERTP is also a big part of our product story, and it seems coherent to me that the wallet might stick to the ERTP layer, i.e. to manipulating transferable rights. The vault case does argue otherwise. I'll have to think it over.

@dckc
Copy link
Member Author

dckc commented Oct 13, 2021

Thanks for taking the time to spell out the argument, @katelynsills . I'm sorry to make you take the time for that... I have some "trial by combat" habits that I have been trying to break for some time, but they still come out now and again.

@katelynsills
Copy link
Contributor

Thanks for taking the time to spell out the argument, @katelynsills . I'm sorry to make you take the time for that... I have some "trial by combat" habits that I have been trying to break for some time, but they still come out now and again.

No problem at all! The more people we have thinking about these kinds of problems, the better.

@dckc dckc changed the title voting rights as offerResults conflicts with wallet API? mediate offerResults methods in wallet (simple data-only args) Oct 16, 2021
@dckc dckc changed the title mediate offerResults methods in wallet (simple data-only args) mediate object methods in wallet (simple data-only args) Nov 8, 2021
@Tartuffo Tartuffo added the MN-1 label Jan 21, 2022
@dckc dckc removed the Risk: High label Jan 25, 2022
@Tartuffo Tartuffo removed the MN-1 label Feb 7, 2022
@rowgraus rowgraus removed their assignment Feb 11, 2022
@dckc
Copy link
Member Author

dckc commented Feb 16, 2022

we can use deploy scripts for voting etc. for mainnet-1
(wallet meeting with @dtribble etc.)

@dckc
Copy link
Member Author

dckc commented May 13, 2022

Lack of this feature is really painful while working on governance for the Inter protocol (#5169 (comment) )

I'm starting to think that "simple data-only args" is not a very interesting line to draw. That way leads to dialog boxes such as:

  • OK to invoke the castBallotFor method on offer result of concise rendering of an offer?

which leads to dialog fatigue, not security.

The interesting line is: which idioms correspond naturally to user gestures? For example, could we

  • use drag-and-drop to designate voting? drag the choice onto the offer?
  • or petnames? put a petname for the offer result in the "Vote" button in the dApp and put the method in hover text? That is: perhaps the dApp could say to the wallet "please put a button here for calling method X on the result of offer N". Is it feasible to securely relate the arguments to things like radio buttons? Is there some general pattern for secure integration of forms?

ref

cc @turadg @Chris-Hibbert @samsiegart

@0xshipthecode
Copy link
Contributor

Hi, we found this issue while trying to understand how we would consume invitations from offers within a dApp. One note:

@dckc if the dApp can control the styling/content in the offer associated with the "please put a button here for calling method X on the result of offer N", this is immediately useful for attacks on the user (fraudulent/misleading text).

The inventiveness of malicious actors in misleading users is hard to overestimate.

@dckc
Copy link
Member Author

dckc commented Jun 15, 2022

Yes, finding something more flexible than the continuing offer pattern while preventing the dApp from confusing / corrupting the offer is the challenge that has kept this issue in the needs-design pile for months.

@dckc
Copy link
Member Author

dckc commented Nov 4, 2022

We found the continuing invitation pattern sufficient for voting and for submitting oracle price updates. I think we're more or less content with it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devex developer experience Governance Governance needs-design wallet
Projects
None yet
Development

No branches or pull requests

5 participants