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

feat: create gmp tracker event interface #186

Merged
merged 23 commits into from
Oct 15, 2024

Conversation

SGiaccobasso
Copy link
Collaborator

@SGiaccobasso SGiaccobasso commented Sep 10, 2024

@SGiaccobasso SGiaccobasso requested a review from a team as a code owner September 10, 2024 13:22
@codecov-commenter
Copy link

codecov-commenter commented Sep 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.66%. Comparing base (4e3ebe5) to head (8d842d5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #186   +/-   ##
=======================================
  Coverage   98.66%   98.66%           
=======================================
  Files          47       47           
  Lines         747      747           
  Branches      154      154           
=======================================
  Hits          737      737           
  Misses          1        1           
  Partials        9        9           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@ecdsafu ecdsafu left a comment

Choose a reason for hiding this comment

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

  1. For our RFQ, sender won't be on the chain that this event is being emitted, the sender of the transaction being emitted will just be a relayer for our settlement process, so this should be a string. Could be a sender from Sui, Sol, or Cosmos.
  2. We don't have decimals available, so wouldn't be able to emit that.
  3. Swaps have a fromToken and toToken, so a bridge amount and single tokenAddress is less relevant here. Better to have fromToken, fromAmount, toToken and toAmount for us. We could arbitrarily pick the destination token if you want just one.

@canhtrinh
Copy link
Contributor

  1. For our RFQ, sender won't be on the chain that this event is being emitted, the sender of the transaction being emitted will just be a relayer for our settlement process, so this should be a string. Could be a sender from Sui, Sol, or Cosmos.
    => Thanks, yeah we'll change to a string
  1. We don't have decimals available, so wouldn't be able to emit that.
    => How do you represent amounts then?
  1. Swaps have a fromToken and toToken, so a bridge amount and single tokenAddress is less relevant here. Better to have fromToken, fromAmount, toToken and toAmount for us. We could arbitrarily pick the destination token if you want just one.
    => Hmm, yeah we just want one. I'll want some team buy-in, but perhaps we can accept both from and to so you don't have to pick

@ecdsafu
Copy link

ecdsafu commented Sep 17, 2024

  1. For our RFQ, sender won't be on the chain that this event is being emitted, the sender of the transaction being emitted will just be a relayer for our settlement process, so this should be a string. Could be a sender from Sui, Sol, or Cosmos.

=> Thanks, yeah we'll change to a string

  1. We don't have decimals available, so wouldn't be able to emit that.

=> How do you represent amounts then?

  1. Swaps have a fromToken and toToken, so a bridge amount and single tokenAddress is less relevant here. Better to have fromToken, fromAmount, toToken and toAmount for us. We could arbitrarily pick the destination token if you want just one.

=> Hmm, yeah we just want one. I'll want some team buy-in, but perhaps we can accept both from and to so you don't have to pick

  1. Sounds good, thanks

  2. We have off chain mappings for the token/chain/decimals and amount is always represented in the decimals of that token/chain

  3. Ok sounds good, if you just want one we can choose only one, will leave it with you if you want to add the second token or not

contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
@SGiaccobasso SGiaccobasso force-pushed the feat/gmp-tracker-event-interface branch from b65ff0c to 154ee3e Compare September 20, 2024 17:41
@SGiaccobasso SGiaccobasso force-pushed the feat/gmp-tracker-event-interface branch from 154ee3e to 586d3fd Compare September 20, 2024 17:41
@ecdsafu
Copy link

ecdsafu commented Sep 24, 2024

Looking good, for our RFQ we will always emit from source on the hub chain. If you add decimals back in, we won't be able to support anyway and would leave at 0

contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/interfaces/IAxelarGMPTransfer.sol Outdated Show resolved Hide resolved
contracts/test/executable/GMPExecutableWithTokenTest.sol Outdated Show resolved Hide resolved
contracts/test/executable/GMPExecutableWithTokenTest.sol Outdated Show resolved Hide resolved
@ecdsafu
Copy link

ecdsafu commented Oct 2, 2024

Hey guys, are these final and good to go into our mainnet contracts? We're deploying soon

@ecdsafu
Copy link

ecdsafu commented Oct 3, 2024

Hey guys, just thought of an issue with this approach. Since all events will be emitted from our hub chain, there is no way to express the fromChain for the order if we use the InterchainTokenSent event. You will have the toChain in the event, but not know what the fromChain is. If you're assuming the fromChain is the chain where the event is emitted, this will be incorrect. Every order will appear to be coming from our hub chain.

Would also be helpful to know what UI this is going to be displayed on. Will each event be on axelarscan as a bridge tx with a fromChain toChain and amount?

@milapsheth milapsheth enabled auto-merge (squash) October 15, 2024 22:16
@milapsheth milapsheth merged commit 2472998 into main Oct 15, 2024
5 checks passed
@milapsheth milapsheth deleted the feat/gmp-tracker-event-interface branch October 15, 2024 22:18
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.

7 participants