-
Notifications
You must be signed in to change notification settings - Fork 135
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
Handling account aliases on the Concordium blockchain (rosetta-cli) #422
Comments
@bisgardo hi, thanks for raising up this question, if I understand correctly, the mapping method of |
Hi @jingweicb thanks for looking into this! Yes Example: Consider two distinct account addresses The bookkeeper has seen and recorded both The solution above fixes the problem by hiding the concept of account aliases entirely. I think a better solution would do the mapping only when reading and writing the bookkeeper's state. That is, in pseudocode:
becomes
and similar for read. But as described above I didn't manage to implement a working solution for this. |
Btw. you can see the solution we ended up using at coinbase/mesh-cli@master...Concordium:rosetta-cli:concordium. Is a little more complex that described in the original issue because account aliases didn't exist from the beginning but was introduced in a protocol update. So instead of storing the canonical address, we store a mapping from the "zero" alias to the first observed address. For old accounts this is the only valid address, for new accounts it's the canonical alias. But conceptually it's pretty much the same as it's still just a fixed mapping from one address to another. |
@bisgardo thanks for your context, I am looking for a way to minimize the changes |
Thank you @jingweicb. Any news? :) |
@bisgardo , thanks for you patience! Generally, we tend to handle the account aliases in rosetta-implementation level. Is there any way to record a unique address for each account in rosetta-implementation? |
Thanks @jingweicb, now it's my turn to apologize for the late response ;) I don't think mapping the accounts in our Rosetta impl is feasible as it would make it stateful, which I would very much prefer it not to be. Mapping to the "zeroth" alias would be possible it not for the fact that old accounts don't have aliases as explained in a previous comment. But even if we could, I think users would be confused to have the addresses change in ways that aren't obvious to them and probably not sympathize with this being done only to satisfy a test tool. I don't feel like Rosetta should have an opinion on what alias to show, but rather show what's actually recorded in the blocks. Does this make sense? May I ask what is holding you back from allowing the mapping to done in the CLI? Note that I don't suggest implementing any kind of native support. The SDK change is only to allow us to implement it in our own fork. |
@bisgardo thanks for your clarification the reason why we do not want to change cli and sdk are also rosetta implementation do not have to be stateless, and users will not see all the alias in our platform |
@bisgardo here are updates from my side |
Is your feature request related to a problem? Please describe.
On the Concordium blockchain we have the concept of account aliases, i.e. only the first 29 (of 32) bytes of an account address are significant. This confuses
rosetta-cli check:data
as it has no way of knowing if two addresses actually reference the same account. If an operation was performed for some address, it will observe that the balance of the "account" of the aliases changed for no reason.Describe the solution you'd like
I've been trying to find a way to patch
rosetta-cli
(viarosetta-sdk-go
) to translate all addresses into the "canonical" one. I've gotten it working by adding the following abomination of a hack intoSyncer.processBlock
:where
canonicalizeAccountAddr
does a lookup using the Concordium SDK and replaces the address in-place.I've not been able to find a proper place to implement the behavior as
Syncer
,StatefulSyncer
,Fetcher
, etc. are all structs and not interfaces.Helper
is an interface type but it's not being constructed in a factory that can be overriden.I'm not sure if monkey patching the block result is the correct solution, but it's the only one I've gotten to work (also tried
BalanceStorage.AddingBlock
andParser.BalanceChanges
but those seemed insufficient).Describe alternatives you've considered
Patching all accounts in our Rosetta implementation. That would defeat the purpose of having aliases and also be prohibitively expensive.
Additional context
Any ideas of the correct way to implement this are greatly appreciated. I'll be happy to propose a PR with the necessary changes to for example support overriding the behavior of
Syncer
.The text was updated successfully, but these errors were encountered: