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(explorer): use explored for the contracts route #761

Open
wants to merge 1 commit into
base: 11-13-refactor_explored-types_add_outputid_to_explorerfilecontract
Choose a base branch
from

Conversation

telestrial
Copy link
Contributor

@telestrial telestrial commented Oct 1, 2024

Questions/Points of Concern (Updated 10/23)

  • The exact timestamp for negotiation, expiration, proof deadline, and payout are a bit off from Sia Central, but I believe that's expected. We're doing this on the client side now with respective block heights. The difference is not significant but I don't know how exact these are expected to be. blockHeightToHumanDate in lib/contracts would be the relevant pipe they are all flowing through.

  • Payout is also a little different, too. I've never seen more that a few cents, but wanted to call that out, too. As far as I know, that's a difference on the explored side.

  • Do the resolved from/to buttons work as we would expect? I believe so but it's hard to find good test contracts that have both. I've only see one or the other. It shouldn't matter, but I like to visually/manually test as much as I can and couldn't find multiple contract renewals.

  • Does the status make sense (in progress, obligation successful, obligation failed)? This is another one where I'd ideally find one of every kind but that's a little more detailed understanding than I may have right now.

  • The network calls at the top of the Contract next page, their order, and their error handling.I may not have them all ordered the most optimal way and then: are we only returning notFound() on an absent contract record? There may be a better way to do all of this, but we have multiple chained calls that rely on previous.

Comparing to a contract page through Sia Central, we're very close, I think. I've left two review comments open down below with two other things to discuss.

Copy link

vercel bot commented Oct 1, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
explorer ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 16, 2024 1:25am
explorer-zen ✅ Ready (Inspect) Visit Preview 💬 2 unresolved Nov 16, 2024 1:25am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
hostd ⬜️ Ignored (Inspect) Visit Preview 💬 Add feedback Nov 16, 2024 1:25am
renterd ⬜️ Ignored (Inspect) Visit Preview 💬 Add feedback Nov 16, 2024 1:25am
website ⬜️ Ignored (Inspect) Visit Preview 💬 Add feedback Nov 16, 2024 1:25am

Copy link

changeset-bot bot commented Oct 1, 2024

🦋 Changeset detected

Latest commit: bac6213

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
explorer Minor
@siafoundation/explored-types Minor
@siafoundation/explored-js Patch
@siafoundation/explored-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor Author

telestrial commented Oct 1, 2024

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

This stack of pull requests is managed by Graphite. Learn more about stacking.

@telestrial telestrial force-pushed the 10-01-feat_explored-types_re-export_base_siacoinoutput_type branch from 60de770 to a8bc401 Compare October 7, 2024 20:08
@telestrial telestrial force-pushed the 10-01-feat_explorer_use_explored_for_the_contracts_route branch from 2da4c65 to 5eb9249 Compare October 7, 2024 20:08
@telestrial telestrial changed the base branch from 10-01-feat_explored-types_re-export_base_siacoinoutput_type to 10-07-refactor_explored-types_add_null_case_to_proof_fields_in_explorerfilecontract_type October 7, 2024 20:08
Copy link
Member

@alexfreska alexfreska left a comment

Choose a reason for hiding this comment

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

The exact timestamp for negotiation, expiration, proof deadline, and payout are a bit off from Sia Central

For all these slight differences, could you attach a screenshot of a contract with the before and after, and we can have everyone double check that the differences are within reason.

Do the resolved from/to buttons work as we would expect? I believe so but it's hard to find good test contracts that have both. I've only see one or the other. It shouldn't matter, but I like to visually/manually test as much as I can and couldn't find multiple contract renewals.

The following is a chain of 3 contracts: https://zen.siascan.com/contract/6344385f4189f14c78c8b793b93a8afebe2d138668becd8efc2057cb0d141557 that you can use to add an e2e test that validates the changes in this PR. Grab the key IDs and info of each from the production explorer and write a test that navigates through the renewals and verifies it matches up.

Does the status make sense (in progress, obligation successful, obligation failed)? This is another one where I'd ideally find one of every kind but that's a little more detailed understanding than I may have right now.

Yes you should add a test for success and failed states, the above link includes a successful contract, lets have the team help find a failed example.

@telestrial telestrial changed the base branch from 10-24-refactor_explored-types_change_confirmationindex_and_confirmationtransactionid_to_possibly_be_null to graphite-base/761 October 25, 2024 15:48
@telestrial telestrial force-pushed the 10-01-feat_explorer_use_explored_for_the_contracts_route branch from c6ed08f to ddc9241 Compare October 25, 2024 15:49
@telestrial telestrial changed the base branch from graphite-base/761 to main October 25, 2024 15:49
@telestrial telestrial force-pushed the 10-01-feat_explorer_use_explored_for_the_contracts_route branch from ddc9241 to a362ad3 Compare October 25, 2024 15:49
@telestrial telestrial force-pushed the 10-01-feat_explorer_use_explored_for_the_contracts_route branch from a362ad3 to 44a3bd0 Compare November 6, 2024 15:51
@telestrial telestrial force-pushed the 10-01-feat_explorer_use_explored_for_the_contracts_route branch from 44a3bd0 to b3f543c Compare November 13, 2024 19:40
@telestrial telestrial changed the base branch from main to 11-13-refactor_explorer-e2e_adjust_address_amount_test November 13, 2024 19:40
@telestrial telestrial force-pushed the 11-13-refactor_explorer-e2e_adjust_address_amount_test branch from 056b87d to 4a6598e Compare November 13, 2024 20:40
@telestrial telestrial force-pushed the 10-01-feat_explorer_use_explored_for_the_contracts_route branch from b3f543c to bac6213 Compare November 13, 2024 20:40
@telestrial telestrial changed the base branch from 11-13-refactor_explorer-e2e_adjust_address_amount_test to 11-13-refactor_explored-types_add_outputid_to_explorerfilecontract November 13, 2024 20:40
const renewedTo = renewalTransaction?.storage_contracts?.[0]
if (formationTransactionError) throw formationTransactionError
if (!formationTransaction)
throw new Error('No formation transaction found in successful reqeust')
Copy link
Member

Choose a reason for hiding this comment

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

request

Copy link
Member

@n8maninger n8maninger left a comment

Choose a reason for hiding this comment

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

Couple of small UX thoughts, couple of spelling nits, but overall good work.

])
if (renewalTransactionError) throw renewalTransactionError
if (!renewalTransaction)
throw new Error('No renewal transaction found in successful reqeust')
Copy link
Member

Choose a reason for hiding this comment

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

request

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants