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

refactor(sns): Use ic-nervous-system-agent in sns-audit #1756

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

anchpop
Copy link
Contributor

@anchpop anchpop commented Sep 30, 2024

Problem

sns-audit is available as a library, which makes it tempting to use in tests and in the CLI. However, there are some blockers:

  1. the sns-audit library requires ic-agent, which is not usable in the context of pocketic tests as far as I know.
  2. I would like new CLI functionality to be written using ic-nervous-system-agent as it makes the code much more readable.

Additionally, the errors from sns-audit errors are all strings. This is not optimal for consuming it as a library where you might want to have some logic that depends on the particular error that occurred, if one didi.

Solution

Rewrite sns-audit to use ic-nervous-system-agent, and use an explicit error type rather than using strings.

Notes

The error messages have changed slightly. They no longer mention the SNS's name. It is now mentioned sooner.

New:

❯ bazel run :sns-audit --config=lint "https://ic0.app/" "mlqf6-nyaaa-aaaaq-aadxq-cai"                                                          
sns_name = Juno Build
sns_tokens_per_icp = 549.4149218088792154581282989
Error: swap is not in the final state yet, so `initial_neurons_fund_participation` is not specified.

Old:

❯ bazel run :sns-audit --config=lint "https://ic0.app/" "mlqf6-nyaaa-aaaaq-aadxq-cai"
sns_tokens_per_icp = 549.4149218088792154581282989
Error: "SNS swap Juno Build is not in the final state yet, so `neurons_fund_refunds` is not specified."

This was done to make the error type simpler. Otherwise, it would have to carry the SNS name information in all cases where it was relevant. Since it is still printed slightly higher up, and hopefully people know the SNSes they are auditing anyway, I hope that's just as good.

How to review this PR

The first commit makes changes to ic-nervous-system-agent to add helpers that will be useful to sns-audit.

The second commit changes sns-audit to use ic-nervous-system-agent. There, you can see that the implementation of sns-audit is dramatically simplified – it now is not concerned with the details of how to call the canisters, as that is handled by ic-nervous-system-agent.

@anchpop anchpop requested a review from a team as a code owner September 30, 2024 19:39
@anchpop anchpop marked this pull request as draft September 30, 2024 19:40
@anchpop anchpop force-pushed the @anchpop/sns-audit-nervous-system-agent branch 3 times, most recently from 1be2a3a to 45cef86 Compare September 30, 2024 20:27
@anchpop anchpop marked this pull request as ready for review September 30, 2024 21:05
@anchpop anchpop force-pushed the @anchpop/sns-audit-nervous-system-agent branch from 45cef86 to 2917240 Compare September 30, 2024 21:07
@anchpop anchpop force-pushed the @anchpop/sns-audit-nervous-system-agent branch from 2917240 to 2019e13 Compare September 30, 2024 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant