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

Fixed Compiled Issues in example/nodejs(escrow example) #127

Merged
merged 4 commits into from
Dec 13, 2023

Conversation

nhenin
Copy link
Collaborator

@nhenin nhenin commented Dec 13, 2023

Summary by CodeRabbit

  • Refactor

    • Streamlined address handling across various components, enhancing the consistency and reliability of address-related operations.
  • Bug Fixes

    • Addressed issues with address validation and representation to ensure accurate processing and display of address information.
  • Documentation

    • Updated documentation to reflect changes in address handling and validation mechanisms.
  • Chores

    • Removed deprecated address-related code to maintain a clean and efficient codebase.

Copy link
Contributor

coderabbitai bot commented Dec 13, 2023

Walkthrough

The codebase has undergone a refactoring that primarily involves the removal of the unAddressBech32 function and the AddressBech32 type across various modules. The AddressBech32Guard has been introduced, likely to enforce stricter type checking or validation. The changes streamline how addresses are handled, particularly in HTTP headers and internal functions, suggesting a move towards more direct and type-safe operations.

Changes

File(s) Summary
.../endpoints/collection.ts, .../transaction/endpoints/collection.ts Removed unAddressBech32 function; updated address handling for headers.
.../guards.ts, .../index.ts, .../rolesConfigurations.ts Removed AddressBech32 exports and references; introduced AddressBech32Guard.
.../details.ts Replaced AddressBech32 with AddressBech32Guard for type validation.
.../address.ts Updated AddressBech32 to AddressBech32Guard and related functions.
.../generic/contracts.ts, .../test/examples/swap.ada.token.e2e.spec.ts Removed unAddressBech32 function and its usage.
.../nodejs/index.ts Removed unAddressBech32 calls in SingleAddressWallet methods.

🐇✨
In the code where types entwine,
A rabbit hopped through, line by line.
unAddressBech32 fades away,
For AddressBech32Guard is here to stay! 🎉
🌟🐾

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on X ?


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • You can reply to a review comment made by CodeRabbit.
  • You can tag CodeRabbit on specific lines of code or files in the PR by tagging @coderabbitai in a comment.
  • You can tag @coderabbitai in a PR comment and ask one-off questions about the PR and the codebase. Use quoted replies to pass the context for follow-up questions.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

@nhenin nhenin force-pushed the revert-address branch 2 times, most recently from 8c59c46 to 407bf55 Compare December 13, 2023 08:36
@nhenin nhenin requested a review from hrajchert December 13, 2023 08:37
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ebf25e7 and 407bf55.
Files ignored due to filter (4)
  • examples/nodejs/package-lock.json
  • package-lock.json
  • packages/runtime/core/package.json
  • packages/runtime/core/src/tsconfig.json
Files selected for processing (13)
  • changelog.d/20231213_092859_nicolas.henin_revert_address.md (1 hunks)
  • examples/nodejs/src/escrow-flow.ts (2 hunks)
  • packages/runtime/client/rest/src/contract/endpoints/collection.ts (2 hunks)
  • packages/runtime/client/rest/src/contract/guards.ts (1 hunks)
  • packages/runtime/client/rest/src/contract/index.ts (1 hunks)
  • packages/runtime/client/rest/src/contract/rolesConfigurations.ts (2 hunks)
  • packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts (2 hunks)
  • packages/runtime/client/rest/src/payout/details.ts (2 hunks)
  • packages/runtime/client/rest/src/withdrawal/endpoints/collection.ts (2 hunks)
  • packages/runtime/core/src/address.ts (1 hunks)
  • packages/runtime/lifecycle/src/generic/contracts.ts (2 hunks)
  • packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (1 hunks)
  • packages/wallet/src/nodejs/index.ts (6 hunks)
Files skipped from review due to trivial changes (2)
  • changelog.d/20231213_092859_nicolas.henin_revert_address.md
  • packages/runtime/client/rest/src/contract/guards.ts
Files skipped from review as they are similar to previous changes (9)
  • packages/runtime/client/rest/src/contract/endpoints/collection.ts
  • packages/runtime/client/rest/src/contract/index.ts
  • packages/runtime/client/rest/src/contract/rolesConfigurations.ts
  • packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts
  • packages/runtime/client/rest/src/payout/details.ts
  • packages/runtime/client/rest/src/withdrawal/endpoints/collection.ts
  • packages/runtime/lifecycle/src/generic/contracts.ts
  • packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts
  • packages/wallet/src/nodejs/index.ts
Additional comments: 5
examples/nodejs/src/escrow-flow.ts (2)
  • 116-120: Ensure that the mintRole function is used correctly and that the types returned by mintRole are compatible with the createContract function's expectations.

  • 113-123: > Note: This review was outside the patches, so it was mapped to the patch with the greatest overlap. Original lines [7-123]

The changes to the roles object in the createContract function call appear to be consistent with the PR objectives and AI-generated summaries. No further issues are observed in this hunk.

packages/runtime/core/src/address.ts (3)
  • 5-27: The refactoring of the AddressBech32 type and related functions to use AddressBech32Guard is consistent with the PR objectives. However, the AddressBech32Guard type guard always returns true, which may not provide any meaningful runtime validation. Please confirm if this is the intended behavior or if a more robust validation should be implemented.
(s): s is t.Branded<string, AddressBech32Brand> => true,
  • 19-20: The use of unsafeEither for decoding might lead to unhandled errors if the decoding fails. Ensure that there is proper error handling in place where addressBech32 is used, or consider implementing a safer approach to decoding.
export const addressBech32 = (s: string) =>
  unsafeEither(AddressBech32Guard.decode(s));
  • 22-27: The AddressesAndCollaterals structure has been updated to use the new AddressBech32Guard type for its changeAddress and usedAddresses fields, which aligns with the PR's objectives to refactor address handling. This change appears to be correctly implemented.

examples/nodejs/src/escrow-flow.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@hrajchert hrajchert left a comment

Choose a reason for hiding this comment

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

minor changes

examples/nodejs/src/escrow-flow.ts Outdated Show resolved Hide resolved
packages/runtime/core/src/address.ts Show resolved Hide resolved
packages/runtime/core/src/tsconfig.json Show resolved Hide resolved
@nhenin nhenin requested a review from hrajchert December 13, 2023 16:34
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between ebf25e7 and 0a6b6d7.
Files ignored due to filter (4)
  • examples/nodejs/package-lock.json
  • package-lock.json
  • packages/runtime/core/package.json
  • packages/runtime/core/src/tsconfig.json
Files selected for processing (12)
  • changelog.d/20231213_092859_nicolas.henin_revert_address.md (1 hunks)
  • packages/runtime/client/rest/src/contract/endpoints/collection.ts (2 hunks)
  • packages/runtime/client/rest/src/contract/guards.ts (1 hunks)
  • packages/runtime/client/rest/src/contract/index.ts (1 hunks)
  • packages/runtime/client/rest/src/contract/rolesConfigurations.ts (3 hunks)
  • packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts (2 hunks)
  • packages/runtime/client/rest/src/payout/details.ts (2 hunks)
  • packages/runtime/client/rest/src/withdrawal/endpoints/collection.ts (2 hunks)
  • packages/runtime/core/src/address.ts (1 hunks)
  • packages/runtime/lifecycle/src/generic/contracts.ts (2 hunks)
  • packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts (1 hunks)
  • packages/wallet/src/nodejs/index.ts (6 hunks)
Files skipped from review due to trivial changes (1)
  • packages/runtime/client/rest/src/contract/index.ts
Files skipped from review as they are similar to previous changes (9)
  • changelog.d/20231213_092859_nicolas.henin_revert_address.md
  • packages/runtime/client/rest/src/contract/endpoints/collection.ts
  • packages/runtime/client/rest/src/contract/guards.ts
  • packages/runtime/client/rest/src/contract/transaction/endpoints/collection.ts
  • packages/runtime/client/rest/src/payout/details.ts
  • packages/runtime/client/rest/src/withdrawal/endpoints/collection.ts
  • packages/runtime/lifecycle/src/generic/contracts.ts
  • packages/runtime/lifecycle/test/examples/swap.ada.token.e2e.spec.ts
  • packages/wallet/src/nodejs/index.ts
Additional comments: 4
packages/runtime/client/rest/src/contract/rolesConfigurations.ts (4)
  • 6-7: The import changes and additions are consistent with the PR's objective to refactor address handling and remove specific functions and types associated with address processing.

  • 133-135: The update to the RoleTokenConfiguration interface to use AddressBech32 directly for the recipients field aligns with the PR's objective to simplify address handling.

  • 152-157: The shell script output indicates that there are no imports of RoleTokenConfigurations or RoleConfigurationsGuard in any TypeScript files outside of rolesConfigurations.ts. This suggests that the changes related to these types and guards are localized to the rolesConfigurations.ts file and do not affect other parts of the codebase.

Given this context, it seems that the introduction of RoleTokenConfigurations and RoleConfigurationsGuard is consistent with the refactoring efforts within the rolesConfigurations.ts file and does not introduce any issues with external dependencies or usage in the broader codebase.

Therefore, the changes related to RoleTokenConfigurations and RoleConfigurationsGuard are verified to be correctly implemented within the scope of the provided information and the PR objectives.

Analysis chain

The introduction of the RoleTokenConfigurations type and RoleConfigurationsGuard appears to be consistent with the refactoring efforts and should be checked for correct usage throughout the codebase.


The shell script output indicates that RoleTokenConfigurations is used in the rolesConfigurations.ts file to define a type and also as part of a type definition for MintRolesTokens. Similarly, RoleConfigurationsGuard is used within the same file to define a constant and as part of a type definition in a t.record.

Given that the usage of both RoleTokenConfigurations and RoleConfigurationsGuard is confined to the rolesConfigurations.ts file and seems to be part of the type definitions related to role configurations, it appears that these introductions are consistent with the refactoring efforts described in the PR objectives and the AI-generated summary.

However, to ensure that these changes are correctly integrated into the broader codebase, it would be necessary to check if there are any other files that might be affected by these changes, especially considering that the types and guards are exported and could be used elsewhere.

Let's check for any imports of RoleTokenConfigurations and RoleConfigurationsGuard in other files to verify if they are being used correctly outside of rolesConfigurations.ts.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for the usage of RoleTokenConfigurations and RoleConfigurationsGuard to ensure they are used correctly.
rg 'RoleTokenConfigurations' --type ts
rg 'RoleConfigurationsGuard' --type ts

Length of output: 583



Script:

#!/bin/bash
# Search for imports of RoleTokenConfigurations and RoleConfigurationsGuard in other files.
rg --type ts 'import.*RoleTokenConfigurations' -g '!packages/runtime/client/rest/src/contract/rolesConfigurations.ts'
rg --type ts 'import.*RoleConfigurationsGuard' -g '!packages/runtime/client/rest/src/contract/rolesConfigurations.ts'

Length of output: 239

* 162-166:

The verification process has confirmed that the RoleTokenConfiguration interface and the ClosedRole type are consistent with the new address handling mechanism. The RoleTokenConfiguration directly uses the AddressBech32 type for keys in the recipients field, and ClosedRole is an alias for AddressBech32, which aligns with the changes made in the pull request. The MintRolesTokens type and MintRolesTokensGuard are used correctly within the rolesConfigurations.ts file and are consistent with the new address handling mechanism.

Analysis chain

The updates to the MintRolesTokens type and MintRolesTokensGuard should be verified for consistency with the new address handling mechanism and correct usage across the codebase.

Scripts Executed

The following scripts were executed for the analysis:


Script:

#!/bin/bash
# Search for the usage of MintRolesTokens and MintRolesTokensGuard to ensure they are used correctly.
rg 'MintRolesTokens' --type ts
rg 'MintRolesTokensGuard' --type ts

Length of output: 870



Script:

#!/bin/bash
# Search for the definition of RoleTokenConfigurations to verify its consistency with the new address handling mechanism.
rg 'RoleTokenConfigurations' --type ts

# Check the usage of MintRolesTokensGuard in rolesConfigurations.ts to ensure it aligns with the new address handling mechanism.
rg 'MintRolesTokensGuard' packages/runtime/client/rest/src/contract/rolesConfigurations.ts

Length of output: 486



Script:

#!/bin/bash
# Search for the definition of RoleTokenConfiguration to verify its consistency with the new address handling mechanism.
rg 'RoleTokenConfiguration' --type ts

# Search for the definition of ClosedRole to understand its structure.
rg 'ClosedRole' --type ts

Length of output: 2785



Script:

#!/bin/bash
# Retrieve the definition of RoleTokenConfiguration from the rolesConfigurations.ts file.
rg 'export interface RoleTokenConfiguration' -A 10 packages/runtime/client/rest/src/contract/rolesConfigurations.ts

Length of output: 418

packages/runtime/core/src/address.ts Show resolved Hide resolved
packages/runtime/core/src/address.ts Show resolved Hide resolved
packages/runtime/core/src/address.ts Show resolved Hide resolved
@hrajchert hrajchert merged commit 89082e9 into main Dec 13, 2023
2 checks passed
@hrajchert hrajchert deleted the revert-address branch December 13, 2023 17:06
@nhenin nhenin self-assigned this Dec 15, 2023
@nhenin nhenin added this to the Priority 1: Open Roles Support milestone Dec 15, 2023
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.

2 participants