-
Notifications
You must be signed in to change notification settings - Fork 59
Wallet UI support for minting delegations management #3601
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
Wallet UI support for minting delegations management #3601
Conversation
34f262a to
98c805c
Compare
2ce99c4 to
057af83
Compare
meiersi-da
left a comment
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.
Very nice work @dfordivam and @adetokunbo. Functionally we're close except for the wording.
The codestyle also looks good, but I'd appreciate if you @ray-roestenburg-da could have a peek yourself or ask somebody from your team to approve as the codeowners.
I've also pinged @waynecollier-da to have a look at the UI screenshots. (Thanks for providing them @adetokunbo 🙏 )
| const res = await walletClient.listMintingDelegations(); | ||
| return res.delegations.map(d => ({ | ||
| contract: Contract.decodeOpenAPI(d.contract, MintingDelegation), | ||
| beneficiaryOnboarded: d.beneficiary_onboarded, |
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.
| beneficiaryOnboarded: d.beneficiary_onboarded, | |
| beneficiaryHosted: d.beneficiary_hosted, |
In Canton docs, we talk about a validator node hosting parties.
The Splice Amulet Wallet UI uses "onboarded" to mean that a party has an active WalletAppInstall contract.
Let's talk about "hosting" 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.
Done
| marginTop={4} | ||
| > | ||
| <Typography variant="h4" id="proposals-label"> | ||
| Proposed |
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.
| Proposed | |
| Proposed Minting Delegations |
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.
the goal is to make clear what kind of delegations are seen 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.
Done
| <TableRow> | ||
| <TableCell>Beneficiary</TableCell> | ||
| <TableCell>Onboarded</TableCell> | ||
| <TableCell>Max Amulets</TableCell> |
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.
| <TableCell>Max Amulets</TableCell> | |
| <TableCell>Merge Threshold</TableCell> |
- the merge logic does not maintain a strict max
- when using "Amulet" care must be taken to read the name from the config, so that the wallet properly shows when configured as a "Canton Coin" wallet.
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.
Done
| <TableHead> | ||
| <TableRow> | ||
| <TableCell>Beneficiary</TableCell> | ||
| <TableCell>Onboarded</TableCell> |
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'd drop this column, and instead add an annotation toe beneficiary parties that are not hosted on the validator node. Possible options:
- show a warning icon before the party-id with a hover text "Minting delegations do not work for this party, as it is not hosted on this validator node."
- show the warning text inline below the party name.
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.
| )} | ||
|
|
||
| <Typography variant="h4" id="delegations-label"> | ||
| Active |
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.
| Active | |
| Active Minting Delegations |
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.
add a hover or inline text "The validator automates minting and merging holdings for the beneficiaries of the delegations below."
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.
Done: went with a hover
| conditions={[ | ||
| { | ||
| disabled: !beneficiaryOnboarded, | ||
| reason: 'Beneficiary is not onboarded', |
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 assume this is shown on hover, is it?
side note: change everywhere to "not hosted on this validator"
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.
It is, and the message has been updated
Done
| beneficiaryOnboarded: boolean; | ||
| } | ||
|
|
||
| export interface MintingDelegationProposalWithOnboardedStatus { |
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 think this interface is not used anywhere, right?
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.
Correct; sorry that should have been cleaned up better.
Removed
| useWalletClient, | ||
| } from '../contexts/WalletServiceContext'; | ||
|
|
||
| export const shortenPartyId = (partyId: string): string => { |
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.
We already have an implementation of the shortenPartyId in TransactionHistory.tsx which I'd love to reuse somehow, perhaps move to src/utils?
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.
While we're at it, please replace elements.length == 2 with elements.length === 2
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.
Done, moved to src/utils/partyId.ts
pawelperek-da
left a comment
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.
Thank you @dfordivam and @adetokunbo, the UI code looks very solid. Please take a look at my 2 nits
Signed-off-by: Divam <dfordivam@gmail.com>
- Add MintingDelegationCollectRewardsTrigger - Bump version of DbExternalPartyWalletStore - Bump version of DbUserWalletStore Squashed commits from the PR reviewed in #3470 Signed-off-by: Divam <dfordivam@gmail.com>
Squashed commits from PR reviewed in #3598 Signed-off-by: Divam <dfordivam@gmail.com>
Extracted duplicated listSortedRewardCoupons method from DbUserWalletStore and DbExternalPartyWalletStore into a new shared mixin trait. PR reviewed in #3646 Signed-off-by: Divam <dfordivam@gmail.com>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
Signed-off-by: Tim Emiola <adetokunbo@emio.la>
98c805c to
f919249
Compare
057af83 to
9bcf04f
Compare
meiersi-da
left a comment
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.
Thank you @dfordivam and @adetokunbo for the very quick and thorough turnaround. This is good to go 💪 👏
PR reviewed on #3601 Signed-off-by: Tim Emiola <adetokunbo@emio.la>
f919249 to
1da591d
Compare
|
Closing this commits have been squashed and merged on |













Fixes #3553
PR author is https://github.com/adetokunbo , I have pushed this branch and created this PR for running CI
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines