Oneshot accounts

requested review from @librelois

Est-ce que l'ordre des deposit_event importe ? Si non, le code des calls peut être un peu factorisé sans coût.

Non on s'en fou, va y factorise le code :)

Je n'ai pas encore fait les tests unitaires car la config de mock est assez longue à faire

Si tu peut prendre le temps de le faire dans cette PR ce serait bien

Il faudrait aussi un test end2end, car j'ai peur qu'on est des soucis lors de la consommation d'un oneshot account du fait que le compte n'existe pas du point de vue de la pallet sytem !

added 1 commit

e775eae8 - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

@tuxmain évite de rajouter le suffixe 1 dans tout ce qui apparais au monde extérieur, comme les nom des call et de leurs paramètres :)

On le fait uniquement dons le code interne.

@tuxmain pour les call qui consomment un oneshot account, il faut empecher substrate de créer un compte via l'incrément du nonce. Pour ce faire, je propose de créer notre propre wrapper DuniterCheckNonce<Inner: SignedExtension>(Inner) et dans l'impl du trait SignedExtension réécrire uniquement le comportement de la méthode pre_dispath dans le cas d'un call qui consomme un oneshot account. Il faut bien sur être transparent en appelant la méthode de Inner dans tout les autres cas.

Cherche les occurences à CheckNonce dans la codebase et tu verra ou est ce que c'est injecté pour chaque runtime

added 18 commits

e775eae8...8bc5e17f - 16 commits from branch master
984d95d8 - PoC about oneshot accounts
5b6686a1 - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

added 1 commit

0406fec8 - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

added 74 commits

0406fec8...33630d52 - 72 commits from branch master
6d4f4bfb - PoC about oneshot accounts
2a6e392b - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

@librelois Je crois qu'il ne reste qu'à faire prendre les frais sur le oneshot account plutôt que sur le compte normal.

Juste pour confirmer : il faut utiliser un OnChargeTransaction custom, c'est ça ?

Il faut aussi décider, pour consume_oneshot_account_two_dests, si on prend les frais sur un destinataire, ou également sur les deux, ou avec un ratio défini par l'utilisateur. Je dirais que sur un seul c'est plus pratique pour l'utilisateur et plus rapide à calculer.

added RN-runtime label

@tuxmain il faut créer une implémentation du trait pallet_transaction_payment::OnChargeTransaction dans la pallet oneshot_account qui doit prentre un générique Inner et apeller Inner dans tous les cas sauf si le call est censé être émis par un oneshot account. Si le call est consé être émis par un oneshot account il faut prélever les fauis au oneshot account, pas au destinataire. Il ne faut jamais prelever des frais à un compte qui n'a pas signé pour ça, ce serait une grave faille de sécurité.

@tuxmain aussi je pense qu'il faut déplacer tout ça dans une nouvelle pallet oneshot_account , et bouger également le CheckNonce dedans. Oui c'est plus dur mais ça me semble être un découplage essentiel. La fonctionnalité "onethot account" que nous ajoutons ici n'est pas spécifique à duniter, donc ça doit être dans sa propre pallet, pallet qu'on pourra d'ailleurs extraire dans son propre dépôt à l'avenir quand elle sera mature.

added 8 commits

2a6e392b...a674fd7b - 6 commits from branch master
0f5c53f7 - PoC about oneshot accounts
b96d8380 - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

added 7 commits

b96d8380...4415b0f5 - 5 commits from branch master
64d1a812 - PoC about oneshot accounts
b866eb8c - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

@librelois J'ai implémenté OnChargeTransaction, dans withdraw_fee je prélève fee si possible en respectant ExistentialDeposit, sinon le compte est supprimé.

On devrait pouvoir ne pas respecter le ExistentialDeposit si le compte est consommé vers un compte normal existant, mais si le call échoue alors on aura un compte oneshot < ExistentialDeposit... Ou alors il faut que le call lui-même en cas d'échec supprime le compte s'il est < ExistentialDeposit ?

@tuxmain keep it simple, le paiement des frais ne doit pas causer la destruction du compte, c'est comme ça que fonctionne déjà tout les call de substrate, donc on fait pareil.

resolved all threads

added 1 commit

a106114d - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

added 3 commits

57ac5589 - 1 commit from branch master
8ae64c4b - PoC about oneshot accounts
09a91c43 - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

added 1 commit

ead18738 - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

added 1 commit

eb91ce7f - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

@librelois Le test d'intégration passe, mais les tests concombre échouent et dans PolkadotJS, createOneshotAccount donne l'erreur :

signAndSend: error: RpcError: 1002: Verification Error: Runtime error: Execution failed:
ApiError(
  FailedToConvertParameter {
    function: "validate_transaction",
    parameter: "tx",
    error: Error {
      cause: None,
      desc: "Could not decode `Call`, variant doesn't exist"
    }
  }
)

(et si je change la variante du MultiAddress, c'est "could not decode [autre chose]".

@tuxmain je viens de tester sur ta branche et les tests cucumber fonctionne très bien une fois corrigés, je te laisse trouver tout selu comment les corriger, un indice toutefois: il faut parfois tenir compte des frais.

En revanche, polkadotjs fait effectivement n’importe-quoi, en plus dans les blocs qui contiennent un extrinsic avec un call oneshot account, polkadotjs indique que c'est un system.killPrefix, je pense que c'est parce qu'il garde en cache les metadata de la ğdev, mais je ne sais pas comment vérifier ça ni comment le forcer à update ses metadata. En tout les cas on ne va pas perdre du temps à essayer de fixer polkadotjs, il faut plutôt via cucumber ou/et tester en ligne de commande :)

added 1 commit

9024a09f - feat(duniter-account): finalized PoC oneshot account

Compare with previous version

changed milestone to %runtime-400

@tuxmain peut tu stp faire le nettoyage dans tes changements pour qu'on puisse y voir clair, la c'est galère a review, notamment supprime tout les changements dans la pallet duniter-account, sauf ceux dont tu à strictement besoin.

Je te recommande de faire un reset soft pour review toi même tout tes changements et ne commiter que ce qui est vraiment indispensable pour cette MR :)

Aussi, il faut bouger CheckNonce dans la pallet oneshot, et bien sur ajouter la généricité nécessaire pour que ça fonctionne. Inspire toi de comment est écrit CheckNonce dans frame_system, il faut constamment s'inspirer du code de substrate lui-même.

EDIT: @tuxmain aussi, désormais tout nouveaux call ou hook devrait être benchmarker dans la MR qui l'ajoute, je vais donc te demander d'ajouter les benchmark tests pour les 3 calls de la pallet oneshot, tu peut t'inspirer de comment j'ai fait pour la pallet universal dividend dans ce commit: !73 (a0b05d23)

Si tu rencontre des difficultés, n’hésite pas à me demander une visio ou une session de peer programming, j'ai conscience que ce que je te demande est difficile, mais c'est la réalité de ce qu'il faut faire pour une contribution complète :)

added 1 commit

cd1b9021 - feat(oneshot-account): Pallet oneshot-account

Compare with previous version

added 2 commits

dedaa23e - feat(oneshot-account): Pallet oneshot-account
ccf38862 - feat(oneshot-account): benchmarking

Compare with previous version

added 1 commit

a52f16e4 - feat(oneshot-account): benchmarking

Compare with previous version

added 16 commits

a52f16e4...ae65f879 - 15 commits from branch master
39c4dfad - feat(oneshot-account): Pallet oneshot-account

Compare with previous version

added 5 commits

39c4dfad...4bf87cec - 4 commits from branch master
166fee3a - feat(oneshot-account): Pallet oneshot-account

Compare with previous version

marked this merge request as ready

added 1 commit

76e7c186 - fix: metadata should comply subxt & polkadotjs expectations

Compare with previous version

@tuxmain après analyse cette MR n'est pas du tout terminée, les modules mock, test et benchmarking ne sont pas déclarés dans le lib.rs donc même pas compilés, quand je les ajoutent rien ne compile.

Merci d'inclure réellement les tests et benchmarking et de les faire fonctionner.

added 1 commit

04c788a0 - fix(oneshot-account): use benchmarking

Compare with previous version

Admin message

Oneshot accounts

Merge request reports

Activity