arc icon indicating copy to clipboard operation
arc copied to clipboard

Make getParametersFromController public

Open dkent600 opened this issue 7 years ago • 5 comments

getParametersFromController is currently private in UniversalScheme. It would be useful to client apps (like arc.js) if this could be made public.

  1. it would be a little more reliable than using 'updateParameters' for identifying a scheme as being universal
  2. it would make it easier to obtain the scheme's parameters hash, given an avatar

dkent600 avatar Aug 17 '18 14:08 dkent600

Maybe we could add a property "UniversalScheme.isUniversal" that always returns true?

dkent600 avatar Aug 17 '18 14:08 dkent600

  1. why there is need to identify a scheme as being universal ?
  2. to obtain a scheme parameters hash giving an avatar you can use https://github.com/daostack/arc/blob/master/contracts/controller/ControllerInterface.sol#L77

orenyodfat avatar Aug 22 '18 06:08 orenyodfat

why there is need to identify a scheme as being universal

  1. It helps in these arc.js issues in knowing how to handle a scheme that has been requested to be registered:

https://github.com/daostack/arc.js/issues/330 https://github.com/daostack/arc.js/issues/335

  1. as a polymorphic coding principle or best practice: There should generally be a canonical way to determine whether a class implements/extends a particular class or interface.

  2. getParametersFromController is the essence of what defines a universal scheme (along with updateParameters). If one wanted to create a javascript class representing everything in common between all universal schemes, a class that in code defines what a universal scheme is and does, then this method would be critical.

  3. it makes no sense to make updateParameters public but not getParametersFromController

Note: it might make sense to rename updateParameters or getParametersFromController to make the two more symmetrical with one another.

to obtain a scheme parameters hash giving an avatar you can use [...]

Yes, but that requires knowing how to obtain a scheme's controller and pulling in the Controller contract ABI, with associated dependencies in the code. Making getParametersFromController public is a lot simpler and easier.

dkent600 avatar Aug 22 '18 12:08 dkent600

  1. I do not see how it helps with daostack/arc.js#330 daostack/arc.js#335 . The wrapper of each of these scheme should have the knowledge of how to initiate it.
  2. Still not sure why
  3. Not sure "getParametersFromController" this is define a universal scheme .
  4. updateParemetes is used to extend the parameters from outside and it onlyOwner , getParametersFromController is an internal helper.
  5. I think using getSchemeParameters is easy enough

orenyodfat avatar Aug 22 '18 13:08 orenyodfat

@orenyodfat

I do not see how it helps with daostack/arc.js#330 daostack/arc.js#335 The wrapper of each of these scheme should have the knowledge of how to initiate it. Not sure "getParametersFromController" this is define a universal scheme .

Schemes need not need to have wrappers to be able to be registered into a DAO. Nor do they have to be deployed by the running version of Arc.js. They don't even have to be Arc schemes. In all of these cases, DAO.new needs to know what to do about scheme parameters, and that means knowing whether the scheme supports an API common to universal schemes. The best way to know that is to follow the rules defined in UniversalSchemeInterface.

I opened this ticket to suggest changes to UniversalSchemeInterface, however, an older closed ticket already exists on the same subject, so I have reopened it with a new comment clarifying and summarizing the request.

A PR in Arc.js proposes to determine whether any given address points to a universal scheme by checking for the existence of an updateParameters method at the address (see that code here).

But updateParameters is less than reliable as a heuristic for determining whether a contract is universal because it is a method name that could easily be used by non-universal schemes. It is also a method that is used nowhere in Arc, and so could easily be removed. getParametersFromController is much more likely to be a good heuristic. A new property isUniversal would be best. (again, see this ticket.)

Still not sure why [there should generally be a canonical way to determine whether a class implements/extends a particular class or interface.]

We need to know when a contract is universal so we can know whether we can use the features of a universal scheme regarding the setting of scheme parameters. Being able to identify classes is a common need generally in object-oriented programming.

updateParemetes is used to extend the parameters from outside and it onlyOwner

I don't understand what is meant by "extend the parameters from outside"?

getParametersFromController is an internal helper. I think using getSchemeParameters is easy enough

As I mentioned above, using getSchemeParameters requires finding the controller of the scheme. Here is what Arc.js has to do to get the controller:

      const controllerAddress = await avatar.owner();
      if (controllerAddress) {
        const UControllerContract = await Utils.requireContract("UController");
        const ControllerContract = await Utils.requireContract("Controller");
        const uControllerAddress = (await UControllerContract.deployed()).address;

        this.isUController = uControllerAddress === controllerAddress;
        return this.isUController ?
          await UControllerContract.at(controllerAddress) :
          await ControllerContract.at(controllerAddress);
      }

Only then can you call getSchemeParameters.

getParametersFromController is obviously a lot easier., and helpful not just to Arc schemes, but also to consumers of Arc, not only as a helper function but in terms of structuring dependencies, as I described in a previous comment on this ticket. For Arc, it is, of course, just a change in the word internal to public.

dkent600 avatar Aug 24 '18 04:08 dkent600