Skip to content
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

Support Joystream #437

Merged
merged 10 commits into from
Nov 22, 2023
Merged

Support Joystream #437

merged 10 commits into from
Nov 22, 2023

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Nov 14, 2023

closes #438


Submission checklist:

Layout

  • Change inspected in the desktop web ui
  • Change inspected in the mobile web ui

Compatibility

  • Functionality of change validated with a connected account with multisig
  • Applicable elements hidden / disabled for watched multisigs / pure
  • Looks good for solo multisig
  • Looks good for multisig with proxy

Copy link
Contributor

@asnaith asnaith left a comment

Choose a reason for hiding this comment

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

Looking good for the most part, I was able to create a multisig (without proxy as it says proxies are not supported on Joystream) and send a tx.

However, there is an app crash if you try to set identity either from the options menu on the main page or the set identity dropdown on the "Send tx" modal.

CleanShot 2023-11-15 at 15 27 48@2x

@Tbaut
Copy link
Collaborator Author

Tbaut commented Nov 16, 2023

Great find. I guess they don't have the identity pallet as well, and Multix assumes it's there. I'll fix, and add a test.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Nov 21, 2023

Aaaand finally fixed the tests!

Copy link
Contributor

@asnaith asnaith left a comment

Choose a reason for hiding this comment

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

Looks great, can no longer see the identity options...but that's not new information because your new test is already telling you that, awesome! 😎

🚀

when?: MultisigStorageInfo['when']
}
export const getAsMultiTx = ({ api, threshold, otherSignatories, tx, weight, when }: Params) => {
return api.tx.multisig.asMulti.meta.args.length === 6
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we make a "6" a constant that will describe itself? as it looks as magic number now :)

Copy link
Collaborator Author

@Tbaut Tbaut Nov 22, 2023

Choose a reason for hiding this comment

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

Good point, it is indeed a magic number as it's actually not future proof. Unfortunately we have no other choice afaik (I took this idea from the pjs/apps code). Joystream will migrate in the next months, and we can eventually remove this "if". I'll make this a constant such as PARAM_LENGTH_OLD_ASMULTI

Copy link
Collaborator

@Lykhoyda Lykhoyda left a comment

Choose a reason for hiding this comment

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

looks good! Please use some constant in getAsMultiTx file

@Tbaut Tbaut merged commit 4a63d90 into main Nov 22, 2023
6 checks passed
@Tbaut Tbaut deleted the tbaut-joystream-support branch November 22, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Joystream
3 participants