Skip to content
This repository has been archived by the owner on Jul 22, 2024. It is now read-only.

refactor: decouple capi from presentation layer #223

Merged
merged 11 commits into from
Jul 5, 2023

Conversation

statictype
Copy link
Contributor

@statictype statictype commented Jul 1, 2023

This PR just removes capi from the preact components as first step in a larger refactoring.

coming up
#225
#224

@statictype statictype marked this pull request as draft July 1, 2023 11:59
@netlify
Copy link

netlify bot commented Jul 1, 2023

Deploy Preview for capi-multisig ready!

Name Link
🔨 Latest commit 668d1ef
🔍 Latest deploy log https://app.netlify.com/sites/capi-multisig/deploys/64a4bc4001855c000835eb0c
😎 Deploy Preview https://deploy-preview-223--capi-multisig.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@statictype statictype marked this pull request as ready for review July 4, 2023 18:39
@statictype statictype requested review from peetzweg and josepot July 4, 2023 21:00
Copy link
Contributor

@josepot josepot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much @statictype for doing this.

I'm also of the opinion that it's best to keep the presentation layer as decoupled from CAPI as possible. As they saying goes: "good fences make good neighbors" 🙁.

As I've mentioned offline: I'm still not completely sure that these functions that take a callback (for the intermediate events) and return a Promise with the final value are the best way to go. However, I do think that this is much better than having no separation at all, and it's quite pragmatic.

@peetzweg
Copy link
Member

peetzweg commented Jul 5, 2023

LGTM.

One thing which just comes to mind is that this way we are not exposing the Runes to the caller of these cancel(), ratify() etc function as we are just calling .run() on the already in the creation.

We might not need the actual Rune of the flow of these extrinsics but doing this already prevents the creation of a ExtrinsicRune only and doing some different stuff with it as we intended to do with the OrthoRunes. 🤔

paritytech/capi#1090

So maybe this handling of the notification will probably look different to these callback stuff once we leverage the abilty of an OrthoRune as described here: paritytech/capi#1090 .

In my mental model decoupling the notification stuff from the actual creation of the ExtrinsicRune feels nicer than doing this callback message type approach. Maybe we still do the notification handling in these function but with the OrthoRunes but than it's hard to disable the notification from the outside. 🤷

https://github.com/paritytech/capi-multisig-app/pull/223/files#diff-74b31376d82cad53a7057c16707bde102daa50dca6962d8adb5a89eeb8daf5d8R25-R41

@statictype
Copy link
Contributor Author

@peetzweg yes, things will look different. i don't like the callbacks either. this is just a first step in creating a layer between capi and the UI

@statictype statictype merged commit 33bce54 into main Jul 5, 2023
@statictype statictype deleted the chore/decouple-capi-from-presentation-layer branch July 5, 2023 16:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants