apibara / starknet-react

A collection of React providers and hooks for StarkNet
https://starknet-react.com
MIT License
363 stars 142 forks source link

[Type] useAccount.address should return an Address type #452

Closed FabienCoutant closed 1 month ago

FabienCoutant commented 3 months ago

useAccount() hook return the following type:

/** Value returned from `useAccount`. */
export type UseAccountResult = {
  /** The connected account object. */
  account?: AccountInterface;
  /** The address of the connected account. */
  address?: string;
  /** The connected connector. */
  connector?: Connector;
  /** Connector's chain id */
  chainId?: bigint;
  /** True if connecting. */
  isConnecting?: boolean;
  /** True if reconnecting. */
  isReconnecting?: boolean;
  /** True if connected. */
  isConnected?: boolean;
  /** True if disconnected. */
  isDisconnected?: boolean;
  /** The connection status. */
  status: AccountStatus;
};

The address should be typed as Address instead of string

MariangelaNM commented 3 months ago

Hello,

I have extensive experience working with React and I can work on this issue. The solution involves adjusting the address property in UseAccountResult to use the Address type.

Benjtalkshow commented 3 months ago

@FabienCoutant

I want to help fix the issue where useAccount.address should return an Address type. I am good with React and TypeScript. I can make this change quickly and correctly.

Please assign this issue to me. Thanks,

fracek commented 3 months ago

Good suggestion. Will be in v3.

rsodre commented 3 months ago

Address as 0x${string}? I remember not having a good experience when I first started using it. Please consider returning a BigNumberish, that's what starknet.js use.

fracek commented 2 months ago

The goal is to have a getAddress function that validates the address and returns 0x${string}. That way it catches bugs related to passing things that are not addresses so functions that expect addresses.

Benjtalkshow commented 2 months ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

@fracek , I'd love to take on this task. This is my second comment on this issue. I have extensive knowledge of React and TypeScript types, and I'll deliver a solution to this issue within a short period of time.

How I plan on tackling this issue

@fracek, I was ready to tackle this issue when you told me to wait for ODHack 7. I was mentioned earlier on this issue to tackle it. Can i go ahead to open a PR as instructed?

fracek commented 2 months ago

@Benjtalkshow that sounds good. Can you open a PR against the develop branch where you add the getAddress helper (in an utils.ts file for example) that simply invokes validateAndParseAddress from starknet.js and casts the result as 0x${string}?

PedroCo3lho commented 1 month ago

Hello @fracek isn't this issue already fixed here:

https://github.com/apibara/starknet-react/blob/2b08aa3f8e9b8140b19d5989ca72cef62a531200/packages/core/src/hooks/use-account.ts#L19-L37

Benjtalkshow commented 1 month ago

@Benjtalkshow that sounds good. Can you open a PR against the develop branch where you add the getAddress helper (in an utils.ts file for example) that simply invokes validateAndParseAddress from starknet.js and casts the result as 0x${string}?

@fracek , I am just seeing that you mentioned me. Am I still eligible to open a PR? I am available and ready to hop on the issue as soon as possible.

fracek commented 1 month ago

Yes of course! Since I commented I made some changes so now you should make a PR against the main branch. BTW if you wait ~10 days or so you can do this work as part of the next OD Hack

Benjtalkshow commented 1 month ago

Yes of course! Since I commented I made some changes so now you should make a PR against the main branch. BTW if you wait ~10 days or so you can do this work as part of the next OD Hack

Ok. I will wait then. Thanks a lot. Can I get the telegram group link?

fracek commented 1 month ago

We will release the telegram group once OD Hack starts!

martinvibes commented 1 month ago

can I work on this issue @FabienCoutant

GoSTEAN commented 1 month ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

@FabienCoutant Can I work on this issue?

How I plan on tackling this issue

  1. Implement or Utilize useAccount Hook: Integrate the useAccount hook within your component to retrieve the UseAccountResult object, which contains all the necessary account-related properties.

  2. Access Account Properties: Use properties like account, address, connector, and chainId from the UseAccountResult object to display or manage the connected account's details within your application.

  3. Handle Connection States: Check the boolean flags (isConnecting, isReconnecting, isConnected, isDisconnected) to control the UI or trigger actions based on whether the account is connecting, reconnecting, connected, or disconnected.

  4. Utilize status Property: Use the status property to get the overall connection status, which can guide decisions on what to display or actions to take based on the account's state.

  5. Test and Verify: Thoroughly test the integration to ensure that all properties and status flags correctly represent the account's state in different scenarios, including initial connection, reconnection, and disconnection.

ShantelPeters commented 1 month ago

Can i please be assigned to this issue @fracek

jaipaljadeja commented 1 month ago

Can i please be assigned to this issue @fracek

@ShantelPeters please apply via OnlyDust : )

Jemiiah commented 1 month ago

I am applying to this issue via OnlyDust platform.

My background and how it can be leveraged

I'm a frontend developer, my first time contributing to this repo it would take me a maximum of 4 working day to complete this issue and I have over 30 + contributions

How I plan on tackling this issue

in intend to solve this issue in following this step:

  1. Update the Type Definition: I would modify the UseAccountResult type definition to replace the string type for the address field with the Address type.
import { Address } from 'your-address-type-definition-file':

export type UseAccountResult = {
  /** The connected account object. */
  account?: AccountInterface;
  /** The address of the connected account. */
  address?: Address;
  /** The connected connector. */
  connector?: Connector;
  /** Connector's chain id */
  chainId?: bigint;
  /** True if connecting. */
  isConnecting?: boolean;
  /** True if reconnecting. */
  isReconnecting?: boolean;
  /** True if connected. */
  isConnected?: boolean;
  /** True if disconnected. */
  isDisconnected?: boolean;
  /** The connection status. */
  status: AccountStatus;
}; 
  1. Ensure Address Type is Correctly Defined: I will make sure that the Address type is correctly defined in your project. This type should be a stricter type definition that represents a valid blockchain address.

export type Address = `0x${string}`;

  1. Update Usage Across the Codebase: After updating the type definition, i will ensure that all places in the codebase where useAccount() is used or where the address field is accessed are updated to reflect this new type definition. This might include updating function parameters, state variables, or any other structures that expect an Address type.

  2. Testing: After making these changes, i will test the application to ensure that the new type definition does not introduce any issues. I will ensure that the address is being handled correctly and that type-checking passes without errors.

Benjtalkshow commented 1 month ago

@fracek , You said I should work on this issue during this ODHack. Can you assign the issue to me now? I already applied through onlydust.

fracek commented 1 month ago

Hello @fracek isn't this issue already fixed here:

https://github.com/apibara/starknet-react/blob/2b08aa3f8e9b8140b19d5989ca72cef62a531200/packages/core/src/hooks/use-account.ts#L19-L37

Hey, yes that specific issue is solved but we need the getAddress helper still.

fracek commented 1 month ago

Hey @Benjtalkshow you need to add the getAddress helper I described above.

Benjtalkshow commented 1 month ago

Hey @Benjtalkshow you need to add the getAddress helper I described above.

Alright. Thanks for giving me the oppurtunity