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(init): Remove connect workflow and create factory init method #33

Merged
merged 15 commits into from
Apr 12, 2024

Conversation

atticusofsparta
Copy link
Contributor

No description provided.

@atticusofsparta atticusofsparta requested a review from a team as a code owner April 5, 2024 15:29
src/utils/common.ts Outdated Show resolved Hide resolved
@dtfiedler
Copy link
Collaborator

dtfiedler commented Apr 9, 2024

mind changing the title of this PR - something like "create init method to support ArIOReadable and ArIOWriteable interfaces and class"

@atticusofsparta atticusofsparta changed the title chore(connect): update connect workflow chore(init): Remove connect workflow and create factory init method Apr 9, 2024
@@ -173,29 +173,28 @@ The SDK provides TypeScript types. When you import the SDK in a TypeScript proje

### APIs

#### `connect(signer)`
#### `init({ signer })`
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the comment below - mention that signer is optional. and if provided ArIOWriteable class is returned that supports write interaction APIs; otherwise an ArIOReadable class is returned and supports read apis only.

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/common/ar-io.ts Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
contract?:
| (BaseContract<unknown> & ReadContract)
| (BaseContract<unknown> & ReadWriteContract);
contract?: WarpContract<unknown> | RemoteContract<unknown>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

does this need to be optional anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont think it actually ever needed to be, since we default it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we need an instance where we want to make one nullable, it can be piped thru an Omit type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually it does need to be optional for a few reasons, mainly to help enable DX, however it may be prudent to create different types rather than forcing use of this one. We should talk on it, i left as is since it was simpler that way for now.

src/common/ar-io.ts Outdated Show resolved Hide resolved
@dtfiedler
Copy link
Collaborator

looking good - can we add write interaction tests?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
arIO.connect(nodeSigner);
const arIOWriteable = ArIO.init({ signer: nodeSigner});

const arIOReadable = ArIO.init()
Copy link
Collaborator

Choose a reason for hiding this comment

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

move this up top and add a comment

Suggested change
const arIOReadable = ArIO.init()
// read-only client that has access to all read APIs
const arIOReadable = ArIO.init()

README.md Show resolved Hide resolved
@@ -569,7 +569,7 @@ const auctions = await arIO.getAuctions({ evaluationOptions });
// }
```

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

Joins a gateway to the ar.io network via its associated wallet.
Copy link
Collaborator

Choose a reason for hiding this comment

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

add that this requires signer to be provided on init() - and same for the rest of them

atticusofsparta and others added 3 commits April 10, 2024 13:00
Co-authored-by: Dylan Fiedler <dtfiedler@users.noreply.github.com>
Co-authored-by: Dylan Fiedler <dtfiedler@users.noreply.github.com>
Co-authored-by: Dylan Fiedler <dtfiedler@users.noreply.github.com>
Comment on lines 34 to 53
it('should connect and return a valid instances of read and write clients', async () => {
const readClient = ArIO.init({
contract: new RemoteContract<ArIOState>({
contractTxId,
cacheUrl: localCacheUrl,
}),
});
expect(client.connect(signer)).toBeDefined();
expect(client).toBeInstanceOf(ArIO);
const writeClient = ArIO.init({
signer,
contract: new WarpContract<ArIOState>({
cacheUrl: localCacheUrl,
contractTxId,
logger: new DefaultLogger({ level: 'none' }),
}),
});
expect(readClient).toBeDefined();
expect(readClient).toBeInstanceOf(ArIOReadable);
expect(writeClient).toBeDefined();
expect(writeClient).toBeInstanceOf(ArIOWritable);
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

i would separate these

Comment on lines +105 to +106
//this.contract = this.contract.connect(warpSigner);
//this.signer = warpSigner;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
//this.contract = this.contract.connect(warpSigner);
//this.signer = warpSigner;

@@ -54,32 +54,26 @@ export class WarpContract<T>
private logger: Logger;
private warp: Warp;
// warp compatible signer that uses ContractSigner
private signer: CustomSignature | undefined;
//private signer: CustomSignature | undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
//private signer: CustomSignature | undefined;

@@ -80,6 +80,8 @@ export async function createLocalWallet(
const address = await arweave.wallets.jwkToAddress(wallet);
// mint some tokens
await arweave.api.get(`/mint/${address}/${amount}`);
const walletBalance = await arweave.wallets.getBalance(address);
console.log(`Wallet balance: ${walletBalance}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
console.log(`Wallet balance: ${walletBalance}`);

@dtfiedler dtfiedler self-requested a review April 12, 2024 02:10
Copy link
Collaborator

@dtfiedler dtfiedler left a comment

Choose a reason for hiding this comment

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

some small cleanup nits - but we can merge and finalize everything in the initial PR

@atticusofsparta atticusofsparta merged commit 2cad263 into PE-5753-add-gar-methods Apr 12, 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 connect-workflow branch April 23, 2024 16:29
@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.

2 participants