Skip to content

Add origins in activity#373

Merged
overseers-desk merged 12 commits into
masterfrom
correct-violation-of-rfc4912+add-origins-in-activity
Jul 20, 2020
Merged

Add origins in activity#373
overseers-desk merged 12 commits into
masterfrom
correct-violation-of-rfc4912+add-origins-in-activity

Conversation

@overseers-desk

@overseers-desk overseers-desk commented Jul 5, 2020

Copy link
Copy Markdown

As per TokenScript weekly meeting #40, developers should base their Activity A3 project on the files provided in ERCs directory. Notice that this is based on PR #365 which will be merged in a few weeks since αW is released with a fix

@overseers-desk overseers-desk changed the title Correct violation of rfc4912+add origins in activity Add origins in activity (based on Correct violation of rfc4912) Jul 5, 2020
@overseers-desk
overseers-desk force-pushed the correct-violation-of-rfc4912+add-origins-in-activity branch from 0b100af to eb0aadc Compare July 5, 2020 02:48
…nt to fit JavaScript which uses the word 'amount'
@hboon

hboon commented Jul 6, 2020

Copy link
Copy Markdown
Member

Something to clear up before commenting on the JavaScript API changed to support this PR, instead of doing this:

web3.tokens.dataChanged = (oldTokens, updatedTokens, tokenIdCard) => {
    const currentTokenInstance = web3.tokens.data.currentInstance;
    document.getElementById(tokenIdCard).innerHTML = new Token(currentTokenInstance).render();
};

We should be doing this (i.e. not access web3.tokens.data.currentInstance):

web3.tokens.dataChanged = (oldTokens, updatedTokens, tokenIdCard) => {
    const currentTokenInstance = updatedTokens.currentInstance;
    document.getElementById(tokenIdCard).innerHTML = new Token(currentTokenInstance).render();
};

The latter assumes this:

{
  updatedTokens: {
    currentInstance: {
      tokenAttribute1: value,
      tokenAttribute2: value,
    }
  }
}

And for this PR

Let's just get rid of currentInstance once and for all, so web3 (under window) is not populated with currentInstance anymore:

{
  web3: {
    tokens: {
      data: {
        currentInstance: {
          tokenAttribute1: value,
          tokenAttribute2: value,
        }
      }
    }
  }
}

Instead, let's make the dataChanged handler work this way:

web3.tokens.dataChanged = (old, updated, tokenIdCard) => {
    const data = updated
    document.getElementById(tokenIdCard).innerHTML = new Token(data).render()
}

where updated (and old) looks like this:

updated = {
  token: {
    tokenAttribute1: value,
    tokenAttribute2: value,
  },
  card: {
    cardAttribute1: value,
    cardAttribute2: value,
  }
}

the developer can choose assign updated directly as props and thus access token attribute like this:

const tokenAttributeValue = this.props.token.tokenAttribute1
const cardAttributeValue = this.props.card.tokenAttribute1

And if they are sure the attribute names don't clash, this is easier to migrate to since only the dataChanged handler needs to be modified:

web3.tokens.dataChanged = (old, updated, tokenIdCard) => {
    const data = Object.assign({}, updated.token, updated.card)
    document.getElementById(tokenIdCard).innerHTML = new Token(data).render()
}

Then this is unchanged:

const attributeValue = this.props.tokenAttribute1 //or this.props.cardAttribute1

All of these assumes that we have at most 1 token, at most 1 card within each firing of dataChanged() Is that correct @colourful-land ?

@hboon

hboon commented Jul 6, 2020

Copy link
Copy Markdown
Member

So web3.tokens.dataChanged is the only place where we tuck things under web3. We can either change it now or next time. But it should be a very localised change (just that 1 line) when it happens.

@hboon

hboon commented Jul 13, 2020

Copy link
Copy Markdown
Member

@colourful-land have updated the JavaScript docs.

@hboon

hboon commented Jul 14, 2020

Copy link
Copy Markdown
Member

I suggest we preserve the value of updatedTokens.currentInstance for a while, i.e roughly this pseudo-code when injecting:

updatedTokens.currentInstance == updatedTokens.token + updatedTokens.card

so that the following JavaScript code in existing TokenScript files doesn't need to be updated immediately:

web3.tokens.dataChanged = (oldTokens, updatedTokens, tokenIdCard) => {
    const currentTokenInstance = updatedTokens.currentInstance;
    document.getElementById(tokenIdCard).innerHTML = new Token(currentTokenInstance).render();
};

cc @James-Sangalli

@hboon

hboon commented Jul 14, 2020

Copy link
Copy Markdown
Member
  • ping James Brown if the PR is good and gets merged so he is in the loop

Comment thread ERCs/ERC721/ERC721-TokenScript.xml Outdated
@hboon

hboon commented Jul 15, 2020

Copy link
Copy Markdown
Member

Merge branch 'master' into correct-violation-of-rfc4912+add-origins-i…

git rebase master instead? :)

@overseers-desk

Copy link
Copy Markdown
Author

Merge branch 'master' into correct-violation-of-rfc4912+add-origins-i…

git rebase master instead? :)

Not sure if anyone has mod in local copy - also didn't like rebase

@overseers-desk

Copy link
Copy Markdown
Author

@James-Sangalli :

if rebased this message wouldn't appear in git log
@colourful-land Merge branch 'master' into correct-violation-of-rfc4912+add-origins-i…9d8061e

I don't know why it is a problem to be solved…

@JamesANZ

Copy link
Copy Markdown
Contributor

tokenAttribute1

@hboon I prefer this syntax for the web3 object as it separates them and prevents collisions (while also letting you know what is global and what is based on cards)

web3.tokens.dataChanged = (oldTokens, updated, tokenIdCard) => {
    document.getElementById(tokenIdCard).innerHTML = new Token(updated.token, updated.card).render();
};

constructor(token, card) {
        this.token = token;
        this.card = card;
}

@hboon

hboon commented Jul 15, 2020

Copy link
Copy Markdown
Member

tokenAttribute1

@hboon I prefer this syntax for the web3 object as it separates them and prevents collisions (while also letting you know what is global and what is based on cards)

web3.tokens.dataChanged = (oldTokens, updated, tokenIdCard) => {
    document.getElementById(tokenIdCard).innerHTML = new Token(updated.token, updated.card).render();
};

constructor(token, card) {
        this.token = token;
        this.card = card;
}

Up to you if that's a better way to write the JavaScript for the views. What you suggested is the same as the one I suggested for what the TokenScript engine exposes as an API, i.e there is updated.token and updated.card (and for the time being, hopefully briefly, the legacy updated.currentInstance) in here:

web3.tokens.dataChanged = (oldTokens, updated, tokenIdCard) => {
}

@overseers-desk overseers-desk changed the title Add origins in activity (based on Correct violation of rfc4912) Add origins in activity Jul 16, 2020
@JamesSmartCell

Copy link
Copy Markdown
Contributor

This PR seems to cover a diverse range of updates.

  1. The dataChanged method - this is how it was always supposed to be working, right? So far it's been a slow evolution toward this.
  2. The origins in activity is a good refactor and will make the parsing code more concise - so, it's acceptable :)

@overseers-desk
overseers-desk marked this pull request as ready for review July 20, 2020 09:36
@overseers-desk
overseers-desk merged commit 2bc5140 into master Jul 20, 2020
@overseers-desk
overseers-desk deleted the correct-violation-of-rfc4912+add-origins-in-activity branch July 20, 2020 09:37
overseers-desk added a commit that referenced this pull request Jun 30, 2023
…+add-origins-in-activity

Add origins in activity
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.

4 participants