Skip to content

Conversation

@jsonsivar
Copy link

@jsonsivar jsonsivar commented Apr 28, 2022

Why?

We need flexibility to add fleek users because we need to transfer team crowns but it is using the principal id as the owner instead of the cycle wallet id.

How?

  • Added new management helper to append a fleek user
  • Added an external function to be able to call that from outside
  • Added candid

Tickets?

None

Contribution checklist?

  • The commit messages are detailed
  • It does not break existing features (unless required)
  • I have performed a self-review of my own code
  • Documentation has been updated to reflect the changes
  • Tests have been added or updated to reflect the changes
  • All code formatting pass
  • All lints pass
  • All tests pass

Security checklist?

  • Injection has been prevented (parameterized queries, no eval or system calls)
  • Sensitive data has been identified and is being protected properly

Demo?

Optionally, provide any screenshot, gif or small video.

@jsonsivar
Copy link
Author

Question for team on this - there are separate check boxes in the PR template for lint and format. What exactly would the lint be for? I ran cargo fmt and checked the format box. cc @heldrida

@scott-dn
Copy link
Contributor

Question for team on this - there are separate check boxes in the PR template for lint and format. What exactly would the lint be for? I ran cargo fmt and checked the format box. cc @heldrida

hey @jsonsivar
cargo fmt —all for format
and
cargo clippy --all-targets --all-features -- -D warnings -D clippy::all for lint

@ozwaldorf
Copy link
Collaborator

Just to note, with dfx 0.9.2 all calls default to principal id, not wallet id, but you can make a call with a wallet using the --wallet flag

@punkbit
Copy link
Member

punkbit commented Apr 28, 2022

Question for team on this - there are separate check boxes in the PR template for lint and format. What exactly would the lint be for? I ran cargo fmt and checked the format box. cc @heldrida

hey @jsonsivar cargo fmt —all for format and cargo clippy --all-targets --all-features -- -D warnings -D clippy::all for lint

Thanks @scott-dn

@punkbit punkbit added the draft label Apr 28, 2022
@punkbit
Copy link
Member

punkbit commented Apr 28, 2022

The PR is set to merge into "backup/master", that's a typo, right? @jsonsivar

@jsonsivar
Copy link
Author

The PR is set to merge into "backup/master", that's a typo, right? @jsonsivar

So I'm actually making this PR to the currently deployed version which is in backup/master. This is so that we can deploy it independent from the enhancements/migration work that's going on in master. At least that's what I was thinking but let me know if we should take a diff approach or sync more on it.

@jsonsivar jsonsivar changed the title WIP: Added api to add a fleek user Added api to add a fleek user Apr 28, 2022
@jsonsivar jsonsivar marked this pull request as draft April 28, 2022 14:34
Copy link

@Nima-Ra Nima-Ra 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 to me! Left a suggestion but it's not a needed change. 🙂

type AddFleekUserResult =
variant {
Err: ApiError;
Ok: opt bool;
Copy link

Choose a reason for hiding this comment

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

Suggested change
Ok: opt bool;
Ok: opt text;

I think this might be better since a boolean can cause some confusion and afaik all psychedelic canisters use Ok(()) to pass the result.

}

add_fleek_user(account);
Ok(None)
Copy link

Choose a reason for hiding this comment

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

related to my suggestion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants