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.

Sign up for GitHub

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

Jump to bottom

37 fix currency and amout, add result #38

Merged
merged 14 commits into from
Dec 7, 2022
Merged

37 fix currency and amout, add result #38

merged 14 commits into from
Dec 7, 2022

Conversation

gregLibert
Copy link
Contributor

No description provided.

This was linked to issues Nov 28, 2022
Fix currency #37
Closed
Fix Amount #36
Closed
Thykof
Thykof requested changes Nov 29, 2022
View reviewed changes
Copy link
Contributor

@Thykof Thykof left a comment •
edited
Loading

Choose a reason for hiding this comment

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

here is my review

assembly/result.ts Outdated Show resolved Hide resolved
assembly/argument.ts Outdated Show resolved Hide resolved
assembly/argument.ts Outdated Show resolved Hide resolved
assembly/argument.ts Outdated Show resolved Hide resolved
assembly/argument.ts Outdated Show resolved Hide resolved
assembly/argument.ts Outdated Show resolved Hide resolved
assembly/currency.ts Outdated Show resolved Hide resolved
assembly/amount.ts Outdated Show resolved Hide resolved
@Thykof Thykof self-assigned this Nov 30, 2022
@Thykof Thykof assigned gregLibert and unassigned Thykof Nov 30, 2022
This was referenced Dec 1, 2022
Closed
Closed
Closed
gregLibert
gregLibert commented Dec 1, 2022
View reviewed changes
Copy link
Contributor Author

@gregLibert gregLibert left a comment

Choose a reason for hiding this comment

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

Doing my own review :-)

assembly/__tests__/amount.spec.ts Outdated Show resolved Hide resolved
assembly/__tests__/amount.spec.ts Outdated Show resolved Hide resolved
assembly/__tests__/argument.spec.ts Outdated Show resolved Hide resolved
assembly/__tests__/argument.spec.ts Outdated Show resolved Hide resolved
assembly/__tests__/argument.spec.ts Outdated Show resolved Hide resolved
assembly/amount.ts Show resolved Hide resolved
assembly/amount.ts Show resolved Hide resolved
assembly/amount.ts Show resolved Hide resolved
assembly/result.ts Outdated Show resolved Hide resolved
assembly/result.ts Show resolved Hide resolved
@Thykof Thykof changed the title 37 fix currency 37 fix currency and amout, add result Dec 2, 2022
@Thykof
Copy link
Contributor

Thykof commented Dec 6, 2022

@gregLibert I would prefer to work with addArgs and fromArgs because it is easier to work with other classes. For example in Amount.addArgs it calls Currency.addArgs. So I have added a test case to show case this. Do you want me to remove Amount.toBytes and Amount.fromBytes?

@gregLibert
Copy link
Contributor Author

@gregLibert I would prefer to work with addArgs and fromArgs because it is easier to work with other classes. For example in Amount.addArgs it calls Currency.addArgs. So I have added a test case to show case this. Do you want me to remove Amount.toBytes and Amount.fromBytes?

Yes plz

@gregLibert gregLibert merged commit 4ab2912 into main Dec 7, 2022
@gregLibert gregLibert deleted the 37-fix-currency branch December 7, 2022 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterjah peterjah peterjah approved these changes

@Thykof Thykof Thykof approved these changes

Assignees
No one assigned
Labels
action:merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix currency Fix Amount
3 participants
@gregLibert @Thykof @peterjah