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

feat: transfer_from and decrease_allowance #134

Merged
merged 23 commits into from
Jul 31, 2024
Merged

feat: transfer_from and decrease_allowance #134

merged 23 commits into from
Jul 31, 2024

Conversation

chungquantin
Copy link
Collaborator

@chungquantin chungquantin commented Jul 27, 2024 •
edited
Loading

ISSUE: #104

Original wasm size: 30.7K, Optimized: 7.2K

API implementation

transfer_from

  • pallet-api implementation
  • pallet unit testing
  • contract integration test

decrease_allowance

  • pallet-api implementation
  • pallet unit testing
  • contract integration test
  • benchmarking (reuse the benchmark of approve())

@chungquantin chungquantin changed the base branch from main to daan/api July 27, 2024 01:25
@chungquantin chungquantin changed the title feat: transfer from decrease allowance feat: transfer_from and decrease_allowance Jul 27, 2024
Daanvdplas
Daanvdplas requested changes Jul 27, 2024
View reviewed changes
Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

Well done, a few nitpicks.

As for the empty lines, I'll make sure we get consensus about this next week and add it to the coding guidelines.

pallets/api/src/fungibles/mod.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Show resolved Hide resolved
pop-api/integration-tests/src/local_fungibles.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Show resolved Hide resolved
pop-api/integration-tests/src/local_fungibles.rs Outdated Show resolved Hide resolved
pop-api/integration-tests/src/local_fungibles.rs Outdated Show resolved Hide resolved
pop-api/integration-tests/src/local_fungibles.rs Outdated Show resolved Hide resolved
chungquantin and others added 11 commits July 28, 2024 13:43
chungquantin
chungquantin commented Jul 29, 2024
View reviewed changes
pallets/api/src/fungibles/tests.rs Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Show resolved Hide resolved
pop-api/src/lib.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Show resolved Hide resolved
chungquantin
chungquantin commented Jul 30, 2024
View reviewed changes
Copy link
Collaborator Author

@chungquantin chungquantin left a comment

Choose a reason for hiding this comment

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

Ready for final review

Daanvdplas
Daanvdplas reviewed Jul 30, 2024
View reviewed changes
pallets/api/src/fungibles/mod.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/mod.rs Outdated Show resolved Hide resolved
pop-api/integration-tests/src/local_fungibles.rs Outdated Show resolved Hide resolved
pop-api/integration-tests/src/local_fungibles.rs Outdated Show resolved Hide resolved
pop-api/integration-tests/src/local_fungibles.rs Outdated Show resolved Hide resolved
pallets/api/src/fungibles/tests.rs Outdated Show resolved Hide resolved
chungquantin and others added 2 commits July 30, 2024 18:01
Daanvdplas
Daanvdplas reviewed Jul 30, 2024
View reviewed changes
Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

My apologies, I mistakenly read the old code where we were checking if value == 0 twice. This should be checked once and returned, as well as the current_allowance - value. Could you please revert that removal?

Daanvdplas
Daanvdplas approved these changes Jul 31, 2024
View reviewed changes
Copy link
Collaborator

@Daanvdplas Daanvdplas left a comment

Choose a reason for hiding this comment

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

Well done Tin!

@chungquantin chungquantin merged commit 5ac5a9a into r0gue-io:daan/api Jul 31, 2024
5 checks passed
chungquantin added a commit that referenced this pull request Sep 6, 2024
@chungquantin @Daanvdplas
feat: transfer_from and decrease_allowance (#134)
59c3389 
Co-authored-by: Daan van der Plas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Daanvdplas Daanvdplas Daanvdplas approved these changes

@evilrobot-01 evilrobot-01 Awaiting requested review from evilrobot-01

@peterwht peterwht Awaiting requested review from peterwht

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chungquantin @Daanvdplas