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

Adding support for multisig accounts #384

Merged
merged 34 commits into from
Feb 24, 2025
Merged

Adding support for multisig accounts #384

merged 34 commits into from
Feb 24, 2025

Conversation

aaroncox
Copy link
Member

@aaroncox aaroncox commented Feb 23, 2025

  • multisig wallet plugin
  • account authority page
  • multisig session display
  • multisig options in settings page
  • rework of resources display
  • added datetime input
  • removed reference to pwa manifest
  • added support for time.eosn contract for checktime action in multisigs
  • improved filtering on activity page to prevent duplicates
  • misc supporting fixes/chanegs

Copy link

cloudflare-workers-and-pages bot commented Feb 23, 2025

Deploying 2nicove with  Cloudflare Pages  Cloudflare Pages

Latest commit: 39af9c9
Status: ✅  Deploy successful!
Preview URL: https://62d32969.unicove2.pages.dev
Branch Preview URL: https://multisig.unicove2.pages.dev

View logs

Comment on lines 260 to 267
<div class="text-left font-medium">
<div>{session.actor}@{session.permission}</div>
{#if session.walletPlugin.id === 'wallet-plugin-multisig'}
<div class="text-xs">
↳ multisig using {session.walletPlugin.data.session.actor}
</div>
{/if}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious what this looks like in context with layout, the arrow glyph, and the small text.

Probably should use a translation here too?

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Here's what it looks like. I've updated all the translations in the account switcher here: e692369

Copy link
Contributor

Choose a reason for hiding this comment

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

It might call for a slightly lighter-weight font in the second line, but otherwise looks good.

I'm always hesitant to use glyphs in the font vs an svg because they can become unpredicatable with different fonts, different browser settings, zoom levels, but more importantly: it's decorative, so it shouldn't be included in the text string (which will be translated at some point)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah totally understood. We probably want a "real" design for these elements, since as I worked on this I was just implementing my developer design to make something available.

I'm very open to change with how its displayed.

Comment on lines +48 to +49
The multi-sig proposal for this transaction has been created and now needs to be approved.
View the proposal below and share it with the parties who need to sign.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be picky about translations at this point?

Copy link
Member Author

Choose a reason for hiding this comment

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

We probably should get better about this, and have someone in charge of that entire pipeline. Considering we're not 100% translated right now though, I don't know if it should be a barrier?

@@ -12,6 +12,7 @@

import { ApprovalManager } from './manager.svelte';
import type { ActionDisplayVariants } from '$lib/types';
import { goto } from '$app/navigation';
Copy link
Contributor

Choose a reason for hiding this comment

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

Wonder if we should use the wrapped goto from $lib/utils so the language tag is included

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, one less redirect!

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated a few pages where this made sense.

6cfe7e9

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to rename this file if we're not linking to it in the <head> anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, probably not. I'll undo the rename.

</fieldset>
<fieldset class="grid gap-3">
{#if ramBytesUseCustom}
<SingleCard>
Copy link
Contributor

Choose a reason for hiding this comment

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

This component was designed with page-level layout in mind. Will have to evaluate how it works in this context.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm probably using a bunch of components wrong 😅

This page in particular shouldn't have even been committed. It is just a copy of one of the other forms, that I never got around to using. I'll delete it for the time being.

@@ -7,7 +7,7 @@

interface Props {
options: ExtendedSelectOption[];
selected: ExtendedSelectOption;
selected?: ExtendedSelectOption;
Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC this was required in the melt-ui builder so I made it required in the interface

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll revert this change and force a default option on the settings page then.

The select element with a default option ands up triggering the effects on the page, setting the value to whatever the select believes the default option should be. This could override defaults set elsewhere.

So there's some funk in here at the moment we should try to figure out how to address.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should address the funk and see if we can make selected optional. It does make sense to have the option to create a dropdown with nothing selected and no default.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, thanks!

<Card>
<div class="flex">
<div class="flex-1">
<Account name={auth.account_name} class="h2">
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on the video in Slack, this should probably be smaller than h2 since it's competing in prominence with the page header. Try h3 or h4

@aaroncox aaroncox merged commit 5f83e6a into dev Feb 24, 2025
2 checks passed
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.

2 participants