-
Notifications
You must be signed in to change notification settings - Fork 459
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
feat: create addNetwork request method on sdk #793
Conversation
b56cfd4
to
b68d44e
Compare
Coverage report for
|
St.❔ |
Category | Percentage | Covered / Total |
---|---|---|---|
🟡 | Statements | 69.4% | 3693/5321 |
🔴 | Branches | 47.14% | 685/1453 |
🔴 | Functions | 53.94% | 800/1483 |
🟡 | Lines | 70% | 3556/5080 |
Test suite run success
339 tests passing in 74 suites.
Report generated by 🧪jest coverage report action from b9b0b30
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.
nice work @eswarasai , few changes but overall looking pretty solid
packages/app/src/systems/DApp/machines/addNetworkRequestMachine.tsx
Outdated
Show resolved
Hide resolved
f4f6911
to
3a52bd8
Compare
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.
- When adding a URL that already exists the SDK should throw an error before opening the UI for the user. This validation needs to be on the same level of url format.
/* store.useUpdateMachineConfig(Services.txRequest, { | ||
actions: { | ||
notifyRefreshNetworks () { | ||
store.refreshNetworks(); | ||
}, | ||
notifyUpdateAccounts () { | ||
store.updateAccounts(); | ||
}, | ||
}, | ||
}); */ |
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.
You can remove this code?
useEffect(() => { | ||
store.refreshNetworks(); | ||
}, []); |
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.
Why we need this refresh here?
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.
Just to ensure that the networks
in the store
are being refreshed and up-to-date with the data stored in the IndexedDB
cc: @LuizAsFight
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.
@LuizAsFight do we have some cache on the network list? If not I don't see why it would be needed.
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.
@luizstacio
this is just to make sure that when network list opens, network list will be correct
as network machine is global, if some other machine forget to call notifyRefreshNetworks
after change networks, the list will be incorrect.
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 don't think we should cover forgetting notifyRefreshNetworks
. This needs to be called for an application to run correctly. And as we don't have caches every time the application is open, it should refresh.
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 don't see what's the problem, a refetch when opening a list is a pretty common behavior
packages/app/src/systems/Network/components/NetworkForm/NetworkForm.tsx
Outdated
Show resolved
Hide resolved
packages/app/src/systems/CRX/background/services/BackgroundService.ts
Outdated
Show resolved
Hide resolved
packages/app/src/systems/DApp/machines/addNetworkRequestMachine.tsx
Outdated
Show resolved
Hide resolved
@@ -45,6 +45,13 @@ Return the current account being used in the wallet application. | |||
testCase="currentAccount" | |||
/> | |||
|
|||
### Add Network |
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.
should also add List Networks, and Current Network
006631b
to
db64e19
Compare
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.
lgtm 🏖️
9fabd41
to
b9b0b30
Compare
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to master, this PR will be updated. # Releases ## @fuel-wallet/sdk@0.11.0 ### Minor Changes - [#806](#806) [`e94d273`](e94d273) Thanks [@mpoplavkov](https://github.com/mpoplavkov)! - fix: default connector Fuel Wallet - [#793](#793) [`7e7300f`](7e7300f) Thanks [@eswarasai](https://github.com/eswarasai)! - Implemented `addNetwork()` method on Fuel SDK enabling developers to add network - [#764](#764) [`144d722`](144d722) Thanks [@eswarasai](https://github.com/eswarasai)! - Enabled FuelWallet SDK to be use directly without the need to wait for the injected script - [#759](#759) [`f79897d`](f79897d) Thanks [@tomiiide](https://github.com/tomiiide)! - Handle all errors in the wallet ### Patch Changes - [#686](#686) [`98c8184`](98c8184) Thanks [@LuizAsFight](https://github.com/LuizAsFight)! - feat: add sdk.addAbi to add custom ABIs for contracts - [#814](#814) [`1cdb147`](1cdb147) Thanks [@luizstacio](https://github.com/luizstacio)! - Fix SDK to enable been used in standalone mode by adding uuid on requests ## @fuel-wallet/types@0.11.0 ### Minor Changes - [#759](#759) [`f79897d`](f79897d) Thanks [@tomiiide](https://github.com/tomiiide)! - Handle all errors in the wallet ### Patch Changes - [#686](#686) [`98c8184`](98c8184) Thanks [@LuizAsFight](https://github.com/LuizAsFight)! - feat: add support to show called methods/params passed to contract calls inside Transaction Screen ## fuels-wallet@0.11.0 ### Minor Changes - [#801](#801) [`992d5e7`](992d5e7) Thanks [@eswarasai](https://github.com/eswarasai)! - Added support for user to connect multiple accounts, even if current account is connected - [#793](#793) [`7e7300f`](7e7300f) Thanks [@eswarasai](https://github.com/eswarasai)! - Implemented `addNetwork()` method on Fuel SDK enabling developers to add network ### Patch Changes - [#813](#813) [`3910e87`](3910e87) Thanks [@luizstacio](https://github.com/luizstacio)! - Fix validation on Asset Screen to allow empty image url field - [#802](#802) [`3b42a3d`](3b42a3d) Thanks [@eswarasai](https://github.com/eswarasai)! - Fixed the broken copy transaction link in send transaction screen - [#794](#794) [`3122ccd`](3122ccd) Thanks [@LuizAsFight](https://github.com/LuizAsFight)! - Don't allow connecting hidden accounts - [#792](#792) [`5b6b062`](5b6b062) Thanks [@luizstacio](https://github.com/luizstacio)! - Change dependencies from @fuel-ts/\* to fuels - [#686](#686) [`98c8184`](98c8184) Thanks [@LuizAsFight](https://github.com/LuizAsFight)! - feat: add support to show called methods/params passed to contract calls inside Transaction Screen - Updated dependencies \[[`98c8184`](98c8184), [`e94d273`](e94d273), [`7e7300f`](7e7300f), [`1cdb147`](1cdb147), [`144d722`](144d722), [`f79897d`](f79897d), [`98c8184`](98c8184)]: - @fuel-wallet/sdk@0.11.0 - @fuel-wallet/types@0.11.0 Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
FRO-84
Fixes: #389
Description
Implemented
addNetwork()
method on Fuel SDK enabling developers to add networkTODO
AddNetworkRequest
screen