search

gnosis / safe-android-kouban

2 stars 2 forks source link

Show deployment tx on etherscan #72

Closed elgatovital closed 4 years ago

elgatovital commented 4 years ago

Closes #67

jpalvarezl commented 4 years ago

Shouldn't we resolve one issue per task, like you mentioned some time ago?

elgatovital commented 4 years ago

Shouldn't we resolve one issue per task, like you mentioned some time ago?

yes. But I still think having links in data config is more clear. This way they are in one place and not scattered across project

jpalvarezl commented 4 years ago

It is a value that doesn't affect the build imo, it's just an asset that serves the purpose of navigating outside the app. I don't think that should be part of the build config. It is the same with the colour of the background for the splash screen. It's just an asset.

elgatovital commented 4 years ago

It is a value that doesn't affect the build imo, it's just an asset that serves the purpose of navigating outside the app. I don't think that should be part of the build config. It is the same with the colour of the background for the splash screen. It's just an asset.

it is a blockchain related configuration. Otherwise we should also move all our mastercopies, fallback handlers and blockchain related build configs to assets.

jpalvarezl commented 4 years ago

We are going to have to agree to disagree, I think. That's the very reason as to why you can have different source sets. I always thought that the reason as to why these values were injected in gradle was because they contained secrets or were necessary by the build machine; not because they were related to the blockchain in any way.

rmeissner commented 4 years ago

My opinion:

I think both are valid, depends purely on your preference. BuildConfig is easier to overwrite in the CI pipeline, but if you have many of these the build file gets cluttered.

For handling multiple issues in one PR, I think for small bug fixes that is fine, but again depends on what the team agreed on ;)

elgatovital commented 4 years ago

We are going to have to agree to disagree, I think. That's the very reason as to why you can have different source sets. I always thought that the reason as to why these values were injected in gradle was because they contained secrets or were necessary by the build machine; not because they were related to the blockchain in any way.

I agree with this point. Currently, all of our links are in data config. We can decide later whether we will make assets out of them. I am simply following the current approach taken in the project.

jpalvarezl commented 4 years ago

If we are proceeding with this approach for the ethscan URL, the only thing I would like to point out is that the URLs themselves are not part of the change list in the PR, so I can't verify their value. What's the reason for that?

rmeissner commented 4 years ago

They should be, right?

elgatovital commented 4 years ago

we already had them in data build config mainnet buildConfigField javaTypes.STRING, "BLOCK_EXPLORER_TX",asString(getKey("BLOCK_EXPLORER_TX", "https://etherscan.io/tx/%s"))

and for rinkeby buildConfigField javaTypes.STRING, "BLOCK_EXPLORER_TX", asString(getKey("BLOCK_EXPLORER_TX", "https://rinkeby.etherscan.io/tx/%s"))