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

Refactor QuarkBuilder and Bridge routing fix #93

Merged
merged 22 commits into from
Oct 28, 2024
Merged

Refactor QuarkBuilder and Bridge routing fix #93

merged 22 commits into from
Oct 28, 2024

Conversation

cwang25
Copy link
Contributor

@cwang25 cwang25 commented Oct 23, 2024

  • Trim off repetitive codes from quark scripts into individual files for ease of maintainability and readability.
  • Adjusted some test assertion on revert that was different due to when people copying codes in quark scripts people changed a bit here and there causing some discrepancies on some check logics (such as check sufficient payment token, check sufficient bridges...etc) Now with shared base code, it will always goes through the exact same checks and codes
  • Bug fix to bridge routing logics. Old bridge logics will try to bridge 0 balances if multiple accounts appears in the list. Now it won't try to bridge zero balance assets. (Found from after adding all alice/bob/fella accounts across all chain-id.

@cwang25 cwang25 marked this pull request as ready for review October 23, 2024 08:06
@cwang25 cwang25 requested a review from kevincheng96 October 23, 2024 17:07
Copy link
Contributor

@fluffywaffles fluffywaffles left a comment

Choose a reason for hiding this comment

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

Did not take a super close look, but seems like decent improvement!

Probably would like to see a 🥈 or ⛑️ with more overall familiarity take a look to give an official ✅.


// Then, transfer `amount` of `assetSymbol` to `recipient`
(IQuarkWallet.QuarkOperation memory operation, Actions.Action memory action) = Actions.transferAsset(
Actions.TransferAsset({
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: if the function is Actions.transferAsset, maybe the struct should be Actions.TransferAssetParams or like?

a little ambiguous as-is, but of course not a big deal; not blocking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds good to me. But I do see similar patterns in other actions too. Maybe I will create a quick follow-up PR to change all patterns to XXXParams 👍

src/builder/scripts/TransferBuilderScripts.sol Outdated Show resolved Hide resolved
src/builder/scripts/TransferBuilderScripts.sol Outdated Show resolved Hide resolved
src/builder/scripts/SwapBuilderScripts.sol Outdated Show resolved Hide resolved
src/builder/scripts/SwapBuilderScripts.sol Outdated Show resolved Hide resolved
Copy link
Contributor

@kevincheng96 kevincheng96 left a comment

Choose a reason for hiding this comment

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

Nice work on this!

src/builder/QuarkBuilderBase.sol Outdated Show resolved Hide resolved
src/builder/scripts/CometBuilderScripts.sol Outdated Show resolved Hide resolved
test/builder/QuarkBuilderCometWithdraw.t.sol Show resolved Hide resolved
test/builder/lib/QuarkBuilderTest.sol Outdated Show resolved Hide resolved
test/builder/lib/QuarkBuilderTest.sol Outdated Show resolved Hide resolved
test/builder/lib/QuarkBuilderTest.sol Outdated Show resolved Hide resolved
src/builder/QuarkBuilderBase.sol Outdated Show resolved Hide resolved
@kevincheng96 kevincheng96 changed the title Refactor on quark scripts Refactor QuarkBuilder Oct 24, 2024
@cwang25 cwang25 changed the title Refactor QuarkBuilder Refactor QuarkBuilder and Bridge routing fix Oct 25, 2024
src/builder/EIP712Helper.sol Outdated Show resolved Hide resolved
test/builder/lib/QuarkBuilderTest.sol Outdated Show resolved Hide resolved
test/builder/lib/QuarkBuilderTest.sol Outdated Show resolved Hide resolved
src/builder/actions/CometActionsBuilder.sol Outdated Show resolved Hide resolved
src/builder/QuarkBuilderBase.sol Outdated Show resolved Hide resolved
src/builder/QuarkBuilderBase.sol Outdated Show resolved Hide resolved
src/builder/QuarkBuilderBase.sol Show resolved Hide resolved
src/builder/QuarkBuilderBase.sol Show resolved Hide resolved
src/builder/QuarkBuilderBase.sol Outdated Show resolved Hide resolved
@cwang25 cwang25 requested a review from kevincheng96 October 25, 2024 22:00
Copy link
Contributor

@kevincheng96 kevincheng96 left a comment

Choose a reason for hiding this comment

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

LGTM. I left a few suggestions to reword some comments

src/builder/QuarkBuilderBase.sol Outdated Show resolved Hide resolved
src/builder/QuarkBuilderBase.sol Outdated Show resolved Hide resolved
cwang25 and others added 2 commits October 28, 2024 10:57
Co-authored-by: Kevin Cheng <kevincheng96@hotmail.com>
Co-authored-by: Kevin Cheng <kevincheng96@hotmail.com>
@cwang25 cwang25 merged commit 4a9f282 into main Oct 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants