-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
cwang25
commented
Oct 23, 2024
•
edited
Loading
edited
- 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.
There was a problem hiding this 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({ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
👍
There was a problem hiding this 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!
…ling only 1 accounts, when have multiple it will brdige all 0 balance accounts
There was a problem hiding this 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
Co-authored-by: Kevin Cheng <kevincheng96@hotmail.com>
Co-authored-by: Kevin Cheng <kevincheng96@hotmail.com>