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

Fix calls to management canister #17

Merged
merged 2 commits into from
Mar 28, 2024

Conversation

nmattia
Copy link
Contributor

@nmattia nmattia commented Mar 26, 2024

This fixes an issue in the RequestID computation where an empty slice canister ID would be considered as "canister_id not set". This was not quite correct, because the management canister ID is the empty slice (encoded as "aaaaa-aa").

This changes the empty check for a nil check instead. Usages of the effective canister ID vs. canister ID are also clarified.

This also changes the way the logger prints the request ID to use a hex representation, which is easier to read & convert than the byte characters.

This fixes an issue in the RequestID computation where an empty slice
canister ID would be considered as "canister_id not set". This was not
quite correct, because the management canister ID is the empty slice
(encoded as "aaaaa-aa").

This changes the empty check for a `nil` check instead. Usages of the
effective canister ID vs. canister ID are also clarified.
agent.go Outdated Show resolved Hide resolved
Copy link
Member

@q-uint q-uint left a comment

Choose a reason for hiding this comment

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

LGTM!

agent.go Outdated Show resolved Hide resolved
agent.go Show resolved Hide resolved
agent.go Show resolved Hide resolved
agent.go Show resolved Hide resolved
agent.go Outdated Show resolved Hide resolved
agent.go Outdated Show resolved Hide resolved
agent.go Show resolved Hide resolved
agent.go Show resolved Hide resolved
agent.go Show resolved Hide resolved
@q-uint
Copy link
Member

q-uint commented Mar 27, 2024

Thanks for helping with this @peterpeterparker and @frederikrothenberger!

@nmattia
Copy link
Contributor Author

nmattia commented Mar 27, 2024

@q-uint thanks for the feedback! I'll implement everything tomorrow. Apologies for the Go noob mistakes :)

@nmattia nmattia marked this pull request as ready for review March 28, 2024 09:13
@nmattia nmattia requested a review from q-uint March 28, 2024 09:20
@q-uint q-uint merged commit 17a235d into aviate-labs:main Mar 28, 2024
@nmattia nmattia deleted the nm-management-canister-id branch March 28, 2024 11:29
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