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

feature/offerable identity #269

Merged
merged 17 commits into from
Mar 22, 2021
Merged

Conversation

JGiter
Copy link
Collaborator

@JGiter JGiter commented Mar 4, 2021

| Jira issue | https://energyweb.atlassian.net/browse/MYEN-694 |

ProxyIdentity redesigned as OfferableIdentity, which allows to offer identity with possibility to accept or reject it. For use cases check tests in the proxyidentity package

Design decisions:

  • Identity owner doesn't control identity document directly. Identity owns its document
  • Identity owner can offer identity by OfferableIdentity.offer
  • Owned identity can be operated by OfferableIdentityOperator which extends Operator and therefore can be used in claims workflow. Some Operators methods are excluded:
    • changeOwner - to prevent identity transfer without acknowledgment
    • revokeDelegate - notion of delegate is not defined on offerable identities

@JGiter JGiter force-pushed the feature/MYEN-694_asset_contract branch from 26bb044 to e47d167 Compare March 5, 2021 11:27
@JGiter JGiter changed the title feature/proxy identity feature/offerable identity Mar 5, 2021
@JGiter JGiter force-pushed the feature/MYEN-694_asset_contract branch 2 times, most recently from 44323de to 30ec13b Compare March 5, 2021 13:38
@JGiter JGiter requested review from ni3gavhane, dwojno and jrhender March 5, 2021 13:39
@jrhender
Copy link
Collaborator

jrhender commented Mar 8, 2021

Looks good to me so far 😃

@JGiter JGiter force-pushed the feature/MYEN-694_asset_contract branch from a4426ba to 5dd021b Compare March 10, 2021 08:40
@JGiter JGiter force-pushed the feature/MYEN-694_asset_contract branch from 5dd021b to 2cf17f5 Compare March 11, 2021 09:05
@JGiter JGiter force-pushed the feature/MYEN-694_asset_contract branch 2 times, most recently from 669c0b7 to cb631cf Compare March 12, 2021 12:35
@JGiter JGiter requested a review from manihagh March 12, 2021 12:35
@JGiter JGiter added the enhancement New feature or request label Mar 12, 2021
@JGiter JGiter self-assigned this Mar 12, 2021
@JGiter JGiter force-pushed the feature/MYEN-694_asset_contract branch from cb631cf to 55e21e6 Compare March 12, 2021 12:44
@JGiter JGiter marked this pull request as ready for review March 12, 2021 13:44
Copy link
Collaborator

@jrhender jrhender left a comment

Choose a reason for hiding this comment

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

Looks good, just a few questions

offeredTo = address(0);
}

function sendTransaction(bytes memory _data, address to, uint256 value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this function need the isOwner modifier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If tranasaction sent to ERC1056, then ownership will be checked by ERC1056, but someone who is not the owner might use this method to send tx from asset to some arbitrary contract what potentially could be dangerous. That is why I added this additional safe guarantee.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add isOwner modifier

pragma solidity ^0.5.0;


contract IdentityManager {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this contract is just doing the events, maybe it's more specific to call it IdentityEvents or maybe IdentityNotifier

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I plan to expand this contract in the future, such as adding methods to return a list of assets or may be batch asset creation

@@ -0,0 +1,45 @@
pragma solidity ^0.5.0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why use the 0.5 compiler instead of a higher version (for both this contract and OfferableIdentity)?
I'm not saying it should be higher, I'm not too sure. But it maybe a higher version would mean a contract inheriting this contract could use new features?

Copy link
Collaborator Author

@JGiter JGiter Mar 16, 2021

Choose a reason for hiding this comment

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

Good idea. Upgraded to 0.8.0

@JGiter JGiter requested a review from jrhender March 16, 2021 08:05
offeredTo = address(0);
}

function sendTransaction(bytes memory _data, address to, uint256 value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add isOwner modifier

@JGiter JGiter requested a review from jrhender March 16, 2021 14:39
Copy link
Collaborator

@jrhender jrhender left a comment

Choose a reason for hiding this comment

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

Looks good

@jrhender
Copy link
Collaborator

@JGiter I was taking a look at ERC725X and wondering if maybe OfferableIdentity should be able to create contracts as well. https://github.com/ERC725Alliance/ERC725/blob/master/implementations/contracts/ERC725/ERC725X.sol Maybe it's overkill but just thought I would throw it out there. What do you think?

@JGiter
Copy link
Collaborator Author

JGiter commented Mar 19, 2021

@JGiter I was taking a look at ERC725X and wondering if maybe OfferableIdentity should be able to create contracts as well. https://github.com/ERC725Alliance/ERC725/blob/master/implementations/contracts/ERC725/ERC725X.sol Maybe it's overkill but just thought I would throw it out there. What do you think?

May be active assets should be able to do this. If so then it make sense to add such feature in separate ActiveIdentity contract, which extends OfferableIdentity

@JGiter JGiter force-pushed the feature/MYEN-694_asset_contract branch from 6f5d78f to 3826a35 Compare March 22, 2021 06:54
@JGiter JGiter merged commit 35104c8 into development Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants