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(inter-protocol): add amm liquidity script #5938

Merged
merged 3 commits into from
Aug 12, 2022

Conversation

samsiegart
Copy link
Contributor

refs: #5354

@samsiegart samsiegart marked this pull request as ready for review August 12, 2022 03:18
@samsiegart samsiegart requested a review from turadg as a code owner August 12, 2022 03:18
Copy link
Member

@turadg turadg left a comment

Choose a reason for hiding this comment

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

I noted some non-blocking requests. I this were production code I'd Request Changes but since this is just utility a script and it's in a fast-moving part of the codebase, I'll approve and leave updates to your discretion.

Comment on lines 16 to 17
// /** @type {<K extends string,V>(es: [K,V][]) => Record<K,V>} */
// const recordFromEntries = Object.fromEntries;
Copy link
Member

Choose a reason for hiding this comment

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

remove dead code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

// /** @type {<K extends string,V>(es: [K,V][]) => Record<K,V>} */
// const recordFromEntries = Object.fromEntries;

const getOfferResult = async (instance, wallet, walletAdmin) => {
Copy link
Member

Choose a reason for hiding this comment

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

consider adding types

let result;
console.log('awaiting offers...');
for await (const offers of makeNotifierFromAsyncIterable(offerIt)) {
console.log('offers iterator:', offers);
Copy link
Member

Choose a reason for hiding this comment

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

this is a next of the iterator, not the iterator itself

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

status === 'complete' && invitationQuery.instance === instance,
)
.forEach(async ({ id }) => {
result = await E(wallet).lookup('offerResult', id);
Copy link
Member

Choose a reason for hiding this comment

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

should this really run for each offer? seems like you want to .find the first match. In which case you don't need a separate filter.

result = offers.find( )
if (result) break;


/**
*
* @param { any } homeP
Copy link
Member

Choose a reason for hiding this comment

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

not blocking, but do we have a type for this somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It used to be import('./user-bundle').HomeBundle which was unresolved, not sure where it came from. And I couldn't find anything else in agoric-sdk that defines a proper type for homeP

Comment on lines 50 to 53
// const iNot = E(wb).getIssuersNotifier();
// const { value: iEntries } = await E(iNot).getUpdateSince();
// const issuers = recordFromEntries(iEntries);
// console.log(issuers);
Copy link
Member

Choose a reason for hiding this comment

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

dead code. if it's meant to be used for additional diagnostics, put it behind a flag to enable such diagnostics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

const offerConfig = {
id: `${now()}`,
invitation,
// installationHandle: amm.installation,
Copy link
Member

Choose a reason for hiding this comment

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

dead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@samsiegart
Copy link
Contributor Author

I noted some non-blocking requests. I this were production code I'd Request Changes but since this is just utility a script and it's in a fast-moving part of the codebase, I'll approve and leave updates to your discretion.

Thanks, yea I'd rather just move forward with the next PR, it takes significant time to start a local chain and test this script from scratch.

@samsiegart samsiegart added the automerge:squash Automatically squash merge label Aug 12, 2022
@dckc
Copy link
Member

dckc commented Aug 12, 2022

@mergify mergify bot merged commit d0f0f7a into master Aug 12, 2022
@mergify mergify bot deleted the add-initial-liquidity-script branch August 12, 2022 19:53
turadg pushed a commit that referenced this pull request Aug 12, 2022
* feat(inter-protocol): add amm liquidity script

* fix: remove dead code

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants