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

chore(interfaces): add methods for gar to ArIO contract interface #31

Merged
merged 27 commits into from
Apr 15, 2024

Conversation

atticusofsparta
Copy link
Contributor

No description provided.

@atticusofsparta atticusofsparta requested a review from a team as a code owner March 27, 2024 16:46
src/common/ar-io.ts Outdated Show resolved Hide resolved
src/common/ar-io.ts Outdated Show resolved Hide resolved
src/common/ar-io.ts Outdated Show resolved Hide resolved
src/common/error.ts Outdated Show resolved Hide resolved
src/common.ts Outdated Show resolved Hide resolved
src/common/ant.ts Outdated Show resolved Hide resolved
src/common/ant.ts Outdated Show resolved Hide resolved
@atticusofsparta atticusofsparta changed the base branch from PE-5753-gar-write to alpha March 27, 2024 20:31
src/common/ar-io.ts Outdated Show resolved Hide resolved
src/common/ar-io.ts Outdated Show resolved Hide resolved
README.md Outdated
@@ -569,6 +569,142 @@ const auctions = await arIO.getAuctions({ evaluationOptions });
// }
```

#### `joinNetwork({ ...JoinNetworkParams})`

Executes the interaction for the connected gateway wallet to join the ar.io network.

Choose a reason for hiding this comment

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

Consider Joins a gateway to the ar.io network via its associated wallet.

README.md Outdated
qty: 4000,
allowDelegatedStaking: true,
delegateRewardShareRatio: 1,
fqdn: 'impossible.com',

Choose a reason for hiding this comment

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

Nit that this example might be a little confusing as impossible could be projecting that the example is one that should NOT be followed.

README.md Outdated
Executes the interaction for the connected gateway wallet to join the ar.io network.

```typescript
const params = {

Choose a reason for hiding this comment

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

Nit: joinNetworkParams is more helpful.

Choose a reason for hiding this comment

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

Nit: order the parameters in a way that logically groups them. e.g. connection settings together, staking settings together, etc.

src/common.ts Outdated
| InteractionResult<unknown, unknown>;

export type JoinNetworkParams = {
qty: number;

Choose a reason for hiding this comment

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

This type isn't doing much to help make it easy to use this correctly and hard to use incorrectly. For example, how should users think about how label and note are different? How should they think about properties? etc. There's a few things you could do:

  • create sub-groupings of these parameters, e.g. connection, staking, operatorInfo, etc.
  • add comments next to each of these properties explaining their relevance
  • make some of the names less ambiguous (e.g. differentiate between label and note somehow)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dtfiedler wdyt:

export type GatewayStakingSettings = {
  allowDelegatedStaking: boolean;
  delegateRewardShareRatio: number;
  minDelegatedStake: number;
  autoStake: boolean;
};

export type GatewayMetadata = {
  label: string;
  note: string;
  properties: string;
};

export type GatewayConnectionSettings = {
  fqdn: string;
  port: number;
  protocol: AllowedProtocols;
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

better

src/common.ts Outdated
autoStake?: boolean;
};

export type UpdateGatewaySettingsParams = {

Choose a reason for hiding this comment

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

I understand the convenience of all the optionality in this type's fields - a user can simply supply the pieces of info they want to update and one or many/all can be changed in ONE L1 transaction. Yay. However, it creates many possible permutations of ways that one can use this type/function and many ways that things can possibly go wrong. It could be helpful for the user to let them know whether or not partial application of their updates takes place or not if something errors (e.g. is it all or nothing) as well as some basic reasons why things might go wrong, e.g. allowable ranges, lengths, character sets, etc. for certain values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Im not understanding the change you have in mind, are you thinking like JSDOC for usage examples?

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's hard with the current contract configuration - but i would do at a minimum something that enforces providing at least one value

// Original type definition refined with proper field-specific types
type UpdateGatewaySettingsParamsBase = {
  allowDelegatedStaking?: boolean;
  delegateRewardShareRatio?: number;
  fqdn?: string;
  label?: string;
  minDelegatedStake?: number;
  note?: string;
  port?: number;
  properties?: string;
  protocol?: AllowedProtocols;
  autoStake?: boolean;
};

// Utility type to require at least one of the fields
type AtLeastOne<T, U = {[K in keyof T]-?: T[K]} > = 
  Partial<U> & U[keyof U];

// Define the type used for function parameters
type UpdateGatewaySettingsParams = AtLeastOne<UpdateGatewaySettingsParamsBase>;

Copy link
Contributor Author

@atticusofsparta atticusofsparta Apr 15, 2024

Choose a reason for hiding this comment

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

the above type didnt work but I accomplished it with a slight variation:
export type AtLeastOne<T, U = { [K in keyof T]-?: T[K] }> = Partial<U> & { [K in keyof U]: Required<Pick<U, K>> }[keyof U];

README.md Outdated Show resolved Hide resolved
@atticusofsparta atticusofsparta merged commit 8900554 into alpha Apr 15, 2024
8 checks passed
@dtfiedler
Copy link
Collaborator

🎉 This PR is included in version 1.0.0-alpha.20 🎉

The release is available on:

Your semantic-release bot 📦🚀

@dtfiedler dtfiedler deleted the PE-5753-add-gar-methods branch April 23, 2024 16:30
@dtfiedler
Copy link
Collaborator

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants