(Feature) [xDai] - Use parametrized network values#1437
Conversation
|
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
[warning] @typescript-eslint/explicit-module-boundary-types
Report generated by eslint-plus-action |
|
Travis automatic deployment: |
…in the truffle artifact - also migrated safeMaster contract instance from truffle-contract to web3
|
Travis automatic deployment: |
| const openSafeProps = await createSafe(form, accounts[0]) | ||
|
|
||
| return openSafeProps.safeAddress | ||
| return createSafe(form, accounts[0]).then((receipt) => receipt.events?.ProxyCreation.returnValues.proxy) |
There was a problem hiding this comment.
why are you doing this? createSafe already returns the safeAddress. https://github.com/gnosis/safe-react/pull/1437/files#diff-b5f8a9922c89630abeb184e2ec4d2539R75
There was a problem hiding this comment.
Actually, originally was returning the safeAddress.
But then, somewhere in the middle createSafe return value was changed to return the promiEvent instead (probably because of https://github.com/gnosis/safe-react/blob/29d3b2ca3211d4d0252d61616bdab5206b6356b9/src/routes/open/container/Open.tsx#L134)
In fact, I'm not sure why the whole then block (https://github.com/gnosis/safe-react/blob/29d3b2ca3211d4d0252d61616bdab5206b6356b9/src/routes/open/container/Open.tsx#L70-L75) remained there, as it was useless.
What I wanted, was to fix the typing errors that arose after typing the contracts. If we all agree that the then block is useless, I can remove it.
There was a problem hiding this comment.
the promise is returning return { safeAddress: safeProps.address, safeTx: receipt } so in the then you should be able to get an object with {safeAddress and safeTx} isn't it?
There was a problem hiding this comment.
For that end, it should have been
return promiEvent.on(/*...*/).then(/*...*/).catch(/*...*/)instead of
promiEvent.on(/*...*/).then(/*...*/).catch(/*...*/)
return promiEventas it's now.
But if you do the first, you lose the promiEvent that is needed in https://github.com/gnosis/safe-react/blob/29d3b2ca3211d4d0252d61616bdab5206b6356b9/src/routes/opening/index.tsx#L171-L177
There was a problem hiding this comment.
make sense, feel free to remove the code in the then in Open.tsx
nicosampler
left a comment
nicosampler
left a comment
There was a problem hiding this comment.
@fernandomg
LGTM, just left a commnet!
|
Travis automatic deployment: |
|
Travis automatic deployment: |
|
The
|
|
Travis automatic deployment: |
| * @param {Web3} web3 | ||
| * @param {ETHEREUM_NETWORK} networkId | ||
| */ | ||
| const createGnosisSafeContract = (web3: Web3, networkId: ETHEREUM_NETWORK) => { |
There was a problem hiding this comment.
Should this be typed to return GnosisSafe ?
There was a problem hiding this comment.
the returned type is inferred in this case, shall we add it anyway? I truly don't remember.
There was a problem hiding this comment.
Yes you are right, I think it's not needed
| export const NODE_ENV = process.env.NODE_ENV | ||
| export const NETWORK = process.env.REACT_APP_NETWORK?.toUpperCase() || 'RINKEBY' | ||
| export const INTERCOM_ID = APP_ENV === 'production' ? process.env.REACT_APP_INTERCOM_ID : 'plssl1fl' | ||
| export const GOOGLE_ANALYTICS_ID = { |
There was a problem hiding this comment.
I think we already discussed this, but just to be sure, I think would be a good idea to create a ticket to discuss a better way to make this config scalable without us to edit this file every time we need to support another network on analytics
There was a problem hiding this comment.
Actually, yesterday I added a description in the setup/deployment/CI task on that regard: #1421 (comment)
| TREZOR: 'TREZOR', | ||
| } | ||
|
|
||
| export enum ExplorerTypes { |
There was a problem hiding this comment.
I would suggest doing a mix between the current implementation
export const getExplorerInfo = (hash: string): BlockScanInfo => {
const { name, url } = getNetworkExplorerInfo()
const blockScanInfo = () => {
const type = hash.length > 42 ? 'tx' : 'address'
return {
url: `${url}${type}/${hash}`,
alt: name || '',
}
}
return blockScanInfo
}
and deleted one.
I think is a good idea to define ExplorerTypes per network explorer. (etherscan uses /tx but blockscout could use /transaction instead)
Then we could define strategies per each type explorer (belonging to a network) and regarding the current network returning the link to the explorer
There was a problem hiding this comment.
Sorry @nicosampler, I followed the discussion where you started it, in private. Let's continue there and summarize here if needed.
src/config/index.ts
Outdated
| return blockScanInfo | ||
| } | ||
|
|
||
| // With some wallets from web3connect you have to use their provider instance only for signing |
There was a problem hiding this comment.
regarding your concern about moving this block of code to this file, a simple solution could be to move this to another file and export it as a function that receives the rpcUrl.
There was a problem hiding this comment.
perhaps, a better solution is to write it as a hook.
This reverts commit 10193e6.
|
Travis automatic deployment: |
5 participants
This PR closes #1416, by: