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

Support string input for SOR public functions #60

Closed
wants to merge 5 commits into from

Conversation

danielmkm
Copy link
Collaborator

No description provided.

@mikemcdonald
Copy link
Member

I'm fine with the concept, but the way it's set up now will always fail the first time, no?

Since the tokenMap isn't populated until fetchAndCachePools which is called after getRequiredToken in getSwaps

@mikemcdonald
Copy link
Member

Hmm actually I don't see a good way this fits in without more tightly coupling subgraph as a data provider which I'd like to avoid. The alternative is always doing onchain calls for decimals and symbol but then that's introducing unnecessary latency and the average user might default to using strings for token inputs without understanding this.

Unless there's a way around those two issues, I think it's best to be more explicit here

@danielmkm
Copy link
Collaborator Author

danielmkm commented Apr 6, 2023

I'm fine with the concept, but the way it's set up now will always fail the first time, no?

Since the tokenMap isn't populated until fetchAndCachePools which is called after getRequiredToken in getSwaps

Totally right, did it pretty quickly, will fix this if we decide to use it.

Hmm actually I don't see a good way this fits in without more tightly coupling subgraph as a data provider which I'd like to avoid. The alternative is always doing onchain calls for decimals and symbol but then that's introducing unnecessary latency and the average user might default to using strings for token inputs without understanding this.

Unless there's a way around those two issues, I think it's best to be more explicit here

Not entirely sure that's the case if i understand your point correctly. The SOR will always have a set of pools to route through (regardless of where they're sourced from), the pools should always have TokenAmounts and currently decimals is a required field on Token. So broadly speaking, given a set of pools you should always be able to determine the unique set tokens available to route through.

@mikemcdonald
Copy link
Member

That's true. I'm still not seeing a super clean way to load all of the raw data before the token checks. Ex: if a Token type was passed, would then need to convert to a string to pass to getCandidatePaths which feels wrong

@danielmkm
Copy link
Collaborator Author

What about this? still a bit funky since fetchPoolsIfNotCached is called twice on a call to getSwaps

@mikemcdonald
Copy link
Member

Ehh I know I'm being kinda difficult, but that also feels weird to add. I'm going to leave as is for now and if other integrators have the same request / frustration we can easily add this in. Since it won't be a breaking change

@danielmkm
Copy link
Collaborator Author

No worries at all man, it turned out to be kinda hacky in the end

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