-
Notifications
You must be signed in to change notification settings - Fork 16
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
Rename ICRC-1 memo to icrc1Memo #425
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Not sure renaming the field really makes it more clear that both field are unrelated. I mean "memo" and "icrc1_memo" are still "memo". In addition, the lib "ledger" is meant for icrc-1 and this is a breaking changes. So personally rather like that we do not rename it. |
I've reverted the changes in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dskloetd
Motivation
The ICP ledger canister offers different interfaces to make transfers. The newer ICRC-1 interface and the older "legacy" interface.
Both interfaces allow passing a
memo
field in the parameters.However these fields called
memo
have absolutely nothing to do with each other.When fetching the transaction later, you get 2 unrelated fields called "memo" and "icrc1_memo".
When using the ICRC-1 interface it is not possible to set the
memo
field and when using the "legacy" interface it is not possible to set theicrc1_memo
field.Given how surprising and confusing this is, I believe it was a mistake to give these fields the same name in the method parameters.
Changes
memo
field in the ICRC-1 method parameters toicrc1Memo
, in the ic-js repo.Tests
Existing unit test have been updated where necessary.