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

Query Improvements - Making the build queries interface easier to use #473

Merged
merged 10 commits into from
Jul 10, 2023

Conversation

lgahdl
Copy link
Contributor

@lgahdl lgahdl commented Jun 14, 2023

  • Doesn't need to send all amounts In/Out for all tokens, only the tokens that will be used;

- Doesn't need to send all amounts for all tokens, only the tokens that will be used;
@lgahdl lgahdl marked this pull request as draft June 14, 2023 23:41
Copy link
Member

@johngrantuk johngrantuk left a comment

Choose a reason for hiding this comment

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

balancer-js/examples/pools/queries.ts needs updated.

Copy link
Contributor

@gmbronco gmbronco left a comment

Choose a reason for hiding this comment

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

How about instead of separate tokens and amounts we change the interface to accept a map? It might be easier to construct and avoids problems with accidental misattribution by ordering the arrays wrong.

{ [address: string]: string }

the same for return data. we could format results to a map, so it's easy to read / parse

@lgahdl lgahdl marked this pull request as ready for review June 16, 2023 20:05
@lgahdl lgahdl self-assigned this Jun 16, 2023
@lgahdl lgahdl requested a review from johngrantuk June 21, 2023 20:39
@johngrantuk
Copy link
Member

I might be wrong but I think @gmbronco was suggesting we change the function input params to be a map of tokens addresses and amounts?

@lgahdl
Copy link
Contributor Author

lgahdl commented Jul 7, 2023

I might be wrong but I think @gmbronco was suggesting we change the function input params to be a map of tokens addresses and amounts?

You're right, sorry, will fix that

@lgahdl lgahdl requested a review from gmbronco July 7, 2023 19:11
Comment on lines +43 to 50
let maxInWithoutBpt;

// Remove BPT token from amounts for user data
if (bptIndex > -1) {
maxInWithoutBpt = removeItem(maxAmountsIn, bptIndex);
} else {
maxInWithoutBpt = maxAmountsIn;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let maxInWithoutBpt;
// Remove BPT token from amounts for user data
if (bptIndex > -1) {
maxInWithoutBpt = removeItem(maxAmountsIn, bptIndex);
} else {
maxInWithoutBpt = maxAmountsIn;
}
let maxInWithoutBpt = maxAmountsIn;
// Remove BPT token from amounts for user data
if (bptIndex > -1) {
maxInWithoutBpt = removeItem(maxAmountsIn, bptIndex);
}

@lgahdl lgahdl dismissed johngrantuk’s stale review July 10, 2023 18:56

The changes are made and John is on vacation, I'll dismiss the review so I can merge it

@lgahdl lgahdl merged commit 9ac85d1 into develop Jul 10, 2023
3 checks passed
@johngrantuk johngrantuk mentioned this pull request Jul 28, 2023
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.

3 participants