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

Solving issue #1905: Aggregate Fights into a single txn to reduce gas… #1906

Merged

Conversation

NindoK
Copy link
Contributor

@NindoK NindoK commented Mar 29, 2023

All Submissions

New Feature Submissions

Related to issues #1905

Notes

Please note, your code must pass all tests and lint checks before it can be merged.

PR Description

In this PR I propose a solution for aggregating all the fights into a single txn.

The solution require minor changes in the contract:

  1. Change the function declaration in order to take in input arrays of data, instead of single values
  2. Loop through each characters and perform fight
  3. Keep track of the cumulated tokens
  4. revert if needed or complete execution.

Instead in the Front End, I have readapted two old functions fightTarget in Combat.vue and doEncounterPayNative in index.ts, I have just renamed these functions with their plural form to remain consistent.

In both files I have changed that instead of the single values like characterId, targetId or fightMultiplier, now they are all arrays.

For now this PR solves only the fighting part, but not the UI. The UI should let the users select for each character their enemy and then with a button fight all of them at once.

With this change the transaction gas goes down from ~356k to ~230k.

image

I have made some small changes into the README.md file, since I had some troubles while setting up the environment.

Testing

The code has been tested multiple times, but I advise to check the math behind the offset, since I might not have understood correctly.

@github-actions github-actions bot added pr-frontend Frontend pull request pr-solidity Solidity pull request labels Mar 29, 2023
@devinep52 devinep52 requested a review from Chriserus April 26, 2023 14:59
@remocwenn remocwenn mentioned this pull request Apr 27, 2023
3 tasks
contracts/TokensManager.sol Show resolved Hide resolved
frontend/src/views/Combat.vue Outdated Show resolved Hide resolved
@NindoK NindoK requested a review from Chriserus May 4, 2023 15:04
@NindoK NindoK requested a review from Chriserus May 7, 2023 18:00
@Chriserus Chriserus requested a review from bilbolPrime May 7, 2023 18:00
Copy link
Member

@Chriserus Chriserus left a comment

Choose a reason for hiding this comment

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

LGTM, also consulted with @bilbolPrime
Please resolve the conflicts

@NindoK
Copy link
Contributor Author

NindoK commented May 8, 2023

LGTM, also consulted with @bilbolPrime Please resolve the conflicts

Awesome! Glad to hear it :D Just solved the conflicts.

@jeffyzxc jeffyzxc changed the base branch from main to feature/team-fights May 11, 2023 04:49
@jeffyzxc jeffyzxc changed the base branch from feature/team-fights to main May 11, 2023 14:05
@jeffyzxc jeffyzxc merged commit 7525766 into CryptoBlades:main May 11, 2023
@NindoK NindoK deleted the Aggregated-fights-into-single-transaction branch May 11, 2023 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-frontend Frontend pull request pr-solidity Solidity pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants