- 
        Couldn't load subscription status. 
- Fork 109
Block explorer selection #2579
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
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
          
        
      | // 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.
| "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
AvailableExplorersstruct and whitelisted in the backend. The frontend learns all available options by calling the/available-explorersendpoint. Adding options will only need a change in the backend the frontend will always show all available selections.notes:
AvailableExplorersof 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.initialConfigin a different place than theconfigbecause it would always update alongside config if they were initialized in the same placeloadConfigsee here. I don't understand why it behaves like this, I just fetch the config again to initializeinitialConfigmaybe there is a better way avoiding two get requests...Right now the frontend uses the
/supported-coinsendpoint 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.