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

chore: upgrade unified bridge SDK #40

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open

chore: upgrade unified bridge SDK #40

wants to merge 19 commits into from

Conversation

meeh0w
Copy link
Member

@meeh0w meeh0w commented Sep 10, 2024

Description

TODOs

  • Release @avalabs/bridge-unified to a stable channel

Changes

  • Upgrades @avalabs/bridge-unified dependency
    • The new version includes some breaking changes. This PR updates our code to make it compatible.
  • Refactors how we "recognise" CCTP (and the upcoming ICTT) bridge transactions - previously we would look at UnifiedBridge config to fetch the addresses, with this PR the updated SDK exposes the analyzeTx() method that we can use.
  • Implements a schema migration for any pending bridge transfers (the BridgeTransfer object changed its shape, some properties changed names and/or places)

Testing

  • Test bridging. It should work exactly the same as it used to.
  • Run sanity checks on other parts of the extension
  • Bonus points for simulating the extension update (initiate bridging from main, switch to this branch and verify that your transaction is still tracked properly).

Screenshots:

Starts bridging via CCTP on main, simulates the extension update, then starts another bridge with the "updated" extension:

CCTP.after.update.-.compressed.mov

Checklist for the author

  • I've covered new/modified business logic with Jest test cases.
  • I've tested the changes myself before sending it to code review and QA.

gergelylovas
gergelylovas previously approved these changes Oct 3, 2024
Comment on lines 535 to 536
isBridgeTx,
getAssetIdentifierOnTargetChain,
Copy link
Contributor

Choose a reason for hiding this comment

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

should we add tests for these?

Copy link
Member Author

Choose a reason for hiding this comment

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

The getAssetIdentifierOnTargetChain will be removed as soon as ICTT is finalized, so I wouldn't bother personally.

isBridgeTx is now analyzeTx - I'll add tests in the upcoming PR (here: #57)

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.

2 participants