Skip to content
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

Update ledger.py to include manager field otherwise it's ignored #12

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aorumbayev
Copy link
Contributor

Asset creation in update_accounts overlooked manager field assignment.
This PR simply includes the missing field reference

@aorumbayev
Copy link
Contributor Author

aorumbayev commented Mar 4, 2023

Also i noticed that something is weird going on in that method,

Aside from this typo. When I have an asset created by contract in innertxn - the manager field clawback and rest are set correctly.

Upon further call from user's wallet to opt-in to that asset (minted by contract) - ledger somehow overwrites the fields in the asset because params dict already contains decoded bytes of the address, and sending encode_address on it again overwrites the manager address.

I'll try to supply an example to replicate the error once i get time

@aorumbayev
Copy link
Contributor Author

@fergalwalsh perhaps i misunderstood how to do opt-in s for assets in algojig, i was sending an actual asset optin txn and evaluating it with ledger that's what caused weird behaviour - do i need to rely on optin_asset method if i want to do so?

Regardless, I think that manager field should be included

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant