-
Notifications
You must be signed in to change notification settings - Fork 12
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
Feature/tx fees #101
Feature/tx fees #101
Conversation
) | ||
); | ||
|
||
const root = this.feeTree.tree.getRoot(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't this it is a good idea to inline this logic that way for multiple reasons. It gets really hard to reason about what parts of that tree make it into the circuit and which don't. So I would like to see this logic (i.e. the tree + witness generation) in a decoupled unit. What I would do is to create a @singleton service consuming the runtime, building the tree and creating a getRoot() and getWitness() method. Also, I'd use bigint for getRoot() to make sure that there is no coupling and it is a constant
) | ||
); | ||
|
||
const balances = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that in the constructor. No particular reason but I get scared when I see circuit and non-circuit code mixed like this.
|
||
public setBalance(tokenId: TokenId, address: PublicKey, amount: Balance) { | ||
const key = new BalancesKey({ tokenId, address }); | ||
this.events.emit("setBalance", key, amount); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not really happy with that, since that requires a extra PR to work properly (would currently fire multiple times for different values without a way to really use them properly)
// Has to be super.config, getters behave really weird when subtyping | ||
const config = super.config?.[moduleName]; | ||
|
||
const config = this.config?.[moduleName]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the configuration functionality. Execute ContainerEvents.test.ts and see for yourself
… into feature/tx-fees
No description provided.