Skip to content
This repository was archived by the owner on Nov 10, 2023. It is now read-only.

feat: Add getCommunicationInfo method and new prop to SafeInfo #3657

Merged
yagopv merged 3 commits into from
Mar 15, 2022
Merged

feat: Add getCommunicationInfo method and new prop to SafeInfo #3657

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d007f10
Add method support
yagopv Mar 11, 2022
50712cd
Rename getCommunicationInfo -> getEnvironmentInfo
yagopv Mar 14, 2022
38764ae
Upgrade Apps SDK
yagopv Mar 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@
},
"dependencies": {
"@ethersproject/hash": "^5.5.0",
"@gnosis.pm/safe-apps-sdk": "6.2.0",
"@gnosis.pm/safe-apps-sdk": "7.3.0",
"@gnosis.pm/safe-apps-sdk-v1": "npm:@gnosis.pm/safe-apps-sdk@0.4.2",
"@gnosis.pm/safe-core-sdk": "^2.0.0",
"@gnosis.pm/safe-deployments": "^1.8.0",
Expand Down
11 changes: 11 additions & 0 deletions src/routes/safe/components/Apps/components/AppFrame.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import { SignMessageModal } from './SignMessageModal'
import { web3HttpProviderOptions } from 'src/logic/wallets/getWeb3'
import { useThirdPartyCookies } from '../hooks/useThirdPartyCookies'
import { ThirdPartyCookiesWarning } from './ThirdPartyCookiesWarning'
import { grantedSelector } from 'src/routes/safe/container/selector'

const AppWrapper = styled.div`
display: flex;
Expand Down Expand Up @@ -87,6 +88,7 @@ const AppFrame = ({ appUrl }: Props): ReactElement => {
const { address: safeAddress, ethBalance, owners, threshold } = useSelector(currentSafe)
const { nativeCurrency, chainId, chainName, shortName } = getChainInfo()
const safeName = useSelector((state) => addressBookEntryName(state, { address: safeAddress }))
const granted = useSelector(grantedSelector)
const { trackEvent } = useAnalytics()
const iframeRef = useRef<HTMLIFrameElement>(null)
const [confirmTransactionModal, setConfirmTransactionModal] =
Expand Down Expand Up @@ -174,6 +176,9 @@ const AppFrame = ({ appUrl }: Props): ReactElement => {
const communicator = useAppCommunicator(iframeRef, safeApp)

useEffect(() => {
/**
* @deprecated: getEnvInfo is a legacy method. Should not be used
*/
communicator?.on('getEnvInfo', () => ({
txServiceUrl: getTxServiceUrl(),
}))
Expand All @@ -186,6 +191,10 @@ const AppFrame = ({ appUrl }: Props): ReactElement => {
return tx
})

communicator?.on(Methods.getEnvironmentInfo, async () => ({
origin: document.location.origin,
}))

communicator?.on(Methods.getSafeInfo, () => ({
safeAddress,
// FIXME `network` is deprecated. we should find how many apps are still using it
Expand All @@ -194,6 +203,7 @@ const AppFrame = ({ appUrl }: Props): ReactElement => {
chainId: parseInt(chainId, 10),
owners,
threshold,
isReadOnly: !granted,
Copy link
Member

@katspaugh katspaugh Mar 11, 2022 •
edited
Loading

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?

Copy link
Member Author

@yagopv yagopv Mar 11, 2022 •
edited
Loading

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?

Copy link
Contributor

@mmv08 mmv08 Mar 12, 2022

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

Copy link
Member Author

@yagopv yagopv Mar 14, 2022

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

Copy link
Contributor

@iamacook iamacook Mar 14, 2022

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'.

Copy link
Collaborator

@dasanra dasanra Mar 14, 2022

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

Copy link
Contributor

@mmv08 mmv08 Mar 14, 2022

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

Copy link
Collaborator

@dasanra dasanra Mar 14, 2022 •
edited
Loading

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

}))

communicator?.on(Methods.getSafeBalances, async (msg) => {
Expand Down Expand Up @@ -263,6 +273,7 @@ const AppFrame = ({ appUrl }: Props): ReactElement => {
chainName,
shortName,
safeAppWeb3Provider,
granted,
])

const onUserTxConfirm = (safeTxHash: string, requestId: RequestId) => {
Expand Down
10 changes: 9 additions & 1 deletion yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1922,6 +1922,14 @@
"@gnosis.pm/safe-react-gateway-sdk" "^2.5.6"
ethers "^5.4.7"

"@gnosis.pm/safe-apps-sdk@7.3.0":
version "7.3.0"
resolved "https://registry.yarnpkg.com/@gnosis.pm/safe-apps-sdk/-/safe-apps-sdk-7.3.0.tgz#3a3ed38b75657a7d5cd58d5a6884ec60087b05a8"
integrity sha512-1f6VHJWqPRKAEg/m+fbO1XiaDrsTcI3PQg31A0ciHATlVVoh35BYXryijaQxXblLzz4eDgbbIXQdNAH683j87Q==
dependencies:
"@gnosis.pm/safe-react-gateway-sdk" "^2.10.0"
ethers "^5.4.7"

"@gnosis.pm/safe-apps-sdk@^6.2.0":
version "6.3.0"
resolved "https://registry.yarnpkg.com/@gnosis.pm/safe-apps-sdk/-/safe-apps-sdk-6.3.0.tgz#19f8bff136bdfdf9003745e4202e1cb85322e493"
Expand Down Expand Up @@ -1965,7 +1973,7 @@
react-media "^1.10.0"
web3-utils "^1.6.0"

"@gnosis.pm/safe-react-gateway-sdk@^2.10.1":
"@gnosis.pm/safe-react-gateway-sdk@^2.10.0", "@gnosis.pm/safe-react-gateway-sdk@^2.10.1":
version "2.10.1"
resolved "https://registry.yarnpkg.com/@gnosis.pm/safe-react-gateway-sdk/-/safe-react-gateway-sdk-2.10.1.tgz#62f4abf733855e734aa1eab4be4778ccd08fe689"
integrity sha512-uIosTEqmoxhCy7WS8sIzXde2nJQwzC+KfNoeDQVeLZtEpnRZQ7R+N/qDtORMUJfKeyc8cIwkKXmVc2DRgSRxOQ==
Expand Down