-
Notifications
You must be signed in to change notification settings - Fork 90
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
Block explorer selection #2579
base: master
Are you sure you want to change the base?
Block explorer selection #2579
Conversation
07e315a
to
9afd818
Compare
if (account) { | ||
if (backend[account.coinCode]) { | ||
setBlockExplorerTxPrefix(backend[account.coinCode].blockExplorerTxPrefix); | ||
} else { |
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 is redundant as we check blockExplorerTxPrefix
undefined on line 337. Should I remove the else branch ?
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.
thanks!
partial review, couldn't finish yet.
backend/backend.go
Outdated
// Block explorers. | ||
// Block explorers (btc). | ||
"https://blockstream.info/tx/", | ||
"https://blockstream.info/testnet/tx/", | ||
"https://mempool.space/tx/", | ||
"https://mempool.space/testnet/tx/", | ||
// Block explorers (ltc). | ||
"https://sochain.com/tx/LTCTEST/", | ||
"https://blockchair.com/litecoin/transaction/", | ||
// Block explorers (eth). | ||
"https://etherscan.io/tx/", | ||
"https://goerli.etherscan.io/tx/", | ||
"https://sepolia.etherscan.io/tx/", | ||
"https://ethplorer.io/tx/", | ||
"https://goerli.ethplorer.io/tx/", | ||
"https://sepolia.ethplorer.io/tx/", |
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.
instead of adding them all here, it's better to add another loop in SystemOpen() that goes through all explorers, so that one only has to modify the list of explorers when making changes (adding or removing explorers).
backend/config/blockexplorer.go
Outdated
// BlockExplorer defines a selectable block explorer. | ||
type BlockExplorer struct { | ||
Name string `json:"name"` | ||
Url string `json:"url"` |
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.
document the fields as well. Especially that the Url is a prefix and the txID will be appended.
backend/config/config.go
Outdated
@@ -59,6 +60,7 @@ const ( | |||
// ethCoinConfig holds configurations for ethereum coins. | |||
type ethCoinConfig struct { | |||
DeprecatedActiveERC20Tokens []string `json:"activeERC20Tokens"` | |||
BlockExplorerTxPrefix string `json:"blockExplorerTxPrefix"` |
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.
i wonder if it's maybe better to add a separate field to the backend config blockExplorers
, so it's all concentrated in one location 🤔
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.
also the name should maybe not mention prefix
, in case there are (or will be) explorers where the txID is not simply appended
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.
that makes sense.
@@ -1013,6 +1013,27 @@ | |||
"title": "Why is there a network fee?" | |||
} | |||
}, | |||
"settings-block-explorer": { |
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.
settingsBlockExplorer, all entries should be camelCase (most are, but there are some misses like right below with settings-electrum 🙈 )
"title": "How do I use a block explorer?" | ||
}, | ||
"options": { | ||
"text": "The BitBoxApp features a protection mechanism designed to prevent the opening of arbitrary links. This serves as a security measure against malicious links. It cannot be bypassed on an individual basis; rather, its suspension would be required to allow users to configure their own URLs.", |
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.
imo this is super confusing to users, especially the last part. i'd stop at the first part
"The BitBoxApp features a protection mechanism designed to prevent the opening of arbitrary links. This serves as a security measure against malicious links."
useEffect(() => { | ||
const fetchData = async () => { | ||
const coins = await backendAPI.getSupportedCoins(); | ||
const allExplorerSelection = await apiGet('available-explorers'); |
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.
make an API call wrapper in api/backend.ts, and a definition of the return type.
563d584
to
fc70f0a
Compare
fc70f0a
to
f45d117
Compare
addressed your comments @benma , please resolve the threads if satisfied. also noticed I can remove |
f45d117
to
ad701cd
Compare
I added another commit that uses accounts instead of supported-coins, but now there will not be any changes/additions anymore. |
516f8f1
to
717687f
Compare
717687f
to
2368734
Compare
force pushed rebase for clean diff |
2368734
to
6e127f7
Compare
Please let me know if I should add something to the |
aace6ba
to
1302c90
Compare
4b61268
to
3a0085e
Compare
0beb449
to
982a1a2
Compare
d65c13d
to
9cabcd3
Compare
9cabcd3
to
239b7fa
Compare
Add a setting that lets users configure their preferred block explorer. Only whitelisted block explorers are available options. The frontend decides which selections to show (e.g only BTC or BTC and ETH) based on the accounts that are present.
Add the block explorer prefix url to the configuration that will be used to open the transaction. The available options to select are statically defined and the frontend can learn them by calling the available-explorers endpoint for which a handler was implemented in this commit.
239b7fa
to
bedadc8
Compare
Allow users to configure their preferred block explorer (from predefined selection) in the settings.
Block explorers need to be added to the
AvailableExplorers
struct and whitelisted in the backend. The frontend learns all available options by calling the/available-explorers
endpoint. Adding options will only need a change in the backend the frontend will always show all available selections.notes:
AvailableExplorers
of course we could also implement an interface instead that each coin can implement, this would be more elegant but I did not see the need for that.initialConfig
in a different place than theconfig
because it would always update alongside config if they were initialized in the same placeloadConfig
see here. I don't understand why it behaves like this, I just fetch the config again to initializeinitialConfig
maybe there is a better way avoiding two get requests...Right now the frontend uses the
/supported-coins
endpoint to decide how many/which selection to show in the setting. This requires a keystore to be connected which is weird UX. We can fix this later by introducing a separate check for a multicoin, or bitcoin only user/wallet in the config.Please let me know if you have suggestions for more block explorers, I just added the most popular. In case they deviate from
host.com/tx/xxx
(e.g.host.com?tx=xx
) there needs to be more refactoring in the backend, maybe then it would make sense to define an interface rather than a static stuct for theAvailableExplorers
.