Skip to content

Add get_account_delegation functionality #3078

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

Merged
merged 102 commits into from
May 21, 2025

Conversation

LXIF
Copy link
Contributor

@LXIF LXIF commented May 16, 2025

Motivation

In order to authenticate, we need to retrieve the account delegation we prepared. NOTE: This PR branches off of #3076 - that PR should probably be reviewed and merged before this one.

Changes

Add get_account_delegation query method, add get_account_delegation function in account_management.rs, remove get_delegation in delegation.rs and replace it with get_account_delegation in the get_delegation endpoint.

Tests

No new tests were added, integration tests for account delegations are in Jira and will be implemented asap. However, as legacy functions have been replaced, there is some amount of test coverage already.

LXIF added 30 commits May 13, 2025 11:15
github-actions bot and others added 2 commits May 19, 2025 13:31
@LXIF LXIF requested a review from lmuntaner May 19, 2025 13:44
Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Only two things were added here from the prepare_delegation PR, right?

  • get_account_delegation in account_management
  • use the new get_account_delegation in get_delegation endpoint.

@LXIF
Copy link
Contributor Author

LXIF commented May 20, 2025

Only two things were added here from the prepare_delegation PR, right?

  • get_account_delegation in account_management
  • use the new get_account_delegation in get_delegation endpoint.

That's the gist, yes.

LXIF and others added 8 commits May 20, 2025 14:31
* add get_accounts_for_origin to account_management and get_accounts to main, update existing code

* add authorization fn

* clippy

* add tests for get and create

* clippy

* add account updating endpoint and related account management functions as well as tests

* cwippy

* add seed_from_anchor to Account

* add seed_from_anchor to account struct

* add calculate_account_delegation_seed function

* make get_accounts return a result

* add principal to GetAccountsError::Unauthorized

* add const

* add global acc limit and unit test

* add max acc limit to update function

* add authorization, add checking if anchor has account (for later)

* clippy

* fix did

* add per-acc auth check in account management

* add tests, restructure per-acc auth check in update_account

* change variant name, update comment

* cleanup function inputs

* cwippy

* cwippy again

* minor changes

* add account delegation preparation function and update endpoint

* fix did

* fix did

* cwippy

* fix did again

* cwippy fix

* fix did?

* fix typing

* clippyyy

* cleanup

* clippy

* fix read_account method to account for seed_from_anchor

* rename

* move seed generation function into account impl

* clippy

* cleanup delegation generation, handle default account in has_account_reference, move anchor_has_account

* return account from anchor_has_account

* cleanupt

* restructure has_account_reference and anchor_has_account

* cwippy

* return default acc if no application number

* cwippy

* cleanup

* restructure storage borrowing

* make nicer

* update list_accounts to return accounts and use read_account

* remove anchor_has_account, pack everything into read_account

* clippy

* clippy

* add comment

* clippy

* change error type

* fix function inputs

* fix tests

* 🤖 cargo-fmt auto-update

* add tests, rename some things

* remove comment

* update comments

* add expect message, add comments

* clippy

* update match statement in calculate_seed

---------

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
* Improve copy in buttons

* Revert changes in use another account
@LXIF LXIF requested a review from lmuntaner May 20, 2025 15:01
Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, but we need the new TS types as well if you change the candid file.

I'm surprised the pipeline is green. Didn't you enable the interface tests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will you also recreate the TS types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lmuntaner I had to disable the interface tests already for the other PRs because some dummy endpoints did not return Result.

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 enable them now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

In another PR I mean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR is already up, but it fails because it apparently compares to the interface of the last release - which would mean we can only re-enable it once we have released.

Copy link
Contributor

Choose a reason for hiding this comment

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

@lmuntaner should be able to force that PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it pass once this PR is merged?

@LXIF LXIF requested a review from lmuntaner May 21, 2025 08:26
Copy link
Collaborator

@lmuntaner lmuntaner left a comment

Choose a reason for hiding this comment

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

LGTM!

@LXIF LXIF added this pull request to the merge queue May 21, 2025
@sea-snake sea-snake removed this pull request from the merge queue due to a manual request May 21, 2025
@LXIF LXIF added this pull request to the merge queue May 21, 2025
Merged via the queue into main with commit 826c981 May 21, 2025
70 checks passed
@LXIF LXIF deleted the andri/implement-retrieve-account-delegation branch May 21, 2025 10:39
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.

3 participants