Add Account preset

[ericnordelo]

Partially fixes #804

• Tests
• Tried the feature on a public network
• Documentation

[andrew-fleming]

Great work, Eric! I left a suggestion and small question. I think we should include a mention and a link in the docs—maybe just in the Account section for now. When we have more, we can consider including a list in the README like we did with cairo v0. Wdyt?

[ericnordelo]

I think we should include a mention and a link in the docs—maybe just in the Account section for now. When we have more, we can consider including a list in the README like we did with cairo v0. Wdyt?

I added a section in the API Reference for presets, I think that's (arguably) more compliant with the doc framework we are trying to target, and then users can follow which implementations the preset is embedding, the constructor, and so on. Let me know what you think. I'd like keeping the tutorial section as clean as possible from references.

[andrew-fleming]

Great job on adding the presets in the API! I think it works really well. In another review (whoops), I left a suggestion and agreed on the idea of elaborating a little bit on the account component. Both are optional. Other than that, LGTM we also need to include the class hash as discussed earlier :)

[martriay]

Very nice job! I'm not sure how I feel about the preset test copying that much code from the component's suite. Should the new differences (helpers, reordering tests) be integrated into the component's tests?

I think it make sense for both suites to share most of the test cases, is there any chance we could reuse the module's for the preset? that'd be a step further in our plans of offering tests to our users.

[martriay]

don't we want to update the line number? by the way, these links are broken across the API page, in Account Component and Preset.

I also realized IDeclarer, IDeployer, as well as camel only traits are not documented, only the impls

[ericnordelo]

don't we want to update the line number?

I think ew shouldn't, because that's very hard to maintain, when updating a file we would need to look in the doc if a line was affected, and I don't think is worth the benefit. Maybe I'm wrong.

by the way, these links are broken across the API page, in Account Component and Preset.

The only link I see broken is the one for the preset, because that file is not in 0.8.0-beta.0, which will be fixed after releasing (maybe I should update to the X.Y.Z until that point? ). Are there other broken links?

I also realized IDeclarer, IDeployer, as well as camel only traits are not documented, only the impls.

true

[ericnordelo]

I also realized IDeclarer, IDeployer, as well as camel only traits are not documented, only the impls.

Do you think it is worth adding them? That would make the API Reference grow bigger without adding much real value IMO (in this particular case).

[martriay]

I think ew shouldn't, because that's very hard to maintain, when updating a file we would need to look in the doc if a line was affected, and I don't think is worth the benefit. Maybe I'm wrong.

True, but without the line to me it makes no sense to link in each section. I think the best experience for our docs is to have direct links (i find them very useful), probably they should not be updated manually in each PR, we could:

• check for link health just once before each release, still manual but way less friction and contained
• build some tool/hook to detect that a linked line of code got modified in a commit and alert us so we update it
• both

[martriay]

Do you think it is worth adding them? That would make the API Reference grow bigger without adding much real value IMO (in this particular case).

I'm not sure. I don't think it'll grow much since I expect most of it to be moved from the component and the component ro look similar to the preset. I don't have strong feelings, I just think it's weird to not mention them since they're referenced (e.g. in the impls). I guess it's good for users to know they're available.

[martriay]

Are there other broken links?

All the source code links, they're not broken links, they're just pointing to the wrong lines.

[martriay]

small detail, but this is inconsistent with the component's initializer test which reads:

    assert(state.account.get_public_key() == PUBKEY, 'Should return public key');

it's similar with the ISRC5/6 error messages below, they're not normalized across the codebase (e.g. account component) and they probably should. some day 😅

[ericnordelo]

Updated

[martriay]

although this specific suggestion got accepted, we still need to normalize. i created an issue, we can discuss there if we agree the normalization should be done through constants instead of copying the string each time. bear in mind this is low prio as it's not user facing

[ericnordelo]

I think it make sense for both suites to share most of the test cases, is there any chance we could reuse the module's for the preset? that'd be a step further in our plans of offering tests to our users.

I don't think we are at that point yet from the available features we have in the test runner. We would need at least to copy-paste the test functions to call internal abstractions (functions probably, or implementations), but that, besides not being pretty, would be hard as well, because the state types they would be receiving are not the same, and Cairo is not a dynamic language. The attempt to achieve that is a lot of effort and a short reward with the version we could accomplish. I think at some point we would be able to do it, just not at this point.

[andrew-fleming]

I think it make sense for both suites to share most of the test cases, is there any chance we could reuse the module's for the preset? that'd be a step further in our plans of offering tests to our users.

I don't think we are at that point yet from the available features we have in the test runner. We would need at least to copy-paste the test functions to call internal abstractions (functions probably, or implementations), but that, besides not being pretty, would be hard as well, because the state types they would be receiving are not the same, and Cairo is not a dynamic language. The attempt to achieve that is a lot of effort and a short reward with the version we could accomplish. I think at some point we would be able to do it, just not at this point.

Yeah, it's tricky. The pattern I came up with just works for tests with dispatchers which, if we're going to have multiple presets, might be worth fleshing out. I'm unsure if it's scalable though, I only tested it with a handful of tests and it's going to need a ton of gas.

#[test]
#[available_gas(200000000000000000)]
fn main() {
    let mut dispatcher = setup_dispatcher();
    test_erc20(dispatcher);
}

fn test_erc20(dispatcher: ERC20ABIDispatcher) {
    test_constructor(dispatcher);
    test_total_supply(dispatcher);
    test_balance_of(dispatcher);
    test_allowance(dispatcher);
    ...
}

[martriay]

Good! We can continue discussion offline or in another issue. This one is good to go, good job @ericnordelo!