-
Notifications
You must be signed in to change notification settings - Fork 373
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
SDK #1347
SDK #1347
Conversation
✅ Deploy Preview for vigilant-albattani-df38ec ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Deploying gmx-interface with
|
Latest commit: |
e244790
|
Status: | ✅ Deploy successful! |
Preview URL: | https://2c446d89.gmx-interface.pages.dev |
Branch Preview URL: | https://sdk.gmx-interface.pages.dev |
74afc8e
to
6168717
Compare
13aba34
to
cb3051a
Compare
sdk/README.md
Outdated
oracleUrl: "https://arbitrum-api.gmxinfra.io", | ||
walletClient: useWallet().walletClient, | ||
subgraph: { | ||
subsquid: "https://gmx.squids.live/gmx-synthetics-arbitrum/graphql", |
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.
having subsquid inside subgraph seems incorrect
they are two parallel entities, both having equal role in our platform
idk yet if it's just README typo or intended that way
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.
Agree, I just copied structure of subgraph urls from UI config, but here it doesn't make sense, we use only subsquid in SDK, so I just rename the object-field to subsquidUrl
property
sdk/README.md
Outdated
marketInfo, | ||
indexToken: marketInfo.indexToken, | ||
increaseAmounts: { | ||
// amounts for position |
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.
would be nice to put all the numbers
also I don't see swapPath, I think it should be required parameter
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.
swapPathStats
may be undefined, i will add complete example
import { Address, zeroAddress } from "viem"; | ||
|
||
const CONTRACTS = { | ||
[ARBITRUM_GOERLI]: { |
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.
I think it's fine to remove ARBITRUM_GOERLI from all the configs, as we in fact don't support it
it might be confusing for users if they would go with it without any effect
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.
Should we remove this in UI too? If so, I think it's better to remove it from everywhere in single and separate PR
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.
I think it just confuses potential sdk users that we support goerli, but we don't
@@ -0,0 +1,536 @@ | |||
/* |
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.
let's update our listing checklist so that we add new markets to sdk as well (even if it's in PR)
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.
Draft is here: https://www.notion.so/gmxio/New-token-market-checklist-pre-SDK-14b03574745d802c962ccb54c02a5964, move to main checklist once we merge
expect(response).toBeDefined(); | ||
}, 30_000); | ||
}); | ||
}); |
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.
can you add a test to check that all markets listed in sdk config are same as in frontend? otherwise it's hard to visually validate each of them, would also simplify publishing new markets
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.
It's not 100% possible as markets in SDK are fetched from chain and filtered by /markets
endpoint
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.
maybe not in tests then,
you build markets map in src/config/static/markets.ts
, I guess you can throw some error there in case market for any chain is not listed in src/config/markets
(I might confused some paths you should get the idea)
sdk/src/modules/oracle.ts
Outdated
.catch((e) => { | ||
// eslint-disable-next-line no-console | ||
console.error(e); | ||
throw e; |
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.
does it make sense to log an error?
also let's be consistent, getGovTokenDelegates doesn't log error but here we do.
I think it's fine to ignore it, user would anyway get it?
marketsInfoData, | ||
tokensData, | ||
orderTypesFilter = [], | ||
marketsDirectionsFilter = [], |
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.
let's allow sending account in params?
}, {} as OrdersInfoData); | ||
|
||
return { | ||
count: orders.count, |
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.
what is purpose of returning orders.count if ordersInfoData may potentially not contain all orders?
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.
we specifically request this field https://github.com/gmx-io/gmx-interface/blob/sdk/sdk/src/modules/orders/utils.ts#L166
Same usage in UI, I thought it's better to keep it
|
||
return { | ||
count: orders.count, | ||
ordersInfoData, |
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.
orders: ordersInfoData
or orders: Object.values(ordersInfoData)
?
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.
elsewhere it's entityInfoData
, why rename only here?
sdk/src/utils/orders.ts
Outdated
// minOutputAmount: order.minOutputAmount, | ||
// initialCollateralAmount: order.initialCollateralDeltaAmount, | ||
// }); | ||
const title = "TODO"; |
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.
maybe remove this field for now or put ""
?
sdk/src/modules/orders/orders.ts
Outdated
account, | ||
collateralAddress: collateralToken.address, | ||
}) | ||
) as PositionOrderInfo[]; |
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.
let's make isOrderForPositionByData
(and others where applicable) a typeguard?
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.
Hmm, it's already a typeguard. I just remove typecast
}, | ||
"include": ["src", "scripts"], | ||
"references": [{ "path": "./sdk" }], | ||
"include": ["src", "scripts"] |
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.
what do you think of restrictring imports from sdk by UI?
with all the duplicated files I think it might be an issue
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.
but we're planning remove duplicates from UI where it applicable, sdk
is available in UI as a project reference, hence we can keep static common stuff in sdk and in UI only update imports (like import DataStore from 'sdk/abis/DataStore.json"
)
clearTimeout(this.fallbackTimerId); | ||
} | ||
} | ||
} |
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.
this file not in use?
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.
correct, removing
sdk/src/utils/objects.ts
Outdated
return obj[key]; | ||
} | ||
|
||
export function getMatchingValueFromObject( |
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.
not used (I know there're many unused stuff but I only comment big ones)
return 3n + BigInt(swapsCount); | ||
} | ||
|
||
export function estimateOrderOraclePriceCount(swapsCount: number): bigint { |
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.
only this function is in use, do you think it makes sense to keep the rest functions?
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.
Eventually the rest will be here, so I think it's ok to move fees utils files entirely
export const HIGH_POSITION_IMPACT_BPS = 50; // 0.5% | ||
export const HIGH_COLLATERAL_IMPACT_BPS = 500; // 5% | ||
export const HIGH_SWAP_IMPACT_BPS = 50; // 0.5% | ||
export const DEFAULT_ACCEPABLE_PRICE_IMPACT_BUFFER = 30; // 0.3% |
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.
only
export const USD_DECIMALS = 30;
export const BASIS_POINTS_DIVISOR = 10000;
export const BASIS_POINTS_DIVISOR_BIGINT = 10000n;
are used, let's remove the rest?
I think it's good to keep configs clean
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.
Seems that we have redudant abis in SDK, probably remove what we don't use in V2?
{ | ||
name: "GMX LP", | ||
symbol: "GLP", | ||
address: "", |
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.
We discussed that tokens and markets config should be an external params to SDK, or probably i don't understand the current concept
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.
Those are defaults, it think it's better to keep configs in one place so in future I'm intended to remove duplicates from UI
Notes
sdk/src/modules
sdk/src/configs/*
- this is mostly copypaste from configs in UI, it's intended to use those files in UI too (like replaceimport {getToken} from 'config/tokens';
toimport {getToken} from 'sdk/configs/tokens';
sdk/src/types/*
- types are copied from UI, except SDK-specific typessdk/src/utils/*
- most utils copied from UI