Introduce AccountIdBuilder

[PhilippGackstatter]

Couple of small fixes following from the account ID refactor (#982).

The biggest addition is the account_id_dummy! macro, hence the PR title and changelog entry.

The changes are:

• Introduce AccountIdBuilder to simplify AccountId generation to address Extend AccountId to two Felts and refactor creation process #982 (comment) cc @Mirko-von-Leipzig.
• Reverse Account ID in foreign account advice map key.
• Remove unnecessary From impls on AccountType and AccountStorageMode. This functionality is essentially covered by methods on AccountId already.
• Small fixes to MASM code and documentation additions and fixes.

[bobbinth]

Looks good! Thank you! I left a couple of small comments inline.

[bobbinth]

nit (feel free to ignore): dummy_account_id feels a bit more natural as the name here.

[Mirko-von-Leipzig]

Feels a bit strange that its a macro. We could implement rand's Distribution but that wouldn't handle type and storage mode variations.

Alternatively we could also have a builder but that is significantly more implementation code. (just thinking out loud, not recommending this).

[PhilippGackstatter]

We could implement rand's Distribution

I think this would make sense for AccountType and AccountStorageMode to get rid of the sample_ functions.

Alternatively we could also have a builder but that is significantly more implementation code. (just thinking out loud, not recommending this).

Thanks for the suggestion. I considered that as well, but the builder seemed like too much boilerplate for just this functionality. Originally I also wanted to get rid of AccountId::dummy and enable calling account_id_dummy! with [u8; 15] and an impl Rng, but it didn't work as nicely as I had hoped.

Maybe the builder would make more sense ultimately. If we introduce another AccountIdVersion, then we'd likely also want to include that in the macro, and then it takes three parameters. And you would be forced to supply all three type, mode and version even if you just want a specific version. With a builder, one can flexibly pick and choose.

I imagine something simple like:

AccountIdBuilder::new(&mut rng).storage_mode(AccountStorageMode::Private).build();
AccountIdBuilder::new(&mut rng).build();

Seems short enough to easily read in tests. Alternatively we can also pass the rng to build I think, though not seeing a significant advantage for one or the other.

[Mirko-von-Leipzig]

Maybe the builder would make more sense ultimately. If we introduce another AccountIdVersion, then we'd likely also want to include that in the macro, and then it takes three parameters. And you would be forced to supply all three type, mode and version even if you just want a specific version. With a builder, one can flexibly pick and choose.

I imagine something simple like:

AccountIdBuilder::new(&mut rng).storage_mode(AccountStorageMode::Private).build();
AccountIdBuilder::new(&mut rng).build();

Seems short enough to easily read in tests. Alternatively we can also pass the rng to build I think, though not seeing a significant advantage for one or the other.

Passing it to build does separate the "configuration" from the generation/seed selection. You could have

.build_with_rng(&mut rng)
.build_with_seed(..)
.build_with_thread_rng(..)

Though there are several APIs one could shuffle these into. Likely this isn't the only place we might want this style of thing 🤷

[PhilippGackstatter]

Went with the AccountIdBuilder and it does seem nicer than the macro. Can you have another look, @Mirko-von-Leipzig? Thank you!

I did not add a AccountId::builder method because the builder is only for testing purposes, so keeping it somewhat separate seems like a good idea.

[bobbinth]

Looks good! Thank you!