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 : ExitFee Settings moved to contract! #42

Closed
wants to merge 16 commits into from
Closed

Feat : ExitFee Settings moved to contract! #42

wants to merge 16 commits into from

Conversation

sk-enya
Copy link

@sk-enya sk-enya commented Oct 10, 2024 •
edited
Loading

  • nvmrc update to v20.17
  • updated commands use of of pnpm form yarn
  • updated pnpm lock file
  • updated readme with more details of executing the scripts.
  • add variable to store exitfee
  • added event to emit on seet of exit fee
  • added setExitFee function to set exit fee only for owner
  • update initialization to set exit fee on initialization of contract.
  • add respective unit test cases to validate multiple scenarios.
  • removed hardcoded logic of exit fee calculation.
  • updated service to use the exit fee from the contract to calculate for disbursment.
  • updated the lightbridge parallel spec with exit fee

Unit Test: Passing

Screenshot 2024-10-11 at 12 55 13 AM

@meanMonk
change:
c0e3030 
- nvmrc update to v20.17
- updated commands use of of pnpm form yarn
- updated pnpm lock file
- updated readme with more details of executing the scripts.
@meanMonk
exit fee changes:
965f014 
- add variable to store exitfee
- added event to emit on seet of exit fee
- added setExitFee function to set exit fee only for owner
- update initialization to set exit fee on initialization of contract.
- add respective unit test cases to validate multiple scenarios.
@meanMonk
formating changes
cbde6d6
@meanMonk
change:
b59f266 
- removed hardcoded logic of exit fee calculation.
- updated service to use the exit fee from the contract to calculate for disbursment.
- updated the lightbridge parallel spec with exit fee
@meanMonk
linting and format fixes!
aa26c62
@sk-enya sk-enya requested review from wsdt and boyuan-chen October 10, 2024 19:26
@boyuan-chen
Copy link
Contributor

Do we want to update the logic to let the smart contract automatically handle the exit fee instead of using the backend service to calculate the amount?

@wsdt
Copy link
Contributor

wsdt commented Oct 11, 2024 •
edited
Loading

Do we want to update the logic to let the smart contract automatically handle the exit fee instead of using the backend service to calculate the amount?

are we sure we want all that? The contracts have been audited. Might force us to re-audit. This was also the reason I added it in the backend service and not in the contract.

wsdt
wsdt reviewed Oct 11, 2024
View reviewed changes
contracts/LightBridge.sol Outdated Show resolved Hide resolved
wsdt
wsdt reviewed Oct 11, 2024
View reviewed changes
contracts/LightBridge.sol Outdated Show resolved Hide resolved
wsdt
wsdt reviewed Oct 11, 2024
View reviewed changes
contracts/LightBridge.sol Outdated Show resolved Hide resolved
wsdt
wsdt reviewed Oct 11, 2024
View reviewed changes
test/lightbridge.unit.spec.ts Outdated Show resolved Hide resolved
wsdt
wsdt approved these changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@wsdt wsdt 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.

great work, looks good to me Sahil!

Another remark, maybe we want to change the base to 1000 (from a 100)? This way we can also set 0.1% and similar fee structures (often times used by contracts).

NOTE: We will also need to reach the fee on the gateway now as it is now dynamic.

@meanMonk
change:
6d0159d 
- revert initialize call change.
- data type updated for exitFee to save w.r.to sourceChainId
- added variable to record feeCollected
- added function to calculate the amountAfterFee and feeAmount
- Added unit specs for calculate fee fuction
- updated unit spec for setExitFee value.
@meanMonk
formatting changes!
058a03e
@meanMonk
change:
73abdc8 
- removed exit fee logic complete from service util
- cleanup exitFee fetch from contract in service call.
- only passing amountAs is in string format to disburseAsset
- clean up spec for deductExitFeeCalculation logic.
@meanMonk
formatting & unused import fix
bce5a64
@meanMonk
lint fix & yarn change to pnpm
79f7e2d
@sk-enya
Copy link
Author

sk-enya commented Oct 11, 2024 •
edited
Loading

Thank you @wsdt ,
updated the changes as per the comment can you please review it once again?

sure, will also make changes on gateway as per the exitFee from contract!

@sk-enya sk-enya requested a review from wsdt October 11, 2024 14:20
wsdt
wsdt reviewed Oct 11, 2024
View reviewed changes
Copy link
Contributor

@wsdt wsdt left a comment

Choose a reason for hiding this comment

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

@wsdt
Copy link
Contributor

wsdt commented Oct 12, 2024

looks good!

Integration tests are failing though:

image

@wsdt wsdt self-requested a review October 12, 2024 09:11
@meanMonk
changes:
b2c9788 
- updated docker-compose base with platform linux/amd4
- cleanup lightbridge as it should be running from reg or parellel yml.
@meanMonk
change:
aa3d8da 
- updated docker file with pnpm update.
- updated package json with pnpm and use docker-compose.
- reverted lb-tests yml
@meanMonk
fix docker command
91deb07
@meanMonk
changes in docker and files
31ab3b2
@sk-enya
Copy link
Author

sk-enya commented Oct 14, 2024

Closing the PR with respective other separate for each task has been raised.
#43

@sk-enya sk-enya closed this Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boyuan-chen boyuan-chen Awaiting requested review from boyuan-chen

@wsdt wsdt Awaiting requested review from wsdt

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

Successfully merging this pull request may close these issues.
4 participants
@sk-enya @boyuan-chen @wsdt @meanMonk