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

fix: resource population bugs #278

Merged
merged 8 commits into from
May 21, 2024
Merged

fix: resource population bugs #278

merged 8 commits into from
May 21, 2024

Conversation

joe-p
Copy link
Contributor

@joe-p joe-p commented May 20, 2024

Proposed Changes

Fixes two bugs on resource population

  1. Pushing a resource to an undefined array doesn't actually work

  2. Box references weren't checking if the app was available

@joe-p joe-p changed the title Fix/resource_pop_bugs fix: resource population bugs May 20, 2024
@joe-p
Copy link
Contributor Author

joe-p commented May 20, 2024

@robdmoore We're seeing the indexer tests timeout and I'm also seeing the same locally since I've updated my localnet. I know we've talked about this before but forget what the reasoning was and how we might reasonably fix this

Copy link

@kylebeee kylebeee 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, keep in mind i'm missing most of the context of these changes. The changes themselves look good though

src/transaction/transaction.ts Outdated Show resolved Hide resolved
@robdmoore
Copy link
Contributor

robdmoore commented May 21, 2024

There's some quite complex logic here that it may make sense to have test coverage for, but on the other hand I know this logic will move to algod at some point so probably time better spent adding test coverage there when that happens?

src/transaction/transaction.ts Show resolved Hide resolved
src/transaction/transaction.ts Outdated Show resolved Hide resolved
@robdmoore
Copy link
Contributor

@joe-p algorand/indexer#1613

@joe-p joe-p force-pushed the fix/resource_pop_bugs branch from 41bbfbe to 1d2ce1f Compare May 21, 2024 10:08
@joe-p
Copy link
Contributor Author

joe-p commented May 21, 2024

There's some quite complex logic here that it may make sense to have test coverage for, but on the other hand I know this logic will move to algod at some point so probably time better spent adding test coverage there when that happens?

Yeah that's exactly what I'm thinking. Will start algod effort this week so didn't want to spend time writing the tests here. I've confirmed that this works in the reti ui.

@joe-p joe-p merged commit 383ebaa into main May 21, 2024
2 checks passed
@robdmoore robdmoore deleted the fix/resource_pop_bugs branch May 21, 2024 10:47
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.

4 participants