feat(network): integrate new rpc providers api #17110
Conversation
dlipicar
requested review from
a team,
micieslak,
caybro and
alexjba
as code owners
Jenkins BuildsClick to see older builds (21)
|
| return 0 | ||
| return self.delegate.getFlatNetworksList()[self.index].rpcProviders.len | ||
|
|
||
| proc countChanged(self: RpcProvidersModel) {.signal.} |
There was a problem hiding this comment.
count property has proven to be quite problematic. When used in QML we're assuming the nim model will be used in all cases. But that's not true. We can easily need at some point to add a proxy on top and any implementation using count will be broken.
If the model count is needed in qml we can use model.ModelCount.count (ui/StatusQ/include/StatusQ/modelcount.h)
There was a problem hiding this comment.
oh this is actually not getting used anywhere, shame on me for copypasting. Will remove.
|
|
||
| QtObject: | ||
| type | ||
| Model* = ref object of QAbstractListModel | ||
| delegate: io_interface.NetworksDataSource | ||
| rpcProvidersPerNetwork: seq[RpcProvidersModel] |
There was a problem hiding this comment.
Have you considered detaching the nested model from the networks model?
I think it's worth considering a flat RPCProvidersModel outside of the scope of networks model.
When needed in qml we can get the data using a SFPM, filtering per chain. But in most cases it's probably not needed at all.
The nested models are ok if it's used for a single view structure. This means to give up flexibility in favor of convenience. But since there's less flexibility, when the data is needed in another context it's really expensive to process the data.
There was a problem hiding this comment.
can do! I don't think this is gonna be used in more than 1 place (the network config screen), but it's true that I had to do some workaroundy stuff due to the usage of nested models.
| Global.openTestnetPopup() | ||
| checked: root.areTestNetworksEnabled | ||
| onToggled:{ | ||
| checked = Qt.binding(() => root.areTestNetworksEnabled) |
There was a problem hiding this comment.
Hmm, you shouldn't be setting the checked property from the toggled handler 🤔
There was a problem hiding this comment.
hmm the binding should break when the user clicks the button, and we're not sure which value root.areTestNetworksEnabled will end up having (since we open a popup letting the user proceed or cancel the toggle, or something could fail when making the backend call to change this setting). Where would you suggest I put this logic instead?
I guess what we really want is a "Detached Switch" that does not actually change its checked state when the user clicks it (it just emits some event), and has its state set externally instead.
dlipicar
force-pushed
the
feat/integrate-new-rpc-providers-api
branch
3 times, most recently
from
d6b036c to
81b808a
Compare
dlipicar
force-pushed
the
feat/integrate-new-rpc-providers-api
branch
from
81b808a to
2f3d731
Compare
What does the PR do
Integrate new RPC Providers API from status-im/status-go#6178
Instead of main/fallback URLs, we now have a list of RPCProviders.
The user cannot
replacethe default providers, they can only define their own, which get used first.For this reason,
Revert to default(which brought back the default provider urls) no longer makes sense, so the option was removed.No functional changes for now, designs for new User RPC Provider configuration UI in progress (see https://www.figma.com/design/idUoxN7OIW2Jpp3PMJ1Rl8/%E2%9A%99%EF%B8%8F-Settings-%7C-Desktop?node-id=25452-240238&t=XlKqIWmeeh2smJTT-0), for this reason I tried to touch as little as possible from the UI elements.
Took the chance to tidy up the surrounding code a little bit.
Affected areas
Settings->Wallet->Network
Architecture compliance
My PR is consistent with this document: Status Desktop Architecture Guide
Screenshot of functionality (including design for comparison)