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

[WIP] Unify pool state #383

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

[WIP] Unify pool state #383

wants to merge 11 commits into from

Conversation

wormat
Copy link
Collaborator

@wormat wormat commented Sep 15, 2022

A step towards creating a generic Client interface. I'd be interested to hear suggestions on how to combine this approach with the current usage of React query/caching.

Checklist

  • Github project and label have been assigned
  • Tests have been added or are unnecessary
  • Docs have been updated or are unnecessary
  • Preview deployment works (visit the Cloudflare preview URL)
  • Manual testing in #product-testing completed or unnecessary
  • New i18n strings do not contain any security vulnerabilities (e.g. should not allow translator to update email/url)
  • When fetching new translations from external sources, do a sanity check (e.g. get people who speak the language to check, shove all new translations into Google translate)

@wormat wormat added refactoring feature New feature or request labels Sep 15, 2022
@github-actions
Copy link

github-actions bot commented Sep 15, 2022

✨ Deployment complete! Take a peek over at https://8fa2bafb.ui-storybook.pages.dev

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Sep 15, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 29ac2a8
Status: ✅  Deploy successful!
Preview URL: https://ff7a4d70.swim-ui.pages.dev
Branch Preview URL: https://sdk-unify-pool-state.swim-ui.pages.dev

View logs

@wormat wormat force-pushed the sdk/unify-pool-state branch 3 times, most recently from 3463cac to 2d74210 Compare September 20, 2022 15:28
swimthor
swimthor previously approved these changes Sep 21, 2022
@wormat wormat changed the title Unify pool state [WIP] Unify pool state Sep 21, 2022
@wormat wormat force-pushed the sdk/unify-pool-state branch 2 times, most recently from 8c58bc9 to 1b6c2a3 Compare October 25, 2022 16:18
Comment on lines 35 to 59
interface DecimalBN {
readonly value: BN;
readonly decimals: number;
}

const decimal = (property = "decimal"): Layout<DecimalBN> =>
struct([u64("value"), u8("decimals")], property);

interface AmpFactor {
readonly initialValue: DecimalBN;
readonly initialTs: BN;
readonly targetValue: DecimalBN;
readonly targetTs: BN;
}

const ampFactor = (property = "ampFactor"): Layout<AmpFactor> =>
struct(
[
decimal("initialValue"),
i64("initialTs"),
decimal("targetValue"),
i64("targetTs"),
],
property,
);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@nicomiicro I had to copy-paste these from the @swim-io/solana package because otherwise borsh complained that they weren't instances of Layout. Any ideas why? I thought the peer dependencies adjustment was supposed to prevent this kind of issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't get a type error locally 🤔
I pushed 14b5fe3
Let's see the output of the CI https://github.com/swim-io/swim/actions/runs/3323839509/jobs/5494688131

Copy link
Contributor

Choose a reason for hiding this comment

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

oh now I see it, it's a runtime error. Checking...

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yes, it's just because you are using the package from the workspace and not a published one. Thus it will load the one from its own node_modules. This should go away once it gets published.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another (good) reason to move to pre-releases for PRs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ahh. Hmm, I don't really want to have to publish twice as often as I do currently.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah I meant when we automate this...

Copy link
Contributor

@nicomiicro nicomiicro left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of questions

apps/ui/src/models/swim/pool.ts Show resolved Hide resolved
({ id }) => id === lpTokenId,
);
const poolTokens = tokenIds.map((tokenId) =>
findOrThrow(this.chainConfig.tokens, ({ id }) => id === tokenId),
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this have the same issue I had with swimUSD not being present in this tokens list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, nice catch, thanks!

Copy link
Collaborator Author

@wormat wormat Oct 26, 2022

Choose a reason for hiding this comment

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

const poolTokenDetails = tokenIds.map((tokenId) => {
const tokenConfig = findOrThrow(
this.chainConfig.tokens,
(token) => token.id === tokenId,
Copy link
Contributor

Choose a reason for hiding this comment

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

same question about swimUSD

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants