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

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

Conversation

SGiaccobasso
Copy link
Collaborator

No description provided.

@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.54%. Comparing base (7c9f527) to head (68ac977).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #186      +/-   ##
==========================================
- Coverage   99.86%   98.54%   -1.33%     
==========================================
  Files          48       48              
  Lines         756      756              
  Branches      157      157              
==========================================
- Hits          755      745      -10     
  Misses          1        1              
- Partials        0       10      +10     
Flag Coverage Δ
?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ 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

  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
canhtrinh and others added 3 commits September 17, 2024 21:59
Co-authored-by: Milap Sheth <milap@interoplabs.io>
Co-authored-by: Milap Sheth <milap@interoplabs.io>
Co-authored-by: Milap Sheth <milap@interoplabs.io>
@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
npty and others added 8 commits October 2, 2024 19:08
Co-authored-by: Milap Sheth <milap@interoplabs.io>
Co-authored-by: Milap Sheth <milap@interoplabs.io>
Co-authored-by: Milap Sheth <milap@interoplabs.io>
Co-authored-by: Milap Sheth <milap@interoplabs.io>
Co-authored-by: Milap Sheth <milap@interoplabs.io>
@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?

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