Skip to content

DAOTracker Support #380

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

Merged
merged 40 commits into from
Nov 12, 2019
Merged

DAOTracker Support #380

merged 40 commits into from
Nov 12, 2019

Conversation

dOrgJelli
Copy link
Contributor

@dOrgJelli dOrgJelli commented Nov 1, 2019

No description provided.

@dOrgJelli dOrgJelli changed the title [WIP] DAOTracker Support DAOTracker Support Nov 4, 2019
@dOrgJelli
Copy link
Contributor Author

dOrgJelli commented Nov 4, 2019

@orenyodfat tests are passing. Also I believe I've resolved all comments, please verify.

@dOrgJelli
Copy link
Contributor Author

dOrgJelli commented Nov 12, 2019

@orenyodfat the new DAOTracker contract that contains the arcVersion is integrated. Here's an overview of the new changes:

  • Added the ops/templates.json file to keep track of what ABI versions should be used with what mappings.
  • Updated generate-contractsInfos.js to...
    setTemplateInfo('ContractName', 'ArcVersion', 'UniqueTemplateName');
    for each templatable ABI. setTemplateInfo(...) is defined in src/utils.ts.
    ex: setTemplateInfo('Avatar', '0.0.1-rc.16', 'Avatar_0_0_1_rc_16');
  • Updated generate-subgraph.js to add all possible datasource templates to the subgraph manifest.
  • Updated DAOTracker/mapping.ts to use the new way of instantiating templates.

Open question:

  • In DAOTracker/mapping.ts, at line # 70, how do you think we should handle the error case where a template isn't found? Should we just fail silently and return from the handler? This essentially means (assuming correct on-chain data) the subgraph is out of date and is missing ABIs + mappings. I propose we add a new entity to the store when this happens, and our automation looks for this entity existing to alerts the team letting them know what's going wrong.

Comment on lines +34 to +40
let avatar = event.params._avatar;
let controller = event.params._controller;
let reputation = event.params._reputation;
let daoToken = event.params._daoToken;
let sender = event.params._sender;
let arcVersion = event.params._arcVersion;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these local vars are redundant here.

arcVersion = daoCreatorInfo.version;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to confirm (somehow) that creating a template for wrong contract does not crash the subgraph .
this can happened only if the event does not come from daocreator.
if we cannot confirm ,so lets consider limit this feature only for daocreator.

Copy link
Contributor Author

@dOrgJelli dOrgJelli Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subgraph does not (as far as I know, looking into this) check to make sure the contract bytecode matches the bytecode in the ABI file. This means that it only watches for events ABIs from a given contract address, and if they do not emit from that address the subgraph will not do anything.

So in the case you're describing, the subgraph would not crash when it creates the template, it would rather just do nothing from there on out. The only case where this could happen is someone wrote custom contracts that emit the events we're looking for, and the subgraph responds to those events from these newly created contracts. The plan here, is that if DAOstack detects a "DAO" is doing this, it would blacklist it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing a test to verify the subgraph doesn't crash.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as far as I see the only way to mitigate that and stay on the safe side is to track this event only if it come via daocreator. otherwise ignore it.

@orenyodfat
Copy link
Contributor

@orenyodfat the new DAOTracker contract that contains the arcVersion is integrated. Here's an overview of the new changes:

  • Added the ops/templates.json file to keep track of what ABI versions should be used with what mappings.
  • Updated generate-contractsInfos.js to...
    setTemplateInfo('ContractName', 'ArcVersion', 'UniqueTemplateName');
    for each templatable ABI. setTemplateInfo(...) is defined in src/utils.ts.
    ex: setTemplateInfo('Avatar', '0.0.1-rc.16', 'Avatar_0_0_1_rc_16');
  • Updated generate-subgraph.js to add all possible datasource templates to the subgraph manifest.
  • Updated DAOTracker/mapping.ts to use the new way of instantiating templates.

Open question:

  • In DAOTracker/mapping.ts, at line # 70, how do you think we should handle the error case where a template isn't found? Should we just fail silently and return from the handler? This essentially means (assuming correct on-chain data) the subgraph is out of date and is missing ABIs + mappings. I propose we add a new entity to the store when this happens, and our automation looks for this entity existing to alerts the team letting them know what's going wrong.

this can happened only when the event does not come from daocreator. right ? lets consider supporting this feature only if it come from daocreator.

@dOrgJelli
Copy link
Contributor Author

@orenyodfat yes this only happens when it comes not from the DAOcreator. I agree that we could limit this to just the DAOcreator for now, and the work done here would still be reusable for when the factories are created.

@dOrgJelli
Copy link
Contributor Author

@orenyodfat The only DAOs that will be tracked are ones created by the DaoCreator. Additionally the embedded blacklist is implemented in the ops/blacklist.json file.

@orenyodfat orenyodfat merged commit e2a6d9b into daostack:master Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants