Skip to content
This repository has been archived by the owner on Dec 1, 2023. It is now read-only.

Add more context to comment #219

Closed
ericnordelo opened this issue Oct 3, 2022 · 2 comments · Fixed by #228
Closed

Add more context to comment #219

ericnordelo opened this issue Oct 3, 2022 · 2 comments · Fixed by #228
Assignees
Milestone

Comments

@ericnordelo
Copy link
Member

    i actually think this comment is a bit confusing with no other context

Originally posted by @martriay in #218 (comment)

@ericnordelo ericnordelo self-assigned this Oct 3, 2022
@ericnordelo ericnordelo moved this to 📋 Backlog in Cairo team Oct 3, 2022
@ericnordelo ericnordelo added this to the next milestone Oct 3, 2022
@martriay martriay modified the milestones: next, current Oct 6, 2022
@martriay
Copy link
Contributor

martriay commented Oct 6, 2022

i was actually vouching for removing it haha. Wasn't the comment a clarification on the diff, rather than a clarification on the code?

@ericnordelo
Copy link
Member Author

ericnordelo commented Oct 6, 2022

My intention with the comment was to add some clarification to the fact that other functions in CLI using address_or_alias normalize the number if it is not an alias, but send specifically doesn't. This is a difference in the code I think it could have been confusing. I'm inclined to let the comment because of this, refactoring it to something more informative, like the following idea:

"address_or_alias is not normalized first here, because Account.send is part of the nile public API, consequently accepting address as hex too"

@ericnordelo ericnordelo moved this from 📋 Backlog to 🏗 In progress in Cairo team Oct 6, 2022
@ericnordelo ericnordelo linked a pull request Oct 6, 2022 that will close this issue
@ericnordelo ericnordelo moved this from 🏗 In progress to 👀 In review in Cairo team Oct 6, 2022
Repository owner moved this from 👀 In review to ✅ Resolved in Cairo team Oct 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants