algorandfoundation / algokit-utils-ts

MIT License
19 stars 8 forks source link

Tweaks to AlgorandClient: #248

Closed robdmoore closed 8 months ago

robdmoore commented 8 months ago

There is a txn dead error in the tests to do with valid round calculation that needs to be resolved and I haven't made any changes to composer yet, but there are some I want to suggest, this covers everything else though.

joe-p commented 8 months ago

All looks good to me but ditching bigint does make me nervous. The SDK is going to all bigint for good reason imo. The more I think about I think having just bigint for all the numeric types makes the most sense (not even number | bigint). Most basic reasoning being that it's more representative of the actual data type. Also, while it's true that given how things currently work it's unlikely we'll need it, there could be changes in the future that would end up breaking deployed code (for example, supply cap increase or changes to how app/asset IDs are derived). There also could be forks of Algorand that would want to use this library.

Also having some places where bigint is used and some places where its not for the same underlying data type (uint64) can be a bit confusing

There is a txn dead error in the tests to do with valid round calculation that needs to be resolved

Presumably you fixed this? Tests seem to be passing for me.

robdmoore commented 8 months ago

bigints are painful to work with, my vote is for using number for things we know will never be > 2^53, but happy to follow consensus from broader group of stakeholders.

The test error I get is URLTokenBaseHTTPError: Network request error. Received status 400 (Bad Request): TransactionPool.Remember: txn dead: round 418 outside of 397--407. As far as I can tell a transaction within a group is getting conflicting rounds.

joe-p commented 8 months ago

bigints are painful to work with, my vote is for using number for things we know will never be > 2^53, but happy to follow consensus from broader group of stakeholders.

Consensus on devrel team was that it was better to use bigint everywhere. I agree they can be annoying, especially when working with numbers, but if everything is a bigint this should really be a problem. It also makes behavior much more predictable knowing most numbers should be bigints

The test error I get is URLTokenBaseHTTPError: Network request error. Received status 400 (Bad Request): TransactionPool.Remember: txn dead: round 418 outside of 397--407. As far as I can tell a transaction within a group is getting conflicting rounds.

Ah ok I was able to reproduce. Previously I was just running the client test by itself which was passing. When you run the full suite you get tests in parallel which affect the blocks. We just need to provide a wider window for testing.

robdmoore commented 8 months ago

Cool, can I make a suggestion that when receiving data we accept number | bigint, but when relaying information we provide bigint. I guess the problem may occur where we have a field that can be both though so if there are cases like that it may be better to just be consistent as annoying as bigint's are to deal with?

joe-p commented 8 months ago

I made the change to just bigint. If it becomes a source of a lot of developer problems then we can re-evaluate and add a number union. I think it's easier to add a number union than to remove it.

I'll be merging this PR and recording bootcamp content with the current state of the base branch.