-
Notifications
You must be signed in to change notification settings - Fork 360
feat: Add getCommunicationInfo method and new prop to SafeInfo #3657
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
Pull Request Test Coverage Report for Build 1981898496
💛 - Coveralls |
iamacook
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
| chainId: parseInt(chainId, 10), | ||
| owners, | ||
| threshold, | ||
| isReadOnly: !granted, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-owners can still execute txns, so it's not 100% read-only.
Maybe isOwnedByMe: granted would be more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mmm could be, naming came from a conversation with Dani. WDYT @dasanra?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apps rather propose transactions than execute them, the nonowner cannot propose a transaction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what's the better option ? For me if nonowners cannot propose transactions isReadOnly could be fine as well in the safe-apps context
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This matches the Safe. We always show the read only warning if a user is not 'granted'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this isReadOnly is inspired as the Safe in the interface is shown as "Read Only" mode. The other name that could fit would be isOwnerConnected as we are trying to allow the apps to know if the owner is connected, so they can distinguish if that transaction could only be simulated or sent to be executed, either at that moment or later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the interface ever starts supporting delegates, isOwnerConnected may become non-relevant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, yes in this case the most relevant is to know if transaction could be sent or not, so isReadOnly should be good
What it solves
Resolves safe-global/safe-apps-sdk#301, resolves safe-global/safe-react-apps#374
How this PR fixes it
We are doing 2 different things here:
Adding a new method
getCommunicationInfo()used by returning some info about the communicator. By now we are returning the origin as we need it in the walletConnect app and is difficult to get it inside an iframe (Current approachancestorOriginsis not supported on FirefoxWe are including a new property
isReadOnlyon the SafeInfo object allowing as to know safe apps if the safe-web is on READ-ONLY mode