feat: core refactor
#30
Conversation
Problem: The current state of the core is not reliable for use. Some of the parameters of the functions are simply ignored (faced that with `apiRequest` previously), some are counter-intuitive and use repetitive naming. Matter fact, ~30% code of the classes is its initialization, and sometimes the actual business logic are just two or even a single function. Solution: - Move away from writing custom classes on top of the other wherever possible and rely on the `@abstract-money/cli` generated modules as much as possible. - Provide intuitive and declarative naming to the methods to be used. - De-classify classes that should not have been classes and reduce complexity. - To eliminate confusion between `AnsAsset` and `Asset` (which has an `amount` field), introduce `Token` type. The list will be populated as the refactor goes.
🦋 Changeset detectedLatest commit: 95664f6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
I figured out that we actually have 5 clients: - base client – knows only the `apiUrl` - public client - knows the `apiUrl` and `CosmWasmClient` - wallet client - knows the `apiUrl` and `SigningCosmWasmClient` - account-public client - knows it's accountId, `apiUrl` and `CosmWasmClient` - account-wallet client - knows it's accountId, `apiUrl` and `SigningCosmWasmClient` The commit is still WIP, yet mostly it's just the functionality of the abstract account that is left to be implemented. Following folders were created to manage the codebase effectively: - `actions` – contains all actions that clients are able to perform | - `public` – contains all public client actions | - `wallet` – contains all wallet client actions | - `account-public` – contains all account-public client actions | - `account-wallet` – contains all account-wallet client actions | ... - base client actions | Some of them are grouped by module groups, such as `ans`, `account-factory`, `manager` etc. - `utils` – contains all utility data-transformation, formatting functions – `clients` – contains the clients implementations | - `decorators` – contains the decorators to inject the client data and bind those to clients - `types` – contains types With such a structure, we're clearly separating the business logic from the utilitary logic that mostly concerns different data transormations. The focus here is also to keep the functions with the most minimal API as possible, so that hardly re-uses `ts-codegen` generated types whenever possible. The API might not look user-friendly at the moment, but later on we will provide aliases for more user-friendly function names.
There was a problem hiding this comment.
Overall, the changes look really good. I didn't like them at first, but came to enjoy the way that the code read when diving deeper.
Left quite a few comments / questions, though the most major is that we need to add doc comments for all the public-facing api at least. And secondly, we should ensure we're not requesting too much info from the API when unnecessary.
The refactor of the API / Console to use this is going to be quite a PITA... but good that it'll be done at once.
packages/core/src/actions/account-factory/get-account-factory-address-from-api.ts
Outdated
Show resolved
Hide resolved
| ` | ||
| query AssetsQuery($chain: ID!, $filter: IdsFilter!) { | ||
| ans(chain: $chain) { | ||
| assets(filter: $filter) { |
There was a problem hiding this comment.
Let's make a task to deprecate the assets api to be replaced with tokens.
There was a problem hiding this comment.
We can improve it at the same time to return different types based on cw20 or native, getting rid of the overridden "address"
There was a problem hiding this comment.
packages/core/src/utils/ans/utils/ans-name-to-ibc-token-name.ts
Outdated
Show resolved
Hide resolved
|
We removed Something we'd considered in the past is allowing for passing |
| cosmWasmClient: CosmWasmClient, | ||
| versionControlAddress: string, | ||
| version?: string, | ||
| ) { | ||
| const registryClient = new VersionControlQueryClient( | ||
| cosmWasmClient, | ||
| versionControlAddress, | ||
| ) |
There was a problem hiding this comment.
The cosmWasmClient is propagated by decorators therefore it simplifies the overall API, yet if we would pass such an entity explicitly the developer would have to create one himself.
packages/core/src/utils/ans/utils/ans-name-to-ibc-token-name.ts
Outdated
Show resolved
Hide resolved
Checkpoint commit to return to if we decide to come back with nested folders approach.
Builds succesfully
I addressed that issue, and now |
|
Size Change: +428 B (0%) Total Size: 91.2 kB
ℹ️ View Unchanged
|
All the mutating actions return
|
packages/core/src/actions/public/get-module-address-from-version-control.ts
Outdated
Show resolved
Hide resolved
packages/core/src/actions/public/get-module-address-from-version-control.ts
Outdated
Show resolved
Hide resolved
packages/core/src/actions/wallet/get-version-control-client-from-api.ts
Outdated
Show resolved
Hide resolved
| getAccountFactoryAddressFromApi: (...args) => | ||
| getAccountFactoryAddressFromApi(apiUrl, ...args), | ||
| getAnsHostAddressFromApi: (...args) => | ||
| getAnsHostAddressFromApi(apiUrl, ...args), | ||
| getAccountsByOwnerFromApi: (...args) => | ||
| getAccountsByOwnerFromApi(apiUrl, ...args), | ||
| getAnsTokenFromApi: (...args) => getAnsTokenFromApi(apiUrl, ...args), | ||
| getAnsTokensFromApi: (...args) => getAnsTokensFromApi(apiUrl, ...args), | ||
| getVersionControlAddressFromApi: (...args) => | ||
| getVersionControlAddressFromApi(apiUrl, ...args), |
There was a problem hiding this comment.
If this is the api client wouldn't it make sense to name them without the FromApi postfix?
There was a problem hiding this comment.
We could rename them here but overall it's better to keep the postfix
| export function accountIdToParameter(id: VersionControlTypes.AccountId) { | ||
| return { seq: id.seq, trace: id.trace } | ||
| } |
There was a problem hiding this comment.
The parameter should be the AccountId type rather than VersionControlTypes.AccountId,
There was a problem hiding this comment.
Unless it's meant just to truncate the chainName?
There was a problem hiding this comment.
this method does not need chainName as it doesn't use one in the response
Ah yes, forgot about async executeMultiple(senderAddress, instructions, fee, memo = "") {
...
const result = await this.signAndBroadcast(senderAddress, msgs, fee, memo);
if ((0, stargate_1.isDeliverTxFailure)(result)) {
throw new Error(createDeliverTxResponseErrorMessage(result));
}
return {
logs: stargate_1.logs.parseRawLog(result.rawLog),
height: result.height,
transactionHash: result.transactionHash,
events: result.events,
gasWanted: result.gasWanted,
gasUsed: result.gasUsed,
};
``` |
packages/core/src/actions/public/get-abstract-module-address-from-version-control.ts
Outdated
Show resolved
Hide resolved
Problem: The current state of the core is not reliable for use. Some of the parameters of the functions are simply ignored (faced that with
apiRequestpreviously), some are counter-intuitive and use repetitive naming. Matter fact, ~30% code of the classes is its initialization, and sometimes the actual business logic are just two or even a single function.Solution:
Move away from writing custom classes on top of the other wherever possible and rely on the
@abstract-money/cligenerated modules as much as possible.Provide intuitive and declarative naming to the methods to be used.
De-classify classes that should not have been classes and reduce complexity.
To eliminate confusion between
AnsAssetandAsset(which has anamountfield), introduceTokentype.The list will be populated as the refactor goes.