Organization.new does not rollback on error

[dkent600]

When Organization.new fails it can leave an organization existant but in an invalid state. It should be able to rollback what it has done when an exception or other error occurs prior to completion.

For example, a failure just occurred for me, and the organization now exists but has no schemes.

[leviadam]

But there is no way to remove a contract that was deployed (unless it is destructible and you are the owner). But it doesn't meter. We will have the last stage of listing it in a registry at the end.

[dkent600]

We will have the last stage of listing it in a registry at the end.

Good enough for me.

[dkent600]

@leviadam Should this be closed? It is not yet done...

We really need it right now, particularly while things are still unstable, because we're getting corrupt DAOs being registered, and they cause problems for Alchemy.

Perhaps this could be done at the same time we work #136 and #132.

[dkent600]

@leviadam I would like to work this, if possible. What is meant specifically by "We will have the last stage of listing it in a registry at the end."?

[dkent600]

I changed the title on this. We should either:

1. move the NewOrg event
2. rely on a different event (and move it?)
3. create a new event that is issued when everything is done.

[orenyodfat]

@dkent600 could you please provide a script for reproducing the issue ? Is that an arc-js(lib) issue or it require contract updates?

[dkent600]

I'm honestly not sure what the exact timing would have to be for an exception to be thrown that results in a corrupt DAO -- somewhere in GenesisScheme.forgeOrg perhaps, or in GenesisScheme.setInitialSchemes in the midst of registering all the schemes?

But it would be easy to throw an exception almost anywhere in Organization.new that would result in a DAO that, at best, does not meet the specifications given by the user (such as adding the required schemes). I image it could be a problem if an exception occurred in between GenesisScheme.setInitialSchemes and the final loop in Organization.new.

[orenyodfat]

GenesisScheme.forgeOrg and GenesisScheme.setInitialSchemes should revert if fail.If this is not the case so we need to fix it ..does it ? I guess that if it fail in between or before the final loop the lib should roll it up and inform the user/caller about the failure.

I image it could be a problem if an exception occurred in between GenesisScheme.setInitialSchemes and the final loop in Organization.new. Are you referring here ? https://github.com/daostack/arc-js/blob/master/lib/organization.js#L136

By the way , I do not see that the Organization.new test for InitialSchemesSet event of setInitialSchemes .why?

[dkent600]

@orenyodfat

GenesisScheme.forgeOrg and GenesisScheme.setInitialSchemes should revert if fail. If this is not the case so we need to fix it ..does it ?

Applications (well, Alchemy-Aurelia) currently use the NewOrg event to discover what organizations exist, in history and in real time. If said organizations are corrupt then we have no way to delete them or fix them, and they can really cause problems with the application. If said organizations are incomplete but at least intact, then a user might be able to repair them, but we would hae an unhappy user who would not trust our system.

Hence my request in the title that we do something different with respect to events, and define an event that reliably represents the complete and correct creation of an organization. I think that would be sufficient to address this issue, though I can imagine you might want to do the proper reverts as well, as you suggested.

I guess that if it fail in between or before the final loop the lib should roll it up and inform the user/caller about the failure.

I'm not clear how would we "roll up"?

By the way , I do not see that the Organization.new test for InitialSchemesSet event of setInitialSchemes .why?

Dunno, I didn't write any of that code and frankly didn't even know about that event. Doh.

[orenyodfat]

Applications (well, Alchemy-Aurelia) currently use the NewOrg event to discover what organizations exist, in history and in real time.

Well, I am not sure this is the right way for real time as organizations might be closed (we still do not have the scheme for that) and also as you wrote above might be corrupted. We might need kind of universal orgs registry contract which keep track of all orgs. @leviadam is that was your intention re "We will have the last stage of listing it in a registry at the end." ? We also consider to have a universal controller which might also provide such an orgs registry track.

Im not clear how would we "roll up"?

Mean to have an indication to the upper layer (error code or something).

Hence my request in the title that we do something different with respect to events, and define an event that reliably represents the complete and correct creation of an organization. I think that would be sufficient to address this issue, though I can imagine you might want to do the proper reverts as well, as you suggested.

InitialSchemesSet event is trigger upon successfully setSchemes (this is the new name of setInitialSchemes). We should document the events in the function comments and in general.

[dkent600]

Im not clear how would we "roll up"?

Mean to have an indication to the upper layer (error code or something).

The application is able to trap exceptions if they are thrown. But that doesn't help because the NewOrg event may already have been triggered.

InitialSchemesSet event is trigger upon successfully setSchemes (this is the new name of setInitialSchemes)

Just to be clear: setSchemes is not the last thing that happens in Organization.new. We need to know when the last thing has happened before achieving complete success in creating the org.

Alchemy would really like to have something here.

[dkent600]

OK, per our conversation today, the InitialSchemesSet event is the best option, but not ideal as it still leaves open the possibility that organization.new may fail after the event is fired, leaving the organization unfinished if not corrupt. Respecting that, we'll keep this open while considering better options.

[orenyodfat]

Well ,looking at the current code, it seems that organization.new doing the following :

1. forgeOrg
2. set parameters for each scheme it would like to register to the organization. ???
   • It use AbsoluteVote as parameters for all scheme , which I am not sure this is a right assumption for all cases.
   • It set parameters for contributionReward scheme ??? https://github.com/daostack/arc-js/blob/automated-tests/lib/organization.js#L105 3 .SetScheme which register all listed preliminary schemes to the organization.
3. Register the organization to the preliminary schemes.

• We can encapsulate phase 4 to the setScheme transaction,( by passing it an AUTO-REGISTER-ORG flag which will be ON be default) so setScheme will also register the org to the schemes. Doing so , will make InitialSchemesSet the last event for setting up an ORG with its scheme.

[dkent600]

Great comments @orenyodfat ! Thanks!

It use AbsoluteVote as parameters for all scheme , which I am not sure this is a right assumption for all cases.

Yes, adding new voting schemes...I would like to figure out how I can stay apprised of new contracts in Arc, and how they are intended to function...I already have some catching up to do.

It set parameters for contributionReward scheme

Check out utils.setParams -- each scheme grabs only the properties it requires.

We can encapsulate phase 4 to the setScheme transaction

In addition to calling scheme.registerOrganization, that final loop in organization.new also does:

   const fee  = initialSchemesFees[i];
   await token.transfer(org.avatar.address, fee);

I assume that this transfer would also be included in setScheme? (together with scheme.registerOrganization).

I wonder if the changes to "phase 4" could help with: https://github.com/daostack/arc-js/issues/9

[orenyodfat]

Check out utils.setParams -- each scheme grabs only the properties it requires.

Not all scheme use same parameters and the order is not forced in any way. Some scheme does not use even voting machines. I think that one possible solution will be to maintain a table which define the parameters needed for each scheme and the code should act accordingly. This issue should be moved to arc-js.

[dkent600]

@orenyodfat The current API doesn't impose any order of parameters nor which ones are used.

I can see that we may eventually want to refactor the config object passed into Organization.new, but it won't be because of utils.setParams.

[orenyodfat]

@dkent600 each scheme has its own SetParameters APIs. You should look on that.

[dkent600]

each scheme has its own SetParameters APIs. You should look on that.

@orenyodfat I have, take a look at how each scheme wrapper in arc-js is implementing utils.setParams()...

[orenyodfat]

Here https://github.com/daostack/arc-js/blob/automated-tests/lib/utils.js#L215 ?

[dkent600]

Yes, that is basically the abstract method, you can see it actually does nothing. Take a look at how each scheme wrapper in arc-js is implementing it

[leviadam]

Addressed in #208 .