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: PromiseIndex type #378

Merged
merged 6 commits into from
Feb 25, 2024
Merged

fix: PromiseIndex type #378

merged 6 commits into from
Feb 25, 2024

Conversation

matiasbenary
Copy link
Contributor

@matiasbenary matiasbenary commented Jan 29, 2024

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • Commit messages follow the conventional commits spec
  • If this is a code change: I have written unit tests.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

Currently there is an error in the type PromiseIndex. Currently it is defined as (number | bigint) & PromiseIndexBrand. Since & is the intersection operator, and that PromiseIndexBrand == -1, then the only valid value is -1.

This is an error since PromiseIndex is used to access a Promise Result, which is at lease 0 (if it exists).

We caught this with @gagdiez while testing the cross contract calls example. Notice that the error was not captured before because it only arises in Typescript (and not in JS, which the current XCC example is written in).

Test Plan

Added a new version of the cross contract call written in TS

Related issues/PRs

#379

@gagdiez
Copy link
Collaborator

gagdiez commented Jan 29, 2024

Notice that the CI in develop is failing, this will be fixed by #377

@gagdiez gagdiez changed the title fix: change type fix: PromiseIndex type Jan 30, 2024
@gagdiez
Copy link
Collaborator

gagdiez commented Jan 30, 2024

@fospring @ailisp

I would actually recommend to:

@gagdiez
Copy link
Collaborator

gagdiez commented Feb 6, 2024

@fospring this looks good to me

@fospring
Copy link
Contributor

LGTM. thank you @matiasbenary @gagdiez

@fospring fospring merged commit c7be89a into near:develop Feb 25, 2024
2 checks passed
@gagdiez gagdiez mentioned this pull request Feb 25, 2024
4 tasks
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.

3 participants