paritytech / capi-multisig-app

[WIP] Simple multisig administration
https://multisig.capi.dev/
Apache License 2.0
8 stars 0 forks source link

chore: upgrade capi to `gamma.1` #217

Closed peetzweg closed 1 year ago

peetzweg commented 1 year ago

capi sync does not work as expected anymore. Had this issue already with beta43, any ideas how to fix this @harrysolovay ?

Screenshot 2023-06-16 at 11 59 36

Without --runtime-config package.json:

Screenshot 2023-06-16 at 12 00 16
netlify[bot] commented 1 year ago

Deploy Preview for capi-multisig ready!

Name Link
Latest commit 30976d5eabd61f39a33556272e2a2acd48aefd21
Latest deploy log https://app.netlify.com/sites/capi-multisig/deploys/64a6757fd4a84a0008e51c3a
Deploy Preview https://deploy-preview-217--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.

peetzweg commented 1 year ago

Also Rune.run() type might be broken? It expects a required argument scope now?

Screenshot 2023-06-16 at 12 10 23
kratico commented 1 year ago

@peetzweg for the capi sync issue, the nets.ts file needs to be updated with


diff --git a/nets.ts b/nets.ts
index 05a9c15..e81c2e4 100644
--- a/nets.ts
+++ b/nets.ts
@@ -1,4 +1,4 @@
-import { net } from "capi"
+import { net } from "capi/nets"

 export const westend = net.ws({
   url: "wss://westend-rpc.polkadot.io/",
peetzweg commented 1 year ago
import { net } from "capi/nets"

Ah alright, wasn't aware of that change. Does not seem to be resolved correctly though.

Screenshot 2023-06-16 at 13 21 38

I've tried as well: capi/nets capi/nets/mod.js capi/nets/mod

kratico commented 1 year ago

Yeap, this is weird and it's related to the type declaration. Despite the type issue, I tried capi sync and it worked.

Did capi sync work for you?

I tweaked this manually in the package.json by rewriting the subpath export and it seems to work

image

Created the issue https://github.com/paritytech/capi/issues/1088

peetzweg commented 1 year ago

Ignoring this type issue here. In development the app does not load anymore and this error Rune related error shows up in the console.

Screenshot 2023-06-16 at 14 01 51
tjjfvi commented 1 year ago

Also Rune.run() type might be broken? It expects a required argument scope now?

This is to be expected (only by us, that is, seeing as we failed to let you know); as of https://github.com/paritytech/capi/pull/1084 this argument is required.

You can declare

const scope = new Scope()

at the top-level of your app, and pass that in to every run. This will ensure that all execution happen in the same scope, thus sharing cache etc.

(Eventually, we'll likely make a useRune hook, which – among other things – will handle passing in the scope via context)

Ignoring this type issue here. In development the app does not load anymore and this error Rune related error shows up in the console.

Once you pass in a scope this should be resolved.

peetzweg commented 1 year ago

I think solving it in a "react" library down the line is quite far away, making this use very cumbersome.

How about setting a "defaultScope" for all Runes in a static way and just pass an override scope on a per rune basis?

Haven't looked into the details of the scope but feels like something which is not changing too often in the lifecylce of an app, or is it?

tjjfvi commented 1 year ago

I'm not sure how you define quite far away, but I could see working on such a library before v0.2.0.

We discussed having a default, global scope, but decided that this would be too much of a footgun, as it would be very easy to forget to pass in an override scope.

peetzweg commented 1 year ago

Ready for review.

harrysolovay commented 1 year ago

For the time being, I would just pass an anonymous scope into every run. No need to persist in a signal. We'll revisit this after addressing paritytech/capi#1091

peetzweg commented 1 year ago

Just updated to gamma.1, works fine so far. Like creating a proxy, replacing delegates and funding. But in ratify / cancel multisig extrinsics I get the following errors on the command line cc @tjjfvi @harrysolovay .

I have not yet isolated the issue so not created a capi issue for it. Maybe it's obvious how to quickfix it?

The actual extrinsic seems to have made it through though.

Screenshot 2023-07-05 at 09 48 47
statictype commented 1 year ago

i'm handling the update in a new PR. this branch is too old, don't want to fix all these conflicts

peetzweg commented 1 year ago

Just resolved all conflicts minutes before. 🤷 Most of the changes are just formatting stuff anyways. Btw we have dprint and prettier in this repo. I think we should drop one of them to prevent this formatting ping pong.

https://github.com/paritytech/capi-multisig-app/blob/30976d5eabd61f39a33556272e2a2acd48aefd21/www/package.json#L29

https://github.com/paritytech/capi-multisig-app/blob/30976d5eabd61f39a33556272e2a2acd48aefd21/dprint.json#L15-L19

statictype commented 1 year ago

wouldn't merge this version now because it produces an error with approvals and cancellations. even if the tx goes through, we can't capture the events anymore.

harrysolovay commented 1 year ago

paritytech/capi#1139