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.

Sign up for GitHub

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

Jump to bottom

Add Kado #186

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Add Kado #186

merged 1 commit into from
Oct 9, 2024
+180 −0

Conversation

paullinator
Copy link
Member

@paullinator paullinator commented Mar 9, 2024 •
edited
Loading

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Description

none

samholmes
samholmes approved these changes Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@samholmes samholmes left a comment

Choose a reason for hiding this comment

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

Approved, but I think cleanup by putting dead-code in a separate commit and separate wip-branch may be better then committing it now.

src/partners/kado.ts Outdated
Comment on lines 64 to 67
// const statusMap: { [key in PartnerStatuses]: Status } = {
// DONE: 'complete',
// other: 'other'
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Commented out code meant for a later PR makes it very hard to review because I don't know if this is accidental or not. Can you stash these commented out portions in a local stash or branch?

src/partners/kado.ts Outdated
// break
}
}
// await snooze(3000)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Accidental or for a later PR?

src/partners/kado.ts Outdated

const MAX_RETRIES = 5
const QUERY_LOOKBACK = 1000 * 60 * 60 * 24 * 30 // 30 days
const QUERY_TIME_BLOCK_MS = QUERY_LOOKBACK
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the extra constant?

src/partners/kado.ts Outdated
Comment on lines 95 to 96
const startTimeS = startTime / 1000
const endTimeS = endTime / 1000
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does this not give you unused variable errors?

src/partners/kado.ts Outdated
Comment on lines 89 to 91
// No looping since all txs are returned in one call for now
// while (true)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just make a separate commit for the while-loop and time variables which can be in a separate wip-branch?

@paullinator
Copy link
Member Author

@samholmes since this code is already deployed in develop, i'll see if they update the API in the near future and I'll just modify this PR for the new API. Otherwise if I get impatient, I'll pull out the dead code.

@paullinator paullinator force-pushed the paul/addKado branch 3 times, most recently from 502433b to 93458a1 Compare April 6, 2024 05:48
@paullinator paullinator force-pushed the paul/addKado branch 2 times, most recently from 9cbd4b6 to bb51763 Compare October 8, 2024 19:56
@paullinator paullinator merged commit f4df7ab into master Oct 9, 2024
1 check passed
@paullinator paullinator deleted the paul/addKado branch October 9, 2024 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samholmes samholmes samholmes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@paullinator @samholmes